Re: [PATCH 01/10] dt: bindings: clock: add mtmips SoCs clock device tree binding documentation

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 20/03/2023 17:18, Sergio Paracuellos wrote:
> Adds device tree binding documentation for clocks and resets in the
> Mediatek MIPS and Ralink SOCs. This covers RT2880, RT3050, RT3052, RT3350,
> RT3883, RT5350, MT7620, MT7628 and MT7688 SoCs.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Subject: drop second/last, redundant "device tree binding
documentation". The "dt-bindings" prefix is already stating that these
are bindings.
(BTW, that's the longest redundant component I ever saw)

> 
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  .../bindings/clock/mtmips-clock.yaml          | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/mtmips-clock.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/mtmips-clock.yaml b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml
> new file mode 100644
> index 000000000000..c92969ce231d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/mtmips-clock.yaml

Filename matching compatible, so vendor prefix and device name (or
family of names).

> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/mtmips-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MTMIPS SoCs Clock

One clock? Are you sure these describe exactly one clock?

> +
> +maintainers:
> +  - Sergio Paracuellos <sergio.paracuellos@gmail.com>
> +
> +description: |
> +  MediaTek MIPS and Ralink SoCs have an XTAL from where the cpu clock is
> +  provided as well as derived clocks for the bus and the peripherals.
> +
> +  Each clock is assigned an identifier and client nodes use this identifier
> +  to specify the clock which they consume.

Drop useless or obvious pieces of description. Describe the hardware.

> +
> +  The clocks are provided inside a system controller node.

???

> +
> +  This node is also a reset provider for all the peripherals.

??? Does it mean it is not only "Clock" but also reset controller?

> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - ralink,rt2880-sysc
> +          - ralink,rt3050-sysc
> +          - ralink,rt3052-sysc
> +          - ralink,rt3352-sysc
> +          - ralink,rt3883-sysc
> +          - ralink,rt5350-sysc
> +          - ralink,mt7620-sysc
> +          - ralink,mt7620a-sysc
> +          - ralink,mt7628-sysc
> +          - ralink,mt7688-sysc
> +          - ralink,rt2880-reset

That's odd. rt2880 is sysc and reset? One device with two compatibles?

Also, order these by name.


> +      - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    description:
> +      The first cell indicates the clock number.
> +    const: 1
> +
> +  '#reset-cells':
> +    description:
> +      The first cell indicates the reset bit within the register.
> +    const: 1

Wait, only rt2880-reset is reset controller? This is confusing.

> +
> +required:
> +  - compatible
> +  - reg
> +  - '#clock-cells'
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    sysc: sysc@0 {

Drop label.

Node names should be generic, clock-controller or reset-controller or
system-controller sometimes.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +      compatible = "ralink,rt5350-sysc", "syscon";
> +      reg = <0x0 0x100>;
> +      #clock-cells = <1>;
> +      #reset-cells = <1>;
> +    };

Best regards,
Krzysztof