Re: [PATCH 1/2] dt-bindings: interconnect: Add Qualcomm CCI dt-bindings

[Krzysztof Kozlowski <krzk@kernel.org>]

On 01/02/2023 09:02, Jun Nie wrote:
> Add devicetree binding of Qualcomm CCI on MSM8939.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

Subject: drop second/last, redundant "dt-bindings". The "dt-bindings"
prefix is already stating that these are bindings.

> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  .../bindings/interconnect/qcom,cci.yaml       | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> new file mode 100644
> index 000000000000..100c440ba220
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,cci.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interconnect/mediatek,cci.yaml#

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Cache Coherent Interconnect (CCI) frequency and voltage scaling
> +
> +maintainers:
> +  - Jun Nie <jun.nie@linaro.org>
> +
> +description: |
> +  Qualcomm Cache Coherent Interconnect (CCI) is a hardware engine used by
> +  MSM8939. The driver is to scale its frequency and adjust the voltage in
> +  hardware accordingly. The voltage provider is modeled as power domain on
> +  MSM8939, so power domain dts node is required.

Don't describe other bindings, but the hardware. Last sentence is not
really related. What's more - it does not fit what you wrote below.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,msm8939-cci
> +
> +  clocks:
> +    maxItems: 1
> +
> +  operating-points-v2: true
> +  opp-table:
> +    type: object
> +
> +required:
> +  - compatible
> +  - clocks
> +  - operating-points-v2
> +  - nvmem-cells

?? You cannot require properties which are not present.

> +  - power-domains

Same here.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    cci: cci {
> +        compatible = "qcom,msm8939-cci";
> +	clocks = <&apcs2>;

Messed indentation.

Use 4 spaces for example indentation.

> +	operating-points-v2 = <&cci_opp_table>;
> +	power-domains = <&cpr>;
> +	nvmem-cells = <&cpr_efuse_speedbin_pvs>;

How does it pass testing?

Best regards,
Krzysztof