On 18/04/2022 14:56, Cixi Geng wrote: > From: Cixi Geng <cixi.geng1@xxxxxxxxxx> > > Add a new bindings to describe ums512 clock compatible string. > > Signed-off-by: Cixi Geng <cixi.geng1@xxxxxxxxxx> > --- > .../bindings/clock/sprd,ums512-clk.yaml | 112 ++++++++++++++++++ > 1 file changed, 112 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml > > diff --git a/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml > new file mode 100644 > index 000000000000..89824d7c6be4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/sprd,ums512-clk.yaml > @@ -0,0 +1,112 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright 2022 Unisoc Inc. > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/clock/sprd,ums512-clk.yaml#"; > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"; > + > +title: UMS512 Clock Control Unit Device Tree Bindings Remove "Device Tree Bindings". You could do the same also in the subject, because you repeat the prefix ("dt-bindings: clk: sprd: Add ums512 clock controller"). > + > +maintainers: > + - Orson Zhai <orsonzhai@xxxxxxxxx> > + - Baolin Wang <baolin.wang7@xxxxxxxxx> > + - Chunyan Zhang <zhang.lyra@xxxxxxxxx> > + > +properties: > + "#clock-cells": > + const: 1 > + > + compatible: Put the compatible first, by convention and makes finding matching bindings easier. > + oneOf: > + - items: > + - const: sprd,ums512-glbregs > + - const: syscon > + - const: simple-mfd Why do you need simple-mfd for these? This looks like a regular syscon, so usually does not come with children. What is more, why this "usual syscon" is a separate clock controller in these bindings? > + - items: > + - enum: > + - sprd,ums512-apahb-gate > + - sprd,ums512-ap-clk > + - sprd,ums512-aonapb-clk > + - sprd,ums512-pmu-gate > + - sprd,ums512-g0-pll > + - sprd,ums512-g2-pll > + - sprd,ums512-g3-pll > + - sprd,ums512-gc-pll > + - sprd,ums512-aon-gate > + - sprd,ums512-audcpapb-gate > + - sprd,ums512-audcpahb-gate > + - sprd,ums512-gpu-clk > + - sprd,ums512-mm-clk > + - sprd,ums512-mm-gate-clk > + - sprd,ums512-apapb-gate > + > + clocks: > + minItems: 1 maxItems needed > + description: | > + The input parent clock(s) phandle for this clock, only list fixed > + clocks which are declared in devicetree. The description does not make sense. These are bindings for a clock controller, but you say here "for this clock", so what does "this" mean here? > + > + clock-names: > + minItems: 1 > + items: > + - const: ext-26m > + - const: ext-32k > + - const: ext-4m > + - const: rco-100m > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - '#clock-cells' Isn't reg also required? Always? Do you have examples where it is not required? How do you configure the clocks without "reg"? I see you copied a lot from sprd,sc9863a-clk.yaml but that file does not look correct. You have nodes with reg but without unit address ("rpll"). These nodes are modeled as children but they are not children - it's a workaround for exposing syscon, isn't it? The sc9863a looks like broken design, so please do not duplicate it here. > + > +if: > + properties: > + compatible: > + enum: > + - sprd,ums512-ap-clk > + - sprd,ums512-aonapb-clk > + - sprd,ums512-mm-clk > +then: > + required: > + - reg > + > +else: > + description: | > + Other UMS512 clock nodes should be the child of a syscon node in > + which compatible string should be: > + "sprd,ums512-glbregs", "syscon", "simple-mfd" > + > + The 'reg' property for the clock node is also required if there is a sub > + range of registers for the clocks. > + > +additionalProperties: true false > + > +examples: > + - | > + ap_clk: clock-controller@20200000 { > + compatible = "sprd,ums512-ap-clk"; > + reg = <0x20200000 0x1000>; > + clocks = <&ext_26m>; > + clock-names = "ext-26m"; > + #clock-cells = <1>; > + }; > + > + - | > + ap_apb_regs: syscon@71000000 { > + compatible = "sprd,ums512-glbregs", "syscon", "simple-mfd"; So here is the answer why you added MFD, but I still don't get why do you need it for one child? It is quite a dance here and in your drivers, instead of adding "syscon" to your clock controller. This also pollutes the actual bindings because you did not add properties required for clock controllers, because of describing here the syscon part. > + reg = <0x71000000 0x3000>; > + #address-cells = <1>; > + #size-cells = <1>; > + #clock-cells = <1>; > + ranges = <0 0x71000000 0x3000>; > + > + apahb_gate: clock-controller@0 { > + compatible = "sprd,ums512-apahb-gate"; > + reg = <0x0 0x2000>; > + #clock-cells = <1>; > + }; > + }; > + > +... Best regards, Krzysztof
