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

[Konrad Dybcio]

On 5/10/26 6:05 PM, Alexandre Hamamdjian via B4 Relay wrote:
> From: Teguh Sobirin <teguh@xxxxxxxx>
> 
> Add initial device tree support for the Ayaneo Pocket DS gaming
> console based on the Qualcomm QCS8550 platform.
> 
> The board file is split into qcs8550-ayaneo-pocket-common.dtsi and
> qcs8550-ayaneo-pocketds.dts in anticipation of additional boards in
> the same family that share the QCS8550 base. Only the Pocket DS has
> been verified on hardware so far; the shared dtsi is intended to pick
> up sibling boards as they are submitted.
> 
> Co-developed-by: Alexandre Hamamdjian <azkali.limited@xxxxxxxxx>
> Signed-off-by: Alexandre Hamamdjian <azkali.limited@xxxxxxxxx>
> Signed-off-by: Teguh Sobirin <teguh@xxxxxxxx>
> ---

[...]

> +		splash_region: splash_region@b8000000 {

underscores in node names are frowned  upon

> +			reg = <0x0 0xb8000000 0x0 0x2b00000>;
> +			label = "cont_splash_region";

Is this label any useful?

> +			no-map;
> +		};
> +	};
> +
> +	sound {
> +		status = "okay";

It's already 'okay' if previously not disabled, please drop this line


> +		compatible = "qcom,sm8550-sndcard", "qcom,sm8450-sndcard";
> +		model = "SM8550-APS";
> +		audio-routing = "SpkrLeft IN", "WSA_SPK1 OUT",
> +				"SpkrRight IN", "WSA_SPK2 OUT",
> +				"VA DMIC0", "vdd-micb",
> +				"VA DMIC1", "vdd-micb";
> +
> +		wsa-dai-link {
> +			link-name = "WSA Playback";
> +
> +			cpu {

'co'dec < 'cp'u < 'p'latform

> +				sound-dai = <&q6apmbedai WSA_CODEC_DMA_RX_0>;
> +			};
> +
> +			codec {
> +				sound-dai = <&spk_amp_l>,
> +					<&spk_amp_r>,
> +					<&swr0 0>,
> +					<&lpass_wsamacro 0>;

Please align these entries

[...]

> +	thermal-zones {
> +		cpuss0-thermal {

You should really be overriding nodes by &labels, this is fragile as-is,
since even if someone deletes the thermal zone nodes, DTC will happily
compile this (and it won't work)

> +			polling-delay = <200>;
> +			trips {

Let's add an \n between the last property and the followng subnodes

> +				cpuss0_fan0: trip-point2 {
> +					temperature = <40000>;
> +					hysteresis = <3000>;
> +					type = "passive";
> +					};

Please remove the extra \t here and separate subsequent nodes
with a \n

[...]

> +	vdd_fan_5v0: vdd-fan-5v0-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vdd_fan_5v0";
> +
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +
> +		gpio = <&tlmm 31 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&fan_pwr_active>;

property-n
property-names

file-wide, please

> +
> +		regulator-state-mem {
> +			regulator-off-in-suspend;
> +		};
> +	};
> +
> +	vph_pwr: regulator-vph-pwr {
> +		compatible = "regulator-fixed";
> +		regulator-name = "  vph_pwr";

stray spaces after "


> +&gpu {
> +	status = "okay";
> +
> +	zap-shader {

Please use the &gpu_zap_shader label to override

> +		firmware-name = "qcom/sm8550/a740_zap.mbn";
> +	};
> +};
> +
> +&gpu_opp_table {
> +	/* Additional speedbin for Adreno A32
> +	 *	719000000  RPMH_REGULATOR_LEVEL_SVS_L2
> +	 *	746000000 RPMH_REGULATOR_LEVEL_NOM
> +	 *	794000000 RPMH_REGULATOR_LEVEL_NOM_L1
> +	 *	827000000 RPMH_REGULATOR_LEVEL_TURBO
> +	 *	860000000 RPMH_REGULATOR_LEVEL_TURBO_L1
> +	 *	1000000000 RPMH_REGULATOR_LEVEL_TURBO_L2
> +	 */
> +	opp-719000000 {
> +		opp-hz = /bits/ 64 <719000000>;
> +		opp-level = <RPMH_REGULATOR_LEVEL_SVS_L2>;
> +		opp-peak-kBps = <10687500>; // Level 7

Let's drop these inline comments

[...]

> +&mdss_dsi0 {
> +	vdda-supply = <&vreg_l3e_1p2>;
> +	status = "okay";

Please keep an \n before 'status'

[...]

> +&mdss_dp0_out {
> +	data-lanes = <0 1>;
> +};

Only 2 lanes? Are the other 2 not wired up?


> +
> +&pcie0 {
> +	wake-gpios = <&tlmm 96 GPIO_ACTIVE_HIGH>;
> +	perst-gpios = <&tlmm 94 GPIO_ACTIVE_LOW>;
> +
> +	max-link-speed = <2>;

Are there issues with Gen3-speed links?

[...]

> +&i2c2 {
> +	clock-frequency = <400000>;
> +	status = "okay";
> +
> +	touchscreen@5d {
> +		reg = <0x5d>;
> +		compatible = "goodix,gt911";
> +
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <39 IRQ_TYPE_EDGE_FALLING>;
> +
> +		reset-gpios = <&tlmm 38 GPIO_ACTIVE_HIGH>;
> +		irq-gpios = <&tlmm 39 GPIO_ACTIVE_HIGH>;
> +		VDDIO-supply = <&vdd_ts>;
> +
> +		touchscreen-size-x = <768>;
> +		touchscreen-size-y = <1024>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&ts2_reset>, <&ts2_irq>;
> +
> +	};

Stray \n above

Does userspace somehow map these touchpanels to the right display,
or do you need to bind them somehow?

Konrad