Re: [PATCH 2/5] arm64: dts: qcom: sc8180x: Add Microsoft Surface Pro X

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] arm64: dts: qcom: disabled qupv3_id_2 prevents uart13 and uart15 from probing
- [Medium] arm64: dts: qcom: build failure from undefined smb2351_2 and smb2351_a labels
- [Medium] arm64: dts: qcom: build failure from undefined remoteproc_slpi label
- [Medium] arm64: dts: qcom: vdda-phy-supply and vdda-pll-supply swapped on USB QMP PHYs
- [Medium] arm64: dts: qcom: missing power-supply and broken power sequencing on edp-panel
- [Low] arm64: dts: qcom: duplicate pmc8180a-thermal zone for pmc8180_temp sensor
- [Low] arm64: dts: qcom: child node names violate RSC, TLMM, PMIC GPIO, and ADC schemas
--

commit 5309d200eb0928bd90998981b5c8364f2e0ed95e
Author: Maximilian Luz <luzmaximilian@gmail.com>

arm64: dts: qcom: sc8180x: Add Microsoft Surface Pro X

This commit adds a common device tree include and three board device trees
for the Microsoft Surface Pro X tablet variants (Cambria, Carina, and
Caspar) based on the Qualcomm SC8180X platform.

> diff --git a/arch/arm64/boot/dts/qcom/sc8180x-microsoft-surface-pro-x.dtsi b/arch/arm64/boot/dts/qcom/sc8180x-microsoft-surface-pro-x.dtsi
> new file mode 100644
> index 0000000000000..17fb01c6b164f
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc8180x-microsoft-surface-pro-x.dtsi
> @@ -0,0 +1,853 @@

[ ... ]

> +	thermal-zones {
> +		pmc8180a-thermal {
> +			polling-delay-passive = <100>;
> +			polling-delay = <0>;
> +
> +			thermal-sensors = <&pmc8180_temp>;

[Severity: Low]
Should this node be named pmc8180-thermal instead of pmc8180a-thermal?

Because sc8180x-pmics.dtsi already defines /thermal-zones/pmc8180-thermal
with thermal-sensors = <&pmc8180_temp>, using the name pmc8180a-thermal
creates a second thermal zone node in the DTB for the same 0-cell sensor
rather than overriding the existing pmc8180-thermal node. When
qpnp_tm_probe() registers pmc8180_temp via devm_thermal_of_zone_register(),
thermal_of_zone_register() only binds the first matching zone
(pmc8180-thermal) and leaves pmc8180a-thermal unused.

[ ... ]

> +&apps_rsc {
> +	pmc8180-a-rpmh-regulators {
> +		compatible = "qcom,pmc8180-rpmh-regulators";
> +		qcom,pmic-id = "a";

[Severity: Low]
This isn't a runtime bug, but several child node names in this file do not
match the patternProperties required by their respective DT bindings
(which set additionalProperties: false) and will cause dtbs_check
validation failures:

1. Under &apps_rsc, pmc8180-a-rpmh-regulators, pmc8180-e-rpmh-regulators,
   and pmc8180c-rpmh-regulators do not match ^regulators(-[0-9])?$ in
   Documentation/devicetree/bindings/soc/qcom/qcom,rpmh-rsc.yaml.

2. Under &tlmm, the subnodes of uart13-state, uart15-state, and ssam-state
   (cts, rts-tx, rx, and wake-int) do not end with -pins as required by
   Documentation/devicetree/bindings/pinctrl/qcom,sc8180x-tlmm.yaml.

3. Under &pmc8180_1_gpios, pmc8180a_gpio1 and pmc8180a_gpio6 do not end
   with -state as required by
   Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml.

4. Under pmc8180e_adc and &pmc8180_adc, the channel child nodes
   (ref-gnd@0, vref-1p25@1, die-temp@6, xo-therm@4c, vph-pwr@83, and
   vcoin@85) do not match ^channel@[0-9a-f]+$ as required by
   Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml.

Could these node names be updated to match the binding schemas?

[ ... ]

> +&smb2351_2 {
> +	status = "disabled";
> +};
> +
> +&smb2351_a {
> +	status = "disabled";
> +};

[Severity: Medium]
Does this compile cleanly with dtc?

In arch/arm64/boot/dts/qcom/sc8180x-pmics.dtsi, the two qcom,smb2351 PMIC
nodes under &spmi_bus are defined without labels:

	pmic@2 {
		compatible = "qcom,smb2351", "qcom,spmi-pmic";
		reg = <0x2 SPMI_USID>;
		...
	};
	...
	pmic@a {
		compatible = "qcom,smb2351", "qcom,spmi-pmic";
		reg = <0xa SPMI_USID>;
		...
	};

Because neither smb2351_2 nor smb2351_a is defined anywhere in the tree,
building the new Surface Pro X DTB targets under CONFIG_ARCH_QCOM fails
with unresolved label errors.

[ ... ]

> +&mdss_edp {
> +	pinctrl-0 = <&edp_hpd_active>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +
> +	aux-bus {
> +		panel {
> +			compatible = "edp-panel";
> +			enable-gpios = <&tlmm 130 GPIO_ACTIVE_HIGH>;
> +			no-hpd;
> +
> +			pinctrl-0 = <&panel_vdd_en_default_state>;
> +			pinctrl-names = "default";

[Severity: Medium]
Should GPIO 130 be modeled as a fixed regulator connected to power-supply
on the panel node rather than enable-gpios?

Documentation/devicetree/bindings/display/panel/panel-edp.yaml requires
power-supply on compatible = "edp-panel" nodes. In addition, GPIO 130 is
the panel VDD regulator enable pin (panel-vdd-en-default-state). In
drivers/gpu/drm/panel/panel-edp.c, panel_edp_probe() requests enable-gpios
with GPIOD_OUT_LOW, which drives GPIO 130 low and cuts panel power, and
generic_edp_panel_probe() -> panel_edp_prepare_once() then re-asserts
enable_gpio and immediately attempts DP AUX EDID reads with zero delay
because no-hpd is set without hpd-absent-delay-ms.

[ ... ]

> +&usb_prim_qmpphy {
> +	vdda-phy-supply = <&vreg_l3c_1p2>;
> +	vdda-pll-supply = <&vreg_l9e_0p88>;
> +
> +	orientation-switch;
> +
> +	status = "okay";
> +};

[ ... ]

> +&usb_sec_qmpphy {
> +	vdda-phy-supply = <&vreg_l3c_1p2>;
> +	vdda-pll-supply = <&vreg_l9e_0p88>;
> +
> +	orientation-switch;
> +
> +	status = "okay";
> +};

[ ... ]

> +&usb_mp_qmpphy0 {
> +	vdda-phy-supply = <&vreg_l3c_1p2>;
> +	vdda-pll-supply = <&vreg_l9e_0p88>;
> +
> +	status = "okay";
> +};
> +
> +&usb_mp_qmpphy1 {
> +	vdda-phy-supply = <&vreg_l3c_1p2>;
> +	vdda-pll-supply = <&vreg_l9e_0p88>;
> +
> +	status = "okay";
> +};

[Severity: Medium]
Are vdda-phy-supply and vdda-pll-supply swapped on usb_prim_qmpphy,
usb_sec_qmpphy, usb_mp_qmpphy0, and usb_mp_qmpphy1?

On SC8180X QMP PHYs (including pcie1_phy and pcie2_phy below, as well as
sc8180x-primus.dts and sc8180x-lenovo-flex-5g.dts), vdda-phy-supply is the
0.88V rail and vdda-pll-supply is the 1.2V rail (&vreg_l3c_1p2). In
drivers/phy/qualcomm/phy-qcom-qmp-combo.c and
drivers/phy/qualcomm/phy-qcom-qmp-usb.c, qmp_phy_vreg_l sets a 21800 uA
load on vdda-phy and a 36000 uA load on vdda-pll, so swapping them applies
the wrong current load votes to the two LDOs.

[ ... ]

> +&remoteproc_slpi {
> +	memory-region = <&slpi_mem>;
> +};

[Severity: Medium]
Where is the remoteproc_slpi label defined?

Both sc8180x-microsoft-surface-pro-x.dtsi and the three board DTS files
(sc8180x-microsoft-cambria.dts, sc8180x-microsoft-carina.dts, and
sc8180x-microsoft-caspar.dts) reference &remoteproc_slpi, but
arch/arm64/boot/dts/qcom/sc8180x.dtsi only defines remoteproc_mpss,
remoteproc_cdsp, and remoteproc_adsp. Because remoteproc_slpi does not
exist in sc8180x.dtsi, dtc fails to build all three DTB targets.

> +
> +&uart13 {
> +	pinctrl-0 = <&uart13_state>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +
> +	bluetooth {
> +		compatible = "qcom,wcn3998-bt";
> +
> +		vddio-supply = <&vreg_s4a_1p8>;
> +		vddxo-supply = <&vreg_l7a_1p8>;
> +		vddrf-supply = <&vreg_l9a_1p3>;
> +		vddch0-supply = <&vreg_l11c_3p3>;
> +
> +		max-speed = <3200000>;
> +	};
> +};
> +
> +&uart15 {
> +	pinctrl-0 = <&uart15_state>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +
> +	embedded-controller {
> +		compatible = "microsoft,surface-sam";
> +
> +		interrupts-extended = <&tlmm 180 IRQ_TYPE_EDGE_RISING>;
> +
> +		current-speed = <4000000>;
> +
> +		pinctrl-0 = <&ssam_state>;
> +		pinctrl-names = "default";
> +	};
> +};

[Severity: High]
Don't we also need to enable &qupv3_id_2 for uart13 and uart15 to probe?

In arch/arm64/boot/dts/qcom/sc8180x.dtsi, uart13 and uart15 are children
of qupv3_id_2 (geniqup@cc0000), which defaults to status = "disabled":

		qupv3_id_2: geniqup@cc0000 {
			compatible = "qcom,geni-se-qup";
			...
			status = "disabled";

Child serial engines under qcom,geni-se-qup are populated when
geni_se_probe() calls devm_of_platform_populate(). Without setting
status = "okay" on &qupv3_id_2, the parent QUPv3 wrapper never probes and
neither uart13 (Bluetooth) nor uart15 (the Surface SAM embedded
controller) is instantiated.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104070441.74127-1-val@packett.cool?part=2