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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Medium] Missing DisplayPort AUX (SBU) Routing in Type-C connector.
- [Medium] Missing thermal `cooling-maps` and incorrect trip point types.
- [Medium] Invalid DAPM widget in `audio-routing`.
- [Medium] Missing `pinctrl-1` property in `pwm_fan` node.
- [Medium] Improper Audio Amplifier Initialization via pinctrl.
- [Low] Downstream device tree property `rocknix-u-boot-dt-id` included in root node.
- [Low] Missing `compatible` string in `panel@0` node.
- [Low] Unreferenced `volume_up_n` pinctrl state, missing Volume Up button.
- [Low] Extraneous `orientation-gpios` property in `pmic-glink`.
- [Low] Deprecated `gpio` property used in fixed regulators.
--

commit 22f111b3d5157012d6f5d9d466cfb26e61ed3b56
Author: Teguh Sobirin <teguh@sobir.in>

arm64: dts: qcom: add basic devicetree for Ayaneo Pocket DS gaming console

Add initial Device Tree for the Ayaneo Pocket DS gaming console based
on the Qualcomm QCS8550 platform.

> diff --git a/arch/arm64/boot/dts/qcom/qcs8550-ayaneo-pocketds.dts b/arch/arm64/boot/dts/qcom/qcs8550-ayaneo-pocketds.dts
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/qcs8550-ayaneo-pocketds.dts
> @@ -0,0 +1,1861 @@
> +// SPDX-License-Identifier: BSD-3-Clause
[ ... ]
> +/ {
> +	model = "AYANEO Pocket DS";
> +	compatible = "ayaneo,pocketds", "qcom,qcs8550", "qcom,sm8550";
> +	rocknix-u-boot-dt-id = "u-boot-pocket-ds";

Is this downstream U-Boot property intended for upstream submission?
It does not appear to be documented in the upstream DT bindings.

> +
> +	aliases {
[ ... ]
> +	tca6424_vcc: regulator-tca6424-vcc {
> +		compatible = "regulator-fixed";
> +		regulator-name = "tca6424_vcc";
> +
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&tlmm 168 GPIO_ACTIVE_HIGH>;

This isn't a bug, but could we use gpios here instead of the deprecated
gpio property? This also applies to vdd_ts and vdd_fan_5v0 later in the file.

> +		enable-active-high;
> +		regulator-boot-on;
[ ... ]
> +	pmic-glink {
> +		compatible = "qcom,sm8550-pmic-glink", "qcom,pmic-glink";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		orientation-gpios = <&tlmm 11 GPIO_ACTIVE_HIGH>;

Does the qcom,pmic-glink driver read orientation-gpios? 

The driver appears to rely on IPC with the firmware to determine Type-C
orientation, which would make this property unused here.

> +
> +		connector@0 {
> +			compatible = "usb-c-connector";
> +			reg = <0>;
> +			power-role = "dual";
> +			data-role = "dual";
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				port@0 {
> +					reg = <0>;
> +
> +					pmic_glink_hs_in: endpoint {
> +						remote-endpoint = <&usb_1_dwc3_hs>;
> +					};
> +				};
> +
> +				port@1 {
> +					reg = <1>;
> +
> +					pmic_glink_ss_in: endpoint {
> +						remote-endpoint = <&redriver_ss_out>;
> +					};
> +				};
> +			};
> +		};

The commit message states DisplayPort is muxed over the USB-C connector, but
port@2 (SBU) is missing. Is SBU routing needed here for the DP AUX channel
(link training and EDID) to ensure DisplayPort Alt Mode functions correctly?

> +	sound {
> +		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";

Is vdd-micb a valid DAPM widget name here? 

It looks like the regulator supply name for the VA macro. Will DAPM fail to
parse this route and leave the digital microphones' bias voltage disabled?

[ ... ]
> +	pwm_fan: pwm-fan {
> +		compatible = "pwm-fan";
> +
> +		pinctrl-0 = <&fan_pwm_active>, <&fan_int>;
> +		pinctrl-names = "default", "sleep";

Are we missing pinctrl-1 here? 

Since pinctrl-names defines a sleep state, won't the PM core fail when
attempting to look up pinctrl-1 during transitions?

[ ... ]
> +&cpuss0_thermal {
> +	polling-delay = <200>;
> +
> +	trips {
> +		cpuss0_fan0: trip-point2 {
> +			temperature = <40000>;
> +			hysteresis = <3000>;
> +			type = "passive";
> +		};

Should the trip point type be active instead of passive since these are
intended for a fan?

Also, are the cooling-maps omitted intentionally across the thermal zones?
Without them, the kernel thermal framework cannot bind these trip points
to the pwm_fan cooling device.

[ ... ]
> +&pm8550_gpios {
[ ... ]
> +	volume_up_n: volume-up-n-state {
> +		pins = "gpio6";
> +		function = "normal";
> +		power-source = <1>;
> +		bias-pull-up;
> +		input-enable;
> +	};

This pinctrl state is defined but does not appear to be referenced anywhere.
Are we missing a gpio-keys device node to instantiate the Volume Up button?

[ ... ]
> +&tlmm {
[ ... ]
> +	spkr_1_sd_n_active: spkr-1-sd-n-active-state {
> +		pins = "gpio7";
> +		function = "gpio";
> +		drive-strength = <16>;
> +		bias-disable;
> +		output-high;
> +	};

Will setting output-high statically configure the active-low speaker shutdown
pins to drive high at boot? 

This might prematurely power up the amplifiers before the audio framework is
initialized, potentially causing an audible pop noise.

[ ... ]
> +&mdss_dsi0 {
> +	vdda-supply = <&vreg_l3e_1p2>;
> +
> +	status = "okay";
> +
> +	display_panel: panel@0 {
> +		reg = <0>;
> +
> +		pinctrl-0 = <&sde_dsi_active>, <&sde_te_active>;
> +		pinctrl-1 = <&sde_dsi_suspend>, <&sde_te_suspend>;
> +		pinctrl-names = "default", "sleep";
> +	};
> +};

Does this panel node need a compatible string? 

Even if panel support is being added later, leaving a child node on an
enabled DSI host incomplete violates the DT schema and will likely cause
the DSI core to fail when parsing child devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-pocketds-v2-0-299dd4247f2f@gmail.com?part=3