Re: [PATCH 2/2] 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 8 potential issue(s) to consider:
- [High] Missing `compatible` string on the primary display panel.
- [High] Missing `cooling-maps` for fan thermal trip points.
- [Medium] PCIe peripheral power and enable signals are hardcoded via pinctrl.
- [Medium] Conflicting `output-low` pinctrl state for a regulator-managed GPIO.
- [Medium] Missing `pinctrl-1` property in the `pwm_fan` node.
- [Low] Inclusion of undocumented, downstream-specific devicetree property `rocknix-u-boot-dt-id`.
- [Low] Leading spaces in `vph_pwr` regulator name.
- [Low] Orphaned pinctrl nodes for a missing secondary display.
--

commit a48e1f08f4a1d52011fe6aeaf547a479fe5b3a94
Author: Teguh Sobirin <teguh@sobir.in>
arm64: dts: qcom: add basic devicetree for Ayaneo Pocket DS gaming console

This commit adds initial device tree support for the Ayaneo Pocket DS
gaming console based on the Qualcomm QCS8550 platform.

> diff --git a/arch/arm64/boot/dts/qcom/qcs8550-ayaneo-pocket-common.dtsi b/arch/arm64/boot/dts/qcom/qcs8550-ayaneo-pocket-common.dtsi
> new file mode 100644
> index 0000000000000..8d215a9d31d54
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/qcs8550-ayaneo-pocket-common.dtsi

[ ... ]

> +	thermal-zones {
> +		cpuss0-thermal {
> +			polling-delay = <200>;
> +			trips {
> +				cpuss0_fan0: trip-point2 {
> +					temperature = <40000>;
> +					hysteresis = <3000>;
> +					type = "passive";
> +					};

Should these thermal zones include a cooling-maps block? Without it, the
passive trip points don't seem to bind to the pwm_fan cooling device, meaning
the thermal framework won't automatically scale the fan based on temperature.

[ ... ]

> +	vph_pwr: regulator-vph-pwr {
> +		compatible = "regulator-fixed";
> +		regulator-name = "  vph_pwr";

This isn't a bug, but there are leading spaces in the regulator name. Will
this cause formatting issues in sysfs or break userspace tools that parse the
regulator tree?

> +		regulator-min-microvolt = <3700000>;
> +		regulator-max-microvolt = <3700000>;
> +
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};
> +
> +	pwm_fan: pwm-fan {
> +		compatible = "pwm-fan";
> +
> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&fan_pwm_active>, <&fan_int>;

Should this node include a pinctrl-1 property? The sleep state is declared in
pinctrl-names, but the corresponding configuration is missing, which might
cause the pinctrl framework to fail during power management transitions.

> +
> +		fan-supply = <&vdd_fan_5v0>;
> +		pwms = <&pm8550_pwm 3 40000>;

[ ... ]

> +	fan_pwr_active: fan-pwr-active-state {
> +		pins = "gpio31";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;
> +	};

Does the output-low property here conflict with the regulator subsystem? This
state is used by the vdd_fan_5v0 fixed regulator, which already manages the
GPIO's output direction and logic level natively.

[ ... ]

> +&mdss_dsi0 {
> +	vdda-supply = <&vreg_l3e_1p2>;
> +	status = "okay";
> +
> +	display_panel: panel@0 {
> +		reg = <0>;
> +
> +		pinctrl-0 = <&sde_dsi_active>, <&sde_te_active>;

Is this panel missing a compatible string? Without it, the DRM panel driver
will not be able to probe and bind to the hardware, rendering the primary
display non-functional.

> +		pinctrl-1 = <&sde_dsi_suspend>, <&sde_te_suspend>;
> +		pinctrl-names = "default", "sleep";
> +	};
> +};

[ ... ]

> +&pcie1 {
> +	wake-gpios = <&tlmm 99 GPIO_ACTIVE_HIGH>;
> +	perst-gpios = <&tlmm 97 GPIO_ACTIVE_LOW>;
> +
> +	pinctrl-0 = <&pcie1_default_state>, <&upd720201_active>, <&gamepad_pwr_en>;

Is it intended to manage the downstream USB controller and gamepad enable
GPIOs directly at the PCIe host level?

Hardcoding them here bypasses the proper device-specific power management and
might break suspend/resume sequencing since the peripherals will remain
unconditionally powered while the PCIe controller is active.

> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +};

[ ... ]

> 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
> index 0000000000000..723b027fa73bd
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/qcs8550-ayaneo-pocketds.dts

[ ... ]

> +/ {
> +	model = "AYANEO Pocket DS";
> +	compatible = "ayaneo,pocketds", "qcom,sm8550";
> +	rocknix-u-boot-dt-id = "u-boot-pocket-ds";

Is this downstream-specific property intended to be upstreamed? Devicetrees
usually describe hardware topology rather than downstream OS or bootloader
identifiers.

> +
> +	tca6424_vcc: regulator-tca6424-vcc {

[ ... ]

> +&tlmm {
> +	gpio-reserved-ranges = <32 4>;
> +	panel_pwr_active: panel-pwr-active-state {
> +		pins = "gpio152", "gpio153";
> +		function = "gpio";
> +		drive-strength = <8>;
> +		bias-disable;
> +		output-high;
> +	};
> +
> +	sde_dsi1_active: sde-dsi1-active-state {
> +		pins = "gpio137";
> +		function = "gpio";
> +		drive-strength = <8>;
> +		bias-disable;
> +		output-high;
> +	};
> +
> +	sde_dsi1_suspend: sde-dsi1-suspend-state {
> +		pins = "gpio137";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-pull-down;
> +	};

Are these pinctrl states needed? They appear to be orphaned since the
secondary display node (&mdss_dsi1) is not defined in this devicetree.

> +
> +	ts_rst_default: ts-rst-default-state {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-pocketds-v1-0-cf05acec06af@gmail.com?part=2