Re: [PATCH 3/3] arm64: dts: qcom: add basic devicetree for Ayaneo Pocket S2 gaming console

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 1/21/26 5:40 PM, Neil Armstrong wrote:
> From: KancyJoe <kancy2333@outlook.com>
> 
> Add initial Device Tree for the Ayaneo Pocket S2 gaming console based
> on the Qualcomm Snapdragon 8 Gen 3 platform.
> 
> The design is similar to a phone wihout the modem, the game control
> is handled via a standalone controller connected to a PCIe USB
> controller.
> 
> Display support will be added in a second time.
> 
> Signed-off-by: KancyJoe <kancy2333@outlook.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---

[...]

> +	fan: pwm-fan {

I'd call it fan {} but gray/grey

> +		status = "okay";

You can drop this line (nothing disables it)

> +		compatible = "pwm-fan";
> +
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <14 IRQ_TYPE_EDGE_FALLING>;

interrupts-extended looks neater

> +
> +		pinctrl-0 = <&fan_pwr_active>,
> +			    <&pwm_fan_ctrl_default>,
> +			    <&fan_int_active>;
> +		pinctrl-1 = <&fan_pwr_sleep>;

fan-pwr looks like an EN pin of a GPIO-controlled regulator

> +		pinctrl-names = "default",
> +				"sleep";
> +
> +		pwms = <&pm8550_pwm 3 50000>;
> +
> +		#cooling-cells = <2>;
> +		cooling-levels = <0 16 32 45 60 80 105 130 155 180 205 230 255>;

Does this come from a preexisting map?

> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +
> +		pinctrl-0 = <&volume_up_n>;
> +		pinctrl-names = "default";
> +
> +		key-volume-up {
> +			label = "Volume Up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			gpios = <&pm8550_gpios 6 GPIO_ACTIVE_LOW>;
> +			debounce-interval = <15>;
> +			linux,can-disable;
> +			wakeup-source;
> +		};
> +	};
> +
> +	pmic-glink {
> +		compatible = "qcom,sm8650-pmic-glink",
> +			     "qcom,sm8550-pmic-glink",
> +			     "qcom,pmic-glink";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		orientation-gpios = <&tlmm 29 GPIO_ACTIVE_HIGH>;
> +
> +		connector@0 {
> +			compatible = "usb-c-connector";
> +			reg = <0>;
> +
> +			power-role = "dual";
> +			data-role = "dual";
> +			self-powered;

Is this property interpreted at all by our setup?

[...]

> +	sound {
> +		compatible = "qcom,sm8650-sndcard", "qcom,sm8450-sndcard";
> +		model = "SM8650-APS2";
> +		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
> +				"SpkrRight IN", "WSA_SPK2 OUT",
> +				"IN1_HPHL", "HPHL_OUT",
> +				"IN2_HPHR", "HPHR_OUT",
> +				"DMIC1", "MIC BIAS1",
> +				"DMIC2", "MIC BIAS2",
> +				"AMIC2", "MIC BIAS2",
> +				"TX SWR_INPUT1", "ADC2_OUTPUT",
> +				"TX SWR_INPUT7", "DMIC1_OUTPUT",
> +				"TX SWR_INPUT8", "DMIC2_OUTPUT";
> +
> +		wcd-playback-dai-link {
> +			link-name = "WCD Playback";
> +
> +			cpu {
> +				sound-dai = <&q6apmbedai RX_CODEC_DMA_RX_0>;
> +			};
> +
> +			codec {
> +				sound-dai = <&wcd939x 0>,
> +					    <&swr1 0>,
> +					    <&lpass_rxmacro 0>;
> +			};

'co'dec < 'cp'u

[...]

> +	wcd939x: audio-codec {

'a'udio-codec should be way higher

[...]

> +	thermal-zones {
> +		cpu2-top-thermal {
> +			trips {
> +				cpu2_active: cpu2-active {
> +					temperature = <38000>;
> +					hysteresis = <2000>;
> +					type = "active";

This is shaky.. let's perhaps reference each thermal zone that you want
to extend with a label.. Or maybe a pair of labels for trips/cooling-maps
per zone?

[...]

> +&pcieport1 {
> +	pinctrl-0 = <&upd720201_active>;

Is this a regulator?

> +	pinctrl-names = "default";
> +
> +	/* Renesas μPD720201 PCIe USB3.0 HOST CONTROLLER */

DON'T SCREAM! :P

> +	usb-controller@0 {
> +		compatible = "pci1912,0014";
> +		reg = <0x10000 0x0 0x0 0x0 0x0>;
> +
> +		pinctrl-0 = <&gamepad_pwr_en>;
> +		pinctrl-names = "default";

Is there a hub connected to it? Or does it go directly to the
aforementioned (game) controller?

[...]

> +&pm8550_pwm {
> +	status = "okay";
> +
> +	multi-led {
> +		color = <LED_COLOR_ID_RGB>;
> +		function = LED_FUNCTION_STATUS;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Would a label="xyz" be useful here?

[...]

> +&tlmm {
> +	/* Reserved I/Os for NFC */
> +	gpio-reserved-ranges = <32 4>,  <36 1>, <38 6>, <74 1>;

double space

Are they all for NFC, are they all required?

[...]

> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 07ae74851621..fcd5a1a45803 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -3917,7 +3917,7 @@ opp-32000000-4 {
>  				};
>  			};
>  
> -			pcie@0 {
> +			pcieport1: pcie@0 {

pcie1_port0, please

Konrad