Messages in this thread | | | Date | Mon, 20 Mar 2023 17:36:35 +0100 | Subject | Re: [PATCH 01/10] dt: bindings: clock: add mtmips SoCs clock device tree binding documentation | From | Krzysztof Kozlowski <> |
| |
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
| |