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

Hi,

On 5/12/26 05:50, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

It's right, please drop this

> 
>> +
>> +	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.

"gpio" is still valid but we use gpios now

> 
>> +		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.

This is plain false, we use orientation-gpios since sm8550 since can't have the orientation
via ucsi messages.

> 
>> +
>> +		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?

Yeah please check, an SBU mux is certainly missing, or maybe the redriver is used
as SBU mux, the nb7vpq904m supports SBU muxing on port2/

> 
>> +	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?

It's right, drop this

> 
> [ ... ]
>> +	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?

It's right, please fix

> 
> [ ... ]
>> +&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?

Yes, please link those to the fan like on Ayaneo PS2

> 
> 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?

Add the gpios keys for the volume+

> 
> [ ... ]
>> +&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.

Drop all the output-high in tlmm, this comes from downstream

> 
> [ ... ]
>> +&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.
> 

Please drop all DSI related stuff including this panel and associated regulator &
pinctrl nodes and add them when the panel stuff is ready.

It seems you never tested your DT, please run :
make CHECK_DTBS=yes qcom/qcs8550-ayaneo-pocketds.dtb
before sending a v3.

Neil