Re: [PATCH 1/2] arm64: dts: qcom: add device tree for Asus Vivobook 16 X1607QA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [High] arm64: dts: qcom: missing Signed-off-by trailer and commit
  message body
- [Medium] arm64: dts: qcom: missing /delete-node/ for pmc8380_6 on
  Purwa X1P42100
- [Medium] arm64: dts: qcom: missing dmic-sample-rate and
  vdd-micb-supply on lpass_vamacro
- [Medium] arm64: dts: qcom: Vivobook 16 DT inherits Vivobook S 15 panel
  and board dtsi
- [Medium] arm64: dts: qcom: Makefile missing x1-el2.dtbo target and
  alphabetical order
- [Low] arm64: dts: qcom: undocumented compatible
  asus,vivobook-16-x1607qa in bindings
- [Low] arm64: dts: qcom: split root nodes, unsorted labels, and gpu
  override style
--

commit 273985d644ee175b794950960be28ab5824da749
Author: yass27106 <yass27106@gmail.com>

> arm64: dts: qcom: add device tree for Asus Vivobook 16 X1607QA
> 
> arch/arm64/boot/dts/qcom/Makefile             |   1 +
>  .../x1p42100-asus-vivobook-16-x1607qa.dts     | 239 ++++++++++++++++++
>  2 files changed, 240 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/qcom/x1p42100-asus-vivobook-16-x1607qa.dts

[Severity: High]
This commit is missing a Signed-off-by: trailer, uses a handle instead of
a real name in the Author header, and has a raw diffstat in place of a
commit description explaining the hardware and supported components.

> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 88230c4e00ea9..7332a73dbac71 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-16-x1607qa.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-16-x1607qa-el2-dtbs and add
x1p42100-asus-vivobook-16-x1607qa-el2.dtb like the other X1E/X1P boards?
Without the composite rule, the build neither produces the -el2.dtb image
nor compiles the base DTB with symbol metadata for x1-el2.dtbo, which is
needed for EL2 boot.  Also, x1p42100-asus-vivobook-16-x1607qa belongs
before x1p42100-asus-vivobook-s15 in alphabetical order.

> diff --git a/arch/arm64/boot/dts/qcom/x1p42100-asus-vivobook-16-x1607qa.dts b/arch/arm64/boot/dts/qcom/x1p42100-asus-vivobook-16-x1607qa.dts
> new file mode 100644
> index 0000000000000..fc572d779b311
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/x1p42100-asus-vivobook-16-x1607qa.dts
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/* Hybrid device tree for ASUS Vivobook 16 X1607QA (X1P-42-100).
> + * Base: proven x1p42100-asus-vivobook-s15 (panel, USB, boot path).
> + * Transplanted from x126100-asus-vivobook-16: audio stack
> + * (WCD9385 codec, WSA8845 speakers, soundwire, sndcard) + firmware paths.
> + */
> +
> +/dts-v1/;
> +
> +#include "purwa.dtsi"
> +#include "x1-asus-vivobook-s15.dtsi"

[Severity: Medium]
Does including x1-asus-vivobook-s15.dtsi without deleting &pmc8380_6,
&pmc8380_6_thermal, and &pmc8380_6_die_temp leave the third CPU cluster's
PMIC enabled on Purwa?  x1-asus-vivobook-s15.dtsi pulls in
hamoa-pmics.dtsi, so on 8-core X1P42100 where that PMIC is unpopulated,
the SPMI, VADC, and thermal drivers will probe and poll a non-existent
device at USID 6 unless those three nodes are deleted as in
x1p42100-asus-vivobook-s15.dts.

[Severity: Medium]
Does including x1-asus-vivobook-s15.dtsi directly also leave the 16-inch
16:10 Vivobook 16 (X1607QA) configured with the Vivobook S 15's 15.6-inch
16:9 2880x1620 eDP panel ("samsung,atna56ac03", "samsung,atna33xc20")
under &mdss_dp3 and other S5507-specific board definitions?

> +
> +/ {
> +	model = "ASUS Vivobook 16 (X1607QA)";
> +	compatible = "asus,vivobook-16-x1607qa", "qcom,x1p42100";

[Severity: Low]
"asus,vivobook-16-x1607qa" is not documented under the qcom,x1p42100
entry in Documentation/devicetree/bindings/arm/qcom.yaml, which causes
make dtbs_check to fail.

[ ... ]

> +&lpass_vamacro {
> +	pinctrl-0 = <&dmic01_default>;
> +	pinctrl-names = "default";
> +};

[Severity: Medium]
Should &lpass_vamacro also specify qcom,dmic-sample-rate and
vdd-micb-supply?  Without them, va_macro_probe() logs an error
("qcom,dmic-sample-rate dt entry missing") and ASoC DAPM substitutes a
dummy regulator for the vdd-micb widget instead of enabling the DMIC
supply rail.

> +
> +/ {
> +	wcd938x: audio-codec {
> +		compatible = "qcom,wcd9385-codec";

[ ... ]

> +&gpu {
> +	status = "okay";
> +
> +	zap-shader {
> +		firmware-name = "qcom/x1p42100/ASUSTeK/vivobook-16-x1607qa/qcdxkmsucpurwa.mbn";
> +	};
> +};

[Severity: Low]
This isn't a bug, but the second / { ... }; block above should be merged
into the root node at the top of the file, the &label overrides should be
sorted alphabetically, and the zap shader override can reference
&gpu_zap_shader directly instead of repeating status = "okay" on &gpu
(which is already enabled in x1-asus-vivobook-s15.dtsi).

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