Re: [PATCH 2/2] arm64: dts: qcom: Add support for Acer Swift Go Pro AI (SFA14-11)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] WSA8845 speakers use `reset-gpios` instead of `powerdown-gpios` and share the GPIO incorrectly.
- [High] Type-C Switch Configuration Error on Type-A Port due to inherited PHY properties.
- [Medium] Missing Audio DAI Link for HDMI output.
- [Medium] Commit message claims 'Webcam' support but no camera nodes are added.
- [Low] Inaccurate `orientation-gpios` array length in `pmic-glink`.
- [Low] The High-Speed USB PHY `usb_1_ss2_hsphy` is enabled despite no USB controller consuming it.
--

commit 61c654b8218e8d2d66d2d6d5ec3990a2ffe03533
Author: Kaipeng Zeng <kaipeng94@gmail.com>
Subject: arm64: dts: qcom: Add support for Acer Swift Go Pro AI (SFA14-11)

This commit introduces the device tree for the Acer Swift Go Pro AI (SFA14-11)
laptop, based on the Snapdragon X Elite SoC. It enables core hardware features
including internal display, USB, Wi-Fi, audio routing, and HDMI output.

> Supported features:
> 
>  - Internal display & Brightness control
>  - Keyboard & Touchpad
>  - USB Type-C, Type-A
>  - Wi-Fi & Bluetooth
>  - ADSP & CDSP
>  - Adreno X1-85 GPU
>  - HDMI & USB Type-C display
>  - Webcam

[Severity: Medium]
Is the webcam feature fully implemented in this patch? The commit message
lists Webcam as a supported feature, but a review of the device tree reveals
no camss, cci, or image sensor nodes added in this patch.

> diff --git a/arch/arm64/boot/dts/qcom/x1e78100-acer-sfa14-11.dts b/arch/arm64/boot/dts/qcom/x1e78100-acer-sfa14-11.dts
> new file mode 100644
> index 0000000000000..f5d91abf11f1d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/x1e78100-acer-sfa14-11.dts

[ ... ]

> +	pmic-glink {
> +		compatible = "qcom,x1e80100-pmic-glink",
> +			     "qcom,sm8550-pmic-glink",
> +			     "qcom,pmic-glink";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		orientation-gpios = <&tlmm 121 GPIO_ACTIVE_HIGH>,
> +				    <&tlmm 123 GPIO_ACTIVE_HIGH>,
> +				    <&tlmm 125 GPIO_ACTIVE_HIGH>;

[Severity: Low]
Does this array length match the number of connectors? The qcom,pmic-glink
bindings require the orientation-gpios array to contain exactly one GPIO entry
per connector node. This board explicitly defines only one connector, but
supplies three GPIOs here.

[ ... ]

> +	sound {
> +		compatible = "qcom,x1e80100-sndcard";
> +		model = "X1E80100-CRD";
> +		audio-routing = "WooferLeft IN", "WSA WSA_SPK1 OUT",
> +				"TweeterLeft IN", "WSA WSA_SPK2 OUT",
> +				"WooferRight IN", "WSA2 WSA_SPK1 OUT",
> +				"TweeterRight IN", "WSA2 WSA_SPK2 OUT",
> +				"IN1_HPHL", "HPHL_OUT",
> +				"IN2_HPHR", "HPHR_OUT",
> +				"AMIC2", "MIC BIAS2",
> +				"VA DMIC0", "MIC BIAS3",
> +				"VA DMIC1", "MIC BIAS3",
> +				"VA DMIC2", "MIC BIAS1",
> +				"VA DMIC3", "MIC BIAS1",
> +				"TX SWR_INPUT1", "ADC2_OUTPUT";
> +
> +		displayport-0-dai-link {
> +			link-name = "DisplayPort0 Playback";
> +
> +			codec {
> +				sound-dai = <&mdss_dp0>;
> +			};
> +
> +			cpu {
> +				sound-dai = <&q6apmbedai DISPLAY_PORT_RX_0>;
> +			};
> +
> +			platform {
> +				sound-dai = <&q6apm>;
> +			};
> +		};
> +
> +		wcd-playback-dai-link {

[Severity: Medium]
Will audio work over HDMI without a DAI link for mdss_dp2? The board routes
HDMI through a bridge connected to mdss_dp2, but the sound node only defines
a DAI link for mdss_dp0. Without displayport-2-dai-link, the audio driver
may not expose the HDMI audio endpoint to userspace.

[ ... ]

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

[Severity: High]
Should this use powerdown-gpios instead of reset-gpios? The ASoC WSA884x codec
driver does not look for reset-gpios, which means the GPIO is never toggled and
the speaker amplifiers might fail to power up.

Additionally, will sharing the GPIO directly here cause a failure when probing
the tweeter? The gpiod_get requests exclusive access, so the second probed
device could fail with -EBUSY. A gpio-reset controller is usually required to
share the line properly.

[ ... ]

> +	swr3 {
> +		pinctrl-0 = <&wsa2_swr_active>, <&spkr_23_sd_n_active>;
> +		pinctrl-names = "default";
> +
> +		status = "okay";
> +
> +		/* WSA8845, Right Woofer */
> +		right_woofer: speaker@0,0 {
> +			compatible = "sdw20217020400";
> +			reg = <0 0>;
> +			reset-gpios = <&lpass_tlmm 13 GPIO_ACTIVE_LOW>;

[Severity: High]
Similar to the left woofer, does this need to use powerdown-gpios and a
gpio-reset controller to avoid probe failures?

[ ... ]

> +&usb_1_ss1_qmpphy {
> +	vdda-phy-supply = <&vreg_l2j_1p2>;
> +	vdda-pll-supply = <&vreg_l2d_0p9>;
> +
> +	status = "okay";
> +};

[Severity: High]
Does this node need to explicitly delete the mode-switch and orientation-switch
properties? In hamoa.dtsi, usb_1_ss1_qmpphy is defined as a combo PHY and
contains those properties, which causes the PHY driver to register a Type-C
switch and default to TYPEC_ORIENTATION_NONE.

Since this board uses a regular Type-A port here and won't receive Type-C
orientation events, this can leave the USB 3.0 lanes permanently disabled,
degrading the port to USB 2.0.

[ ... ]

> +&usb_1_ss2_hsphy {
> +	vdd-supply = <&vreg_l3j_0p8>;
> +	vdda12-supply = <&vreg_l2j_1p2>;
> +
> +	status = "okay";
> +};

[Severity: Low]
Is it necessary to enable this High-Speed PHY? The usb_1_ss2 controller is
disabled because this port is exclusively used for DisplayPort output.
Enabling the PHY without a USB controller to consume it leaves a dangling
device probing indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702063156.35169-1-kaipeng94@gmail.com?part=2