Re: [PATCH v5] arm64: dts: qcom: sm8550: add Xiaomi Pad 6S Pro (sheng)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Invalid clock reparenting for dual DSI synchronization.
- [High] Using `GPIO_OPEN_DRAIN` does not bypass gpiolib's exclusive access check for shared reset lines.
- [High] Severe overvoltage risk by configuring a 0.7V-named regulator to 1.9V.
- [Medium] Asymmetric speaker assignment breaks stereo audio routing.
- [Medium] Unnecessary memory allocation for non-existent modem RMTFS.
- [Low] Use of undocumented and downstream DT bindings.
--

--- Patch [1]: [PATCH v5] arm64: dts: qcom: sm8550: add Xiaomi Pad 6S Pro (sheng) ---
commit 84985aa6cc3ea47e6961c9fece55de9d4418b9fc
Author: map220v <map220v300@gmail.com>

arm64: dts: qcom: sm8550: add Xiaomi Pad 6S Pro (sheng)

Add device tree for Xiaomi Pad 6S Pro (sheng) based on SM8550.

> v4: address Sashiko AI review comments
> - Add GPIO_OPEN_DRAIN to shared cs35l43 reset-gpios to avoid -EBUSY

[Severity: High]
Will this still result in -EBUSY errors for 5 out of the 6 speaker
amplifiers? 

Adding GPIO_OPEN_DRAIN is an electrical configuration and does not instruct
gpiolib to bypass exclusive access checks.

> diff --git a/arch/arm64/boot/dts/qcom/sm8550-xiaomi-sheng.dts b/arch/arm64/boot/dts/qcom/sm8550-xiaomi-sheng.dts
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm8550-xiaomi-sheng.dts
[ ... ]
> +	reserved-memory {
[ ... ]
> +		rmtfs_mem: rmtfs-region@ccd00000 {
> +			compatible = "qcom,rmtfs-mem";
> +			reg = <0x0 0xccd00000 0x0 0x280000>;
> +			no-map;
> +
> +			qcom,client-id = <1>;
> +		};

[Severity: Medium]
Is this reserved memory region necessary? 

The commit message notes the device lacks a modem, but this node allocates
memory for client-id 1, which strictly maps to the MPSS remote file system
client.

[ ... ]
> +	regulators-5 {
> +		compatible = "qcom,pm8550vs-rpmh-regulators";
[ ... ]
> +		vreg_s3g_0p7: smps3 {
> +			regulator-name = "vreg_s3g_0p7";
> +			regulator-min-microvolt = <600000>;
> +			regulator-max-microvolt = <1900000>; //2004000

[Severity: High]
Could this create an overvoltage risk for other hardware relying on a 0.7V
rail? 

The regulator node name implies a nominally 0.7V rail, but it allows up to
1.9V. If this is a dedicated 1.8V rail, should the node be renamed to
reflect the hardware reality and prevent unsafe reuse?

> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};
[ ... ]
> +&i2c5 {
> +	clock-frequency = <1000000>;
> +	status = "okay";
> +
> +	cs35l43_trl: cs35l43@40 {
> +		compatible = "cirrus,cs35l43";
> +		reg = <0x40>;
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <98 IRQ_TYPE_LEVEL_LOW>;
> +		reset-gpios = <&tlmm 90 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;

[Severity: High]
Does this properly bypass gpiolib's exclusive access check for shared
reset lines? 

Setting GPIO_OPEN_DRAIN alone does not bypass exclusive access, so devices
probing after the first one might still receive -EBUSY.

> +		#sound-dai-cells = <0>;
[ ... ]
> +&i2c8 {
> +	clock-frequency = <1000000>;
> +	status = "okay";
> +
> +	cs35l43_blh: cs35l43@40 {
> +		compatible = "cirrus,cs35l43";
> +		reg = <0x40>;
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> +		reset-gpios = <&tlmm 90 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +		#sound-dai-cells = <0>;
> +		sound-name-prefix = "BLH";
> +		cirrus,dsp-part-name = "blh-cs35l43";

[Severity: Medium]
Does this asymmetric speaker assignment break stereo audio routing? 

There are left-side tweeter nodes defined (TLH and BLH) but no right-side
high speakers, which seems to create an asymmetric 4-left / 2-right topology.

> +		cirrus,vpbr-thld = <8>;
[ ... ]
> +&mdss_dsi0 {
> +	vdda-supply = <&vreg_l3e_1p2>;
> +	qcom,dual-dsi-mode;
> +	qcom,sync-dual-dsi;
> +	qcom,master-dsi;
> +	status = "okay";
> +
> +	display_panel: panel@0 {
> +		compatible = "xiaomi,sheng-nt36532e", "novatek,nt36532e";
> +		reg = <0>;
> +
> +		pinctrl-0 = <&sde_dsi_active>;
> +		pinctrl-1 = <&sde_dsi_suspend>;
> +		pinctrl-names = "default", "sleep";
> +
> +		vddio-supply = <&vreg_s3g_0p7>;

[Severity: High]
Is it safe to assign a nominally 0.7V regulator to the display panel's
vddio-supply here? This usage forces the voltage to the higher 1.9V limit.

> +		avdd-supply = <&bl_vddpos_5p8>;
[ ... ]
> +&mdss_dsi1 {
> +	vdda-supply = <&vreg_l3e_1p2>;
> +	qcom,dual-dsi-mode;
> +	qcom,sync-dual-dsi;
> +	/* DSI1 is slave, so use DSI0 clocks */
> +	assigned-clocks = <&mdss_dsi1_phy 0>, <&mdss_dsi1_phy 1>;
> +	assigned-clock-parents = <&mdss_dsi0_phy 0>, <&mdss_dsi0_phy 1>;

[Severity: High]
Will this clock reparenting fail during probe and break dual display
initialization? 

DSI PHY PLL output clocks are fixed to their local PLLs and do not have
configurable parents. Shouldn't the consumer dispcc clocks be parented to
the master PHY's outputs to properly sync dual DSI?

> +	status = "okay";
> +};
[ ... ]
> +&spi4 {
> +	status = "okay";
> +
> +	touchscreen@0 {
> +		compatible = "novatek,NVT-ts-spi";
> +		reg = <0>;
> +
> +		spi-max-frequency = <19200000>;
> +		novatek,irq-gpio = <&tlmm 25 GPIO_ACTIVE_LOW>;

[Severity: Low]
Will these undocumented downstream compatibles and properties fail dtbs_check
and prevent standard upstream drivers from binding?

> +
> +		panel = <&display_panel>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721150740.300-1-code_0002@163.com?part=1