Re: [PATCH v3 3/5] arm64: dts: qcom: agatti: add LPASS devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] dt-bindings: qcom,agatti: unit address mismatch for lpasscc node
- [Low] dt-bindings: qcom,agatti: swr nodes lack status disabled or pinctrl
--

commit 0dce088f4d3617832ec5dd0787d22b15acf31f29
Author: Alexey Klimov <alexey.klimov@linaro.org>

arm64: dts: qcom: agatti: add LPASS devices
    
This commit adds the rxmacro, txmacro, vamacro, soundwire nodes, and lpass
clock controllers required to support audio playback and capture on sm6115
and its derivatives.

> diff --git a/arch/arm64/boot/dts/qcom/agatti.dtsi b/arch/arm64/boot/dts/qcom/agatti.dtsi
> index e7fed60a59d93..6d99e24e3dc2a 100644
> --- a/arch/arm64/boot/dts/qcom/agatti.dtsi
> +++ b/arch/arm64/boot/dts/qcom/agatti.dtsi

[ ... ]

> @@ -2238,6 +2274,127 @@ glink-edge {
>  			};
>  		};
>  
> +		rxmacro: codec@a600000 {

[ ... ]

> +		swr1: soundwire@a610000 {
> +			compatible = "qcom,soundwire-v1.6.0";
> +			reg = <0x0 0x0a610000 0x0 0x10000>;
> +			interrupts = <GIC_SPI 297 IRQ_TYPE_LEVEL_HIGH>;

[Severity: Low]
Does this node need to be disabled by default?

The swr1 RX soundwire node is added to the common .dtsi without a
status = "disabled" property or default pinctrl properties. If a downstream
board file includes this .dtsi but does not explicitly configure the pinctrl
or disable the node, the driver will unconditionally probe and fail to
operate correctly due to unlinked pins. 

Would it be better to disable external bus interfaces in common includes?

> +
> +			clocks = <&rxmacro>;
> +			clock-names = "iface";

[ ... ]

> +		swr0: soundwire@a740000 {
> +			compatible = "qcom,soundwire-v1.6.0";
> +			reg = <0x0 0x0a740000 0x0 0x10000>;
> +			interrupts = <GIC_SPI 296 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;

[Severity: Low]
Similarly for the swr0 TX soundwire node, should this node also be disabled
or provided with default pinctrl properties to prevent unconditional probing
on boards that do not configure it?

> +			clocks = <&txmacro>;
> +			clock-names = "iface";

[ ... ]

> +		lpasscc: clock-controller@a7ec000 {
> +			compatible = "qcom,sm6115-lpasscc";
> +			reg = <0x0 0x0a7e0000 0x0 0x12000>;

[Severity: Low]
Is there a typo in the unit address or the reg property here?

The node name clock-controller@a7ec000 does not match the base address in
its reg property (0x0a7e0000). This causes a simple_bus_reg validation
warning during dtc compilation.

> +			#reset-cells = <1>;
> +		};
> +
>  		remoteproc_adsp: remoteproc@ab00000 {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790114902.git.srinivas.kandagatla@oss.qualcomm.com?part=3