Re: [PATCH 2/2] arm64: dts: qcom: add ASUS Vivobook S16 S3607QA (X1P42100)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential
issues to consider:
- [Medium] arm64: dts: qcom: missing x1p42100-asus-vivobook-s16-el2.dtb
  target in Makefile
- [Medium] arm64: dts: qcom: vivobook-s16: missing regulator-always-on
  on vreg_l15b_1p8
- [Low] arm64: dts: qcom: use of deprecated firmware-name property in
  wifi@0 node
- [Low] arm64: dts: qcom: SoC CPU thermal zones overridden via path refs
  in board DTS
- [Low] arm64: dts: qcom: misplaced WCN comment and unsorted nodes in
  vivobook-s16
- [Low] arm64: dts: qcom: x1p42100-asus-vivobook-s16: unused i2c1 and
  qupv3_2 enabled
--

commit 104a00c9fa1edca7f449af5c0a4e3e5e2801e6a7
Author: Ingo A. Kubbilun <ingo.kubbilun@gmail.com>

arm64: dts: qcom: add ASUS Vivobook S16 S3607QA (X1P42100)

This commit adds the device tree for the ASUS Vivobook S16 S3607QA laptop
based on the Qualcomm Snapdragon X X1P42100 SoC.

> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 88230c4e00ea9..11cd956c082a5 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -509,6 +509,7 @@ x1e80100-qcp-el2-dtbs	:= x1e80100-qcp.dtb x1-el2.dtbo
>  dtb-$(CONFIG_ARCH_QCOM)	+= x1e80100-qcp.dtb x1e80100-qcp-el2.dtb
>  x1p42100-asus-vivobook-s15-el2-dtbs	:= x1p42100-asus-vivobook-s15.dtb x1-el2.dtbo
>  dtb-$(CONFIG_ARCH_QCOM)	+= x1p42100-asus-vivobook-s15.dtb x1p42100-asus-vivobook-s15-el2.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= x1p42100-asus-vivobook-s16.dtb
>  x1p42100-asus-zenbook-a14-el2-dtbs	:= x1p42100-asus-zenbook-a14.dtb x1-el2.dtbo
>  dtb-$(CONFIG_ARCH_QCOM)	+= x1p42100-asus-zenbook-a14.dtb x1p42100-asus-zenbook-a14-el2.dtb
>  x1p42100-asus-zenbook-a14-lcd-el2-dtbs	:= x1p42100-asus-zenbook-a14-lcd.dtb x1-el2.dtbo

[Severity: Medium]
Should this also define x1p42100-asus-vivobook-s16-el2-dtbs and add
x1p42100-asus-vivobook-s16-el2.dtb like the other X1 targets?  Without
the composite rule, x1p42100-asus-vivobook-s16-el2.dtb is not built and
the base DTB is not compiled with symbols for applying x1-el2.dtbo when
booting at EL2.

> diff --git a/arch/arm64/boot/dts/qcom/x1p42100-asus-vivobook-s16.dts b/arch/arm64/boot/dts/qcom/x1p42100-asus-vivobook-s16.dts
> new file mode 100644
> index 0000000000000..7dc4f4bd94e5c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/x1p42100-asus-vivobook-s16.dts

[ ... ]

> +	vreg_wcn_3p3: regulator-wcn-3p3 {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_WCN_3P3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&tlmm 214 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-0 = <&wcn_sw_en>;
> +		pinctrl-names = "default";
> +
> +		regulator-boot-on;
> +	};
> +
> +	/*
> +	 * TLMM 116/117 (BT/WLAN enable) are listed in ACPI BTH0 and QPPX.
> +	 * WCN_3P3 switch on TLMM 214 is not visible in ACPI; it is the same
> +	 * on every ASUS X1 design in mainline (S15, Zenbook A14).
> +	 */
> +	us_euro_mux_ctrl: mux-controller {
> +		compatible = "gpio-mux";
> +		pinctrl-0 = <&us_euro_hs_sel>;
> +		pinctrl-names = "default";
> +		mux-supply = <&vreg_l16b_2p5>;
> +		#mux-control-cells = <0>;
> +		mux-gpios = <&tlmm 68 GPIO_ACTIVE_HIGH>;
> +	};

[Severity: Low]
This isn't a bug, but the comment about the WCN GPIOs (TLMM 116/117 and
TLMM 214) is attached to mux-controller instead of wcn6855-pmu.
Also, mux-controller in the root node (and us-euro-hs-sel-state and
wcd-reset-n-active-state in &tlmm below) are out of alphabetical order.

[ ... ]

> +		vreg_l15b_1p8: ldo15 {
> +			regulator-name = "vreg_l15b_1p8";
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> +		};

[Severity: Medium]
Should vreg_l15b_1p8 have regulator-always-on, matching vreg_l12b_1p2
above and other X1 laptops (see commit 06c8fc3e132c ("arm64: dts: qcom:
x1-dell-thena: mark l12b and l15b always-on"))?

Unlike on the Vivobook S15 where wcn7850-pmu also consumes vreg_l15b_1p8,
wcn6855-pmu here uses vreg_wcn_1p9, leaving only the audio codecs
(wcd938x, left_spkr and right_spkr) as consumers in DT.  If the audio
codec drivers defer probe or are unbound, the regulator framework will
disable vreg_l15b_1p8 and cut power to shared board components not
modeled in DT.

[ ... ]

> +&i2c1 {
> +	clock-frequency = <400000>;
> +	status = "okay";
> +};

[Severity: Low]
Should &i2c1 (and &qupv3_2 below) be left disabled since neither has any
child devices enabled on this board?  Enabling &i2c1 causes
geni_i2c_probe() to mux gpio4 and gpio5 to qup0_se1, vote for QUP0
interconnect bandwidth, and initialize an unused I2C controller.

[ ... ]

> +&pcie4_port0 {
> +	reset-gpios = <&tlmm 146 GPIO_ACTIVE_LOW>;
> +	wake-gpios = <&tlmm 148 GPIO_ACTIVE_LOW>;
> +
> +	/* ACPI PCI4.RP1.WLTM; WCN6855 (FastConnect 6900, Wi-Fi 6E) */
> +	wifi@0 {
> +		compatible = "pci17cb,1103";
> +		reg = <0x10000 0x0 0x0 0x0 0x0>;
> +
> +		/*
> +		 * NFA765 module (Windows BDF: bdwlan_wcn685x_2p1_nfa765a_*).
> +		 * Use the NFA765 firmware build from linux-firmware
> +		 * (ath11k/WCN6855/hw2.1/nfa765/{amss,m3}.bin); the default
> +		 * WLAN.HSP.1.1-03125 LITE build crashes after BDF download.
> +		 */
> +		firmware-name = "nfa765";

[Severity: Low]
Commit adce4fa49961 ("dt-bindings: net: wireless: ath11k-pci: deprecate
'firmware-name' property") deprecated firmware-name on pci17cb,1103 nodes
in favor of ath11k_core_usecase_firmware_table in
drivers/net/wireless/ath/ath11k/core.c (introduced in commit c386a2b10689
("wifi: ath11k: add usecase firmware handling based on device
compatible")).  Should "asus,vivobook-s16-s3607qa" be added to
ath11k_core_usecase_firmware_table instead of using firmware-name here?

[ ... ]

> +/*
> + * CPU thermal throttling. hamoa.dtsi only has 115 C critical trips for the
> + * CPU zones, so under sustained load (e.g. make -j8) the SoC ran into the
> + * hardware/EC protection and powered off abruptly. Add a passive trip at
> + * 90 C that lets the thermal core reduce the cluster frequency (scmi-cpufreq
> + * cooling devices), like the GPU zones already do at 95 C.
> + */
> +&{/thermal-zones/cpu0-0-top-thermal} {
> +	polling-delay-passive = <250>;
> +
> +	trips {
> +		cpu0_0_top_alert: cpu-alert {
> +			temperature = <90000>;
> +			hysteresis = <2000>;
> +			type = "passive";
> +		};
> +	};
> +
> +	cooling-maps {
> +		map0 {
> +			trip = <&cpu0_0_top_alert>;
> +			cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +						 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +						 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +						 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +		};
> +	};
> +};

[Severity: Low]
Both the TSENS sensors and the SCMI cpufreq cooling devices are
SoC-internal components defined in hamoa.dtsi, where commit 06eadce93697
("arm64: dts: qcom: x1e80100: Drop unused passive thermal trip points for
CPU") dropped the SoC passive trips.  If TSENS-based passive cooling is
needed across Purwa/Hamoa boards, should this live in the SoC DTSI (or use
&thermal_zones / phandle labels) instead of repeating 20
&{/thermal-zones/...} path overrides in the board DTS?

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