Re: [PATCH v1 8/8] arm64: dts: qcom: Add Microsoft Surface Pro 12in

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Touchscreen reset pin is statically driven high before power is applied.
- [Medium] Audio routing specifies 4 DMICs but pinctrl only configures pins for DMIC0 and DMIC1.
- [Medium] Regulator enable pin statically forced high by pinctrl.
- [Medium] Inconsistent regulator assignment for USB QMP PHYs.
- [Medium] Missing pinctrl configurations for volume keys.
- [Low] Unused `panel_pins` pinctrl state defined in device tree.
--

commit bfa1ccc215b18064216c44242bb8398931a0520f
Author: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>

arm64: dts: qcom: Add Microsoft Surface Pro 12in

This commit adds the initial device tree for the Microsoft Surface
Pro 12in, enabling various peripherals such as UFS, touchscreen,
audio, and USB.

> diff --git a/arch/arm64/boot/dts/qcom/x1p42100-microsoft-sp12in.dts b/arch/arm64/boot/dts/qcom/x1p42100-microsoft-sp12in.dts
> new file mode 100644
> index 0000000000000..3a754179a8481
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/x1p42100-microsoft-sp12in.dts
> @@ -0,0 +1,1160 @@
[ ... ]
> +		key-vol-up {
> +			gpios = <&pm8550_gpios 8 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_VOLUMEUP>;
> +			debounce-interval = <15>;
> +			linux,can-disable;
> +			wakeup-source;
> +		};
> +
> +		key-vol-down {
> +			gpios = <&pm8550_gpios 6 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_VOLUMEDOWN>;
> +			debounce-interval = <15>;
> +			linux,can-disable;
> +			wakeup-source;
> +		};

Will these volume keys be prone to spurious interrupts? These nodes map
to pm8550_gpios as GPIO_ACTIVE_LOW, but do not specify any pinctrl to
configure internal pull-ups on the physical pads. Unless external
pull-ups are present on the board, floating pins could trigger unwanted
wakeups.

[ ... ]
> +	sound {
> +		compatible = "qcom,x1e80100-sndcard";
> +		model = "X1P42100-Microsoft-Surface-Pro-12in";
> +		audio-routing = "SpkrLeft IN", "WSA WSA_SPK1 OUT",
> +				"SpkrRight IN", "WSA WSA_SPK2 OUT",
> +				"VA DMIC0", "vdd-micb",
> +				"VA DMIC1", "vdd-micb",
> +				"VA DMIC2", "vdd-micb",
> +				"VA DMIC3", "vdd-micb";
[ ... ]
> +&lpass_vamacro {
> +	qcom,dmic-sample-rate = <4800000>;
> +
> +	vdd-micb-supply = <&vreg_l1b_1p8>;
> +
> +	pinctrl-0 = <&dmic01_default>;
> +	pinctrl-names = "default";
> +};

Will audio capture from DMIC2 and DMIC3 fail with this configuration?
The audio-routing maps 4 DMICs, but only the dmic01_default pins are
configured in pinctrl, leaving the data and clock pins for DMIC2 and
DMIC3 unconfigured. Should <&dmic23_default> be included here?

[ ... ]
> +	touchscreen@16 {
> +		compatible = "hid-over-i2c";
> +		reg = <0x16>;
> +
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 38 IRQ_TYPE_LEVEL_LOW>;
> +		wakeup-source;
> +
> +		vddl-supply = <&vreg_l15b_1p8>;
> +
> +		pinctrl-0 = <&ts0_default>;
> +		pinctrl-names = "default";
> +	};

Could this damage the touchscreen IC? The touchscreen node omits a
reset-gpios property, but relies on ts0_default which hardcodes
output-high on the reset pin.

Because pinctrl states are applied prior to the driver probe (which
enables vddl-supply), driving logic high into the unpowered IC violates
power sequencing and risks back-powering via ESD diodes.

Would it be better to provide reset-gpios in the touchscreen node so
the driver can manage the line dynamically after regulators stabilize?

[ ... ]
> +&tlmm {
> +	gpio-reserved-ranges = <34 2>, /* Unused */
> +				   <44 4>; /* SPI (TPM) */
> +
> +	edp_reg_en: edp-reg-en-state {
> +		pins = "gpio70";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-high;
> +	};

Does this prevent the regulator framework from turning off vreg_edp_3p3?
Hardcoding output-high in the pinctrl state overrides the regulator's
ability to autonomously pull the line low when the supply is disabled,
such as during system suspend.

[ ... ]
> +	panel_pins: panel-pins-state {
> +		pins = "gpio29";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;
> +	};

This isn't a bug, but panel_pins is defined here yet never referenced
by the panel node or any other component. Is it dead code, or is a
reference to it missing elsewhere to properly enable the display?

[ ... ]
> +	 ts0_default: ts0-default-state {
> +		int-n-pins {
> +			pins = "gpio38";
> +			function = "gpio";
> +			bias-disable;
> +		};
> +
> +		reset-n-pins {
> +			pins = "gpio48";
> +			function = "gpio";
> +			output-high;
> +			drive-strength = <16>;
> +		};
> +	 };

(This is where output-high is currently defined for the touchscreen reset,
as mentioned above.)

[ ... ]
> +&usb_1_ss0_qmpphy {
> +	vdda-phy-supply = <&vreg_l3e_1p2>;
> +	vdda-pll-supply = <&vreg_l1j_0p8>;
> +
> +	status = "okay";
> +};

Should this use vreg_l2j_1p2 for the vdda-phy-supply instead?

Mixing a PHY supply from PMIC 'e' with a PLL supply from PMIC 'j' on
the same node differs from usb_1_ss1_qmpphy, which consistently uses
vreg_l2j_1p2. Using vreg_l3e_1p2 here looks similar to the pcie4_phy
configuration, which could be a copy-paste error resulting in improper
voltage scaling for this USB PHY.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778498477.git.harrison.vanderbyl@gmail.com?part=8