Re: [PATCH RFC 01/23] dt-bindings: clock: qcom: add SM8650 LPASS core & audio clock controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] dt-bindings: clock: qcom: broken examples in
  qcom,sm8650-lpasscorecc.yaml
- [Low] dt-bindings: clock: qcom: unwieldy per-compatible conditionals
  in schema
--

commit 822c52dcc683f4949600245b7d9f62d13802e752
Author: Neil Armstrong <neil.armstrong@linaro.org>

dt-bindings: clock: qcom: add SM8650 LPASS core & audio clock controllers

This commit adds the Device Tree binding schema and header definitions for
the SM8650 LPASS HM, core, audio, and AON clock controllers.

> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm8650-lpasscorecc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm8650-lpasscorecc.yaml
> new file mode 100644
> index 0000000000000..9c5207ee02f15
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,sm8650-lpasscorecc.yaml

[ ... ]

> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,sm8650-lpasshm
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Board XO source
> +        clock-names:
> +          items:
> +            - const: bi_tcxo
> +      required:
> +        - power-domains
> +        - '#power-domain-cells'
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,sm8650-lpasscorecc
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Board XO source
> +        clock-names:
> +          items:
> +            - const: bi_tcxo
> +        required-opps: false
> +      required:
> +        - power-domains
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,sm8650-lpassaoncc
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Board XO source
> +            - description: LPASS_CORE_CC_CORE_CLK
> +        clock-names:
> +          items:
> +            - const: bi_tcxo
> +            - const: iface
> +        power-domains: false
> +        required-opps: false
> +      required:
> +        - '#power-domain-cells'
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,sm8650-lpassaudiocc
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: Board XO source
> +            - description: LPASS_AON_CC_MAIN_RCG_CLK_SRC
> +        clock-names:
> +          items:
> +            - const: bi_tcxo
> +            - const: iface
> +        required-opps: false
> +      required:
> +        - power-domains

[Severity: Low]
Each of the four compatibles requires its own if-then block to override
clocks, clock-names, power-domains, required-opps, and
#power-domain-cells (which also leaves #power-domain-cells unconstrained
on qcom,sm8650-lpasscorecc and qcom,sm8650-lpassaudiocc).  Would it be
cleaner to split these variants into separate binding schema files instead
of using per-compatible conditionals for every entry?

> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,lpassaudiocc-sm8650.h>
> +    #include <dt-bindings/clock/qcom,lpasscorecc-sm8650.h>
> +    #include <dt-bindings/clock/qcom,sm8650-gcc.h>
> +    #include <dt-bindings/power/qcom,rpmhpd.h>
> +
> +    clock-controller@6bc0000 {
> +        compatible = "qcom,sm8650-lpassaudiocc";
> +        reg = <0x06bc0000 0x38000>;
> +        clocks = <&bi_tcxo_div2>;
> +        clock-names = "bi_tcxo";
> +        power-domains = <&lpass_aon LPASS_AON_CC_LPASS_AUDIO_HM_GDSC>;
> +        #clock-cells = <1>;
> +    };
> +
> +    lpass_aon: clock-controller@6e00000 {
> +        compatible = "qcom,sm8650-lpassaoncc";
> +        reg = <0x06e00000 0x30000>;
> +        clocks = <&bi_tcxo_div2>, <&lpass_core LPASS_CORE_CC_CORE_CLK>;
> +        clock-names = "bi_tcxo", "iface";
> +        #clock-cells = <1>;
> +        #power-domain-cells = <1>;
> +    };
> +
> +    lpass_core: clock-controller@7b00000 {
> +        compatible = "qcom,sm8650-lpasscorecc";
> +        reg = <0x07b00000 0x58000>;
> +        clocks = <&bi_tcxo_div2>;
> +        clock-names = "bi_tcxo";
> +        power-domains = <&lpass_hm LPASS_CORE_HM_GDSC>;
> +        #clock-cells = <1>;
> +    };
> +
> +    lpass_hm: clock-controller@7e00000 {
> +        compatible = "qcom,sm8650-lpasshm";
> +        reg = <0x07e00000 0xa000>;
> +        clocks = <&bi_tcxo_div2>, <&gcc GCC_LPASS_CFG_NOC_SWAY_CLK>;
> +        clock-names = "bi_tcxo", "iface";
> +        power-domains = <&rpmhpd RPMHPD_LCX>;
> +        required-opps = <&rpmhpd_opp_nom>;
> +        #clock-cells = <1>;
> +        #power-domain-cells = <1>;
> +    };

[Severity: Medium]
Does this example pass make dt_binding_check?

In lpass_hm, GCC_LPASS_CFG_NOC_SWAY_CLK is not defined in
include/dt-bindings/clock/qcom,sm8650-gcc.h, causing dtc to fail with a
syntax error.  In addition, lpass_hm specifies two clocks ("bi_tcxo" and
"iface") while the qcom,sm8650-lpasshm conditional schema only allows
"bi_tcxo", and clock-controller@6bc0000 (qcom,sm8650-lpassaudiocc) only
provides "bi_tcxo" while its conditional schema requires both "bi_tcxo"
and "iface".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=1