Re: [PATCH v2 7/7] arm64: dts: qcom: Add Microsoft Surface Pro 12in

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Device Tree and Driver mismatch for WSA8845 speaker reset GPIO
- [Low] Incorrect indentation in `&pm8550_gpios` node
--

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

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

This commit introduces the initial device tree for the Microsoft Surface Pro
12in, adding support for various peripherals including UFS, touchscreen,
USB, audio, Wi-Fi, and Bluetooth.

> 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..32b8df2497913
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/x1p42100-microsoft-sp12in.dts

[ ... ]

> +&pm8550_gpios {
> +    vol_up_n_default: vol-up-n-state {
> +		pins = "gpio8";
> +		function = "normal";
> +		power-source = <1>;
> +		bias-pull-up;
> +		input-enable;
> +	};
> +
> +    vol_down_n_default: vol-down-n-state {

This isn't a bug, but should these subnodes be indented with a hardware tab
instead of spaces to match standard device tree coding guidelines?

[ ... ]

> +&swr0 {
> +	pinctrl-0 = <&wsa_swr_active>, <&spkr_01_sd_n_active>;
> +	pinctrl-names = "default";
> +
> +	status = "okay";
> +
> +	/* WSA8845, Left speaker */
> +	left_spkr: speaker@0,0 {
> +		compatible = "sdw20217020400";
> +		reg = <0 0>;
> +		reset-gpios = <&lpass_tlmm 12 GPIO_ACTIVE_LOW>;

Will this prevent the speakers from enumerating on the SoundWire bus?

Looking at the WSA884x codec driver in sound/soc/codecs/wsa884x.c, it
requests this GPIO by the name "powerdown":

    devm_gpiod_get_optional(dev, "powerdown", ...)

Because the driver looks for the powerdown-gpios property, it will fail to
acquire the GPIO named reset-gpios here. Concurrently, the pinctrl state
spkr_01_sd_n_active drives this pin output-low, asserting the active-low
reset.

Since the driver never releases the reset, the speakers will stay powered
down and audio playback will fail. Should this property be renamed to
powerdown-gpios?

> +		#sound-dai-cells = <0>;
> +		sound-name-prefix = "SpkrLeft";
> +		vdd-1p8-supply = <&vreg_l15b_1p8>;
> +		vdd-io-supply = <&vreg_l12b_1p2>;
> +		qcom,port-mapping = <1 2 3 7 10 13>;
> +	};
> +
> +	/* WSA8845, Right speaker */
> +	right_spkr: speaker@0,1 {
> +		compatible = "sdw20217020400";
> +		reg = <0 1>;
> +		reset-gpios = <&lpass_tlmm 12 GPIO_ACTIVE_LOW>;

Does this speaker node have the same naming issue where it should use
powerdown-gpios instead of reset-gpios?

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