Messages in this thread | | | Date | Fri, 13 Jan 2023 12:17:36 +0100 | Subject | Re: [PATCH 1/2] dt-bindings: clock: Add binding for Loongson-1 clock driver | From | Krzysztof Kozlowski <> |
| |
On 13/01/2023 12:07, Keguang Zhang wrote: > Add devicetree binding document for the Loongson-1 clock driver.
Subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings.
Subject: Drop driver, not related to hardware.
> > Signed-off-by: Keguang Zhang <keguang.zhang@gmail.com> > --- > .../bindings/clock/loongson,ls1x-clk.yaml | 81 +++++++++++++++++++ > 1 file changed, 81 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > > diff --git a/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > new file mode 100644 > index 000000000000..4709c6757f1e > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/loongson,ls1x-clk.yaml > @@ -0,0 +1,81 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/loongson,ls1x-clk.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Loongson-1 Clock Controller
Wasn't this already sent? https://lore.kernel.org/all/20190130194731.GA25716@bogus/ Then this is a v4? Aren't you duplicating efforts (and reviewers efforts)?
> + > +maintainers: > + - Keguang Zhang <keguang.zhang@gmail.com> > + > +properties:
compatible is a first property.
> + "#clock-cells": > + const: 0 > + > + compatible: > + enum: > + - loongson,ls1b-clk-pll > + - loongson,ls1b-clk-cpu > + - loongson,ls1b-clk-ahb > + - loongson,ls1c-clk-pll > + - loongson,ls1c-clk-cpu > + - loongson,ls1c-clk-ahb
Are you registering single clocks? It looks like. No, make a proper clock controller.
> + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > +required: > + - "#clock-cells" > + - compatible > + - clocks > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + clocks {
No, not really related to the binding.
> + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + xtal: xtal {
Incorrect in this context. Missing unit address.
> + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <33000000>; > + }; > + > + pll: pll@1fe78030 {
Node names should be generic, so "clock-controller" https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "loongson,ls1b-clk-pll"; > + #clock-cells = <0>; > + clocks = <&xtal>; > + reg = <0x1fe78030 0x4>;
compatible is first property, then reg, then the rest.
> + }; > + > + cpu_clk: cpu_clk@1fe78034 {
No underscores in node names. Anyway this should be gone - make a proper clock controller.
Best regards, Krzysztof
| |