Re: [PATCH 5/6] arm64: dts: qcom: sm8250-hdk: Add the QCA6390 NIC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: sm8250-hdk: vreg_3p3 added after pcie2 references it
- [Low] arm64: dts: qcom: sm8250-hdk: tlmm pin state node names violate schema
--

commit b98ad1ad783f0dda0bddcfceb9a3d14617f06e2d
Author: redstonee <1516235445@qq.com>

arm64: dts: qcom: sm8250-hdk: Add the QCA6390 NIC

This patch adds device tree nodes for the qca6390 wireless and bluetooth
module on the sm8250-hdk board, including its pmu node, regulators, pinctrl
configuration, uart6 bluetooth node, and pcieport0 wifi node.

> diff --git a/arch/arm64/boot/dts/qcom/sm8250-hdk.dts b/arch/arm64/boot/dts/qcom/sm8250-hdk.dts
> index 77273b6c2216a..eb899c92fcfb7 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250-hdk.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8250-hdk.dts
> @@ -32,6 +32,13 @@ vph_pwr: vph-pwr-regulator {
>  		regulator-max-microvolt = <3700000>;
>  	};
>  
> +	vreg_3p3: vreg-3p3-regulator {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vreg_3p3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};

[Severity: Medium]
Should vreg_3p3 be defined in the preceding commit be0d22fcb00f ("arm64: dts:
qcom: sm8250-hdk: Enable the PCIe ports") instead of this patch?

In commit be0d22fcb00f, &pcie2 in arch/arm64/boot/dts/qcom/sm8250-hdk.dts
references <&vreg_3p3>:

	&pcie2 {
		status = "okay";
		vddpe-3v3-supply = <&vreg_3p3>;
	};

Because vreg_3p3 is not defined until this commit (and is not consumed by any
of the qca6390 nodes on pcie0 or uart6), building sm8250-hdk.dtb at commit
be0d22fcb00f fails with a reference to a non-existent node or label
"vreg_3p3", breaking git bisect across the series.

[ ... ]

> @@ -450,6 +525,39 @@ rst-pins {
>  			output-high;
>  		};
>  	};
> +
> +	qca6390_pins: qca6390-pins {
> +		wlan-enable-pins {
> +			pins = "gpio20";
> +			function = "gpio";
> +			drive-strength = <16>;
> +			output-low;
> +			bias-pull-up;
> +		};
> +
> +		bt-enable-pins {
> +			pins = "gpio21";
> +			function = "gpio";
> +			drive-strength = <16>;
> +			output-low;
> +			bias-pull-up;
> +		};
> +	};
> +};

[Severity: Low]
Does the qca6390-pins node name under &tlmm conform to the binding schema in
Documentation/devicetree/bindings/pinctrl/qcom,sm8250-pinctrl.yaml?

The binding specifies unevaluatedProperties: false and requires direct child
pin configuration nodes under &tlmm (which matches "qcom,sm8250-pinctrl") to
end with "-state", while "-pins" is only permitted for nested subnodes inside
a "-state" node:

  patternProperties:
    "-state$":
      oneOf:
        - $ref: "#/$defs/qcom-sm8250-tlmm-state"
        - patternProperties:
            "-pins$":
              $ref: "#/$defs/qcom-sm8250-tlmm-state"
          additionalProperties: false

Because qca6390-pins (as well as sd-ufs-cd-pins and lt9611-pins added under
&tlmm earlier in the series) uses the "-pins" suffix directly under &tlmm,
running dtbs_check on sm8250-hdk.dtb fails schema validation. Should these
top-level pin state nodes under &tlmm use a "-state" suffix instead (for
example, qca6390-default-state)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022113601.2320940-1-dc_redstone@outlook.com?part=5