| Message ID | 70f0522a9394e9da2f31871442d47f6ad0ff41aa.1626948070.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/4] arm64: dts: ipq6018: correct TCSR block area | expand |
On Thu, Jul 22, 2021 at 01:01:09PM +0300, Baruch Siach wrote: > DT binding for the PWM block in Qualcomm IPQ6018 SoC. > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > v6: > > Device node is child of TCSR; remove phandle (Rob Herring) > > Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König) > > v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn > Andersson, Kathiravan T) > > v4: Update the binding example node as well (Rob Herring's bot) > > v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring) > > v2: Make #pwm-cells const (Rob Herring) > --- > .../devicetree/bindings/pwm/ipq-pwm.yaml | 69 +++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > > diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > new file mode 100644 > index 000000000000..ee2bb03a1223 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > @@ -0,0 +1,69 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm IPQ6018 PWM controller > + > +maintainers: > + - Baruch Siach <baruch@tkos.co.il> > + > +properties: > + "#pwm-cells": > + const: 2 > + > + compatible: > + const: qcom,ipq6018-pwm > + > + offset: > + description: | > + Offset of PWM register in the TCSR block. > + maxItems: 1 Use 'reg' here instead. > + > + clocks: > + maxItems: 1 > + > + clock-names: > + const: core > + > + assigned-clocks: > + maxItems: 1 > + > + assigned-clock-rates: > + maxItems: 1 > + > +required: > + - "#pwm-cells" > + - compatible > + - clocks > + - clock-names > + - assigned-clocks > + - assigned-clock-rates > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/qcom,gcc-ipq6018.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + tcsr: syscon@1937000 { > + compatible = "syscon", "simple-mfd"; This should really have a specific compatible string, but we don't warn for that (yet). > + reg = <0x0 0x01937000 0x0 0x21000>; > + > + pwm: pwm { > + #pwm-cells = <2>; > + compatible = "qcom,ipq6018-pwm"; > + offset = <0xa010>; > + clocks = <&gcc GCC_ADSS_PWM_CLK>; > + clock-names = "core"; > + assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>; > + assigned-clock-rates = <100000000>; > + status = "disabled"; Drop 'status' > + }; > + }; > + }; > -- > 2.30.2 > >
On Thu 22 Jul 05:01 CDT 2021, Baruch Siach wrote: > DT binding for the PWM block in Qualcomm IPQ6018 SoC. > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > v6: > > Device node is child of TCSR; remove phandle (Rob Herring) > > Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König) > > v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn > Andersson, Kathiravan T) > > v4: Update the binding example node as well (Rob Herring's bot) > > v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring) > > v2: Make #pwm-cells const (Rob Herring) > --- > .../devicetree/bindings/pwm/ipq-pwm.yaml | 69 +++++++++++++++++++ > 1 file changed, 69 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > > diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > new file mode 100644 > index 000000000000..ee2bb03a1223 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > @@ -0,0 +1,69 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm IPQ6018 PWM controller > + > +maintainers: > + - Baruch Siach <baruch@tkos.co.il> > + > +properties: > + "#pwm-cells": > + const: 2 > + > + compatible: > + const: qcom,ipq6018-pwm > + > + offset: > + description: | '|' maintains the formatting of the text, you don't need that. > + Offset of PWM register in the TCSR block. > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + const: core With a single clock, it's nice to skip the -names. > + > + assigned-clocks: > + maxItems: 1 > + > + assigned-clock-rates: > + maxItems: 1 These (assigned-*) are generic properties that may be used on a lot of nodes, should they really be part of the individual binding, Rob? > + > +required: > + - "#pwm-cells" > + - compatible > + - clocks > + - clock-names > + - assigned-clocks > + - assigned-clock-rates > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/qcom,gcc-ipq6018.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + Skip soc and *-cells... > + tcsr: syscon@1937000 { > + compatible = "syscon", "simple-mfd"; > + reg = <0x0 0x01937000 0x0 0x21000>; > + ..and just make this "reg = <0x01937000 0x21000>", in the example. Then as we put this in the particular dts we adjust for whatever *-cells that has defined for the parent bus. > + pwm: pwm { > + #pwm-cells = <2>; I know it's important that this is a pwm thing, but I would prefer to see the node start with compatible, offset/reg, clocks. And then end with whatever is exposed (i.e. #pwm-cells) Regards, Bjorn > + compatible = "qcom,ipq6018-pwm"; > + offset = <0xa010>; > + clocks = <&gcc GCC_ADSS_PWM_CLK>; > + clock-names = "core"; > + assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>; > + assigned-clock-rates = <100000000>; > + status = "disabled"; > + }; > + }; > + }; > -- > 2.30.2 >
Hi Bjorn, On Sun, Jul 25 2021, Bjorn Andersson wrote: > On Thu 22 Jul 05:01 CDT 2021, Baruch Siach wrote: >> + clocks: >> + maxItems: 1 >> + >> + clock-names: >> + const: core > > With a single clock, it's nice to skip the -names. I find it nicer and better for forward compatibility with hardware variants the might introduce more clocks. Are there any downsides to -names? Thanks, baruch
On Sun 25 Jul 21:08 PDT 2021, Baruch Siach wrote: > Hi Bjorn, > > On Sun, Jul 25 2021, Bjorn Andersson wrote: > > On Thu 22 Jul 05:01 CDT 2021, Baruch Siach wrote: > >> + clocks: > >> + maxItems: 1 > >> + > >> + clock-names: > >> + const: core > > > > With a single clock, it's nice to skip the -names. > > I find it nicer and better for forward compatibility with hardware > variants the might introduce more clocks. > Do you foresee any need for forward compatibility? What other clocks would this binding have to refer to? That said, you'd achieve the same forward compatibility by just making sure that the current clock is the first on in the amended binding (which you have to do with or without -names). > Are there any downsides to -names? > Look at the number of places in a typical dts that we could have added clock-names, reg-names, interrupt-names, power-domain-names etc for a single cell. I do find it beneficial to keep things cleaner and sticking with the design of "single resource has no -names". Regards, Bjorn
On Sun, Jul 25, 2021 at 12:27 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Thu 22 Jul 05:01 CDT 2021, Baruch Siach wrote: > > > DT binding for the PWM block in Qualcomm IPQ6018 SoC. > > > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > --- > > v6: > > > > Device node is child of TCSR; remove phandle (Rob Herring) > > > > Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König) > > > > v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn > > Andersson, Kathiravan T) > > > > v4: Update the binding example node as well (Rob Herring's bot) > > > > v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring) > > > > v2: Make #pwm-cells const (Rob Herring) > > --- > > .../devicetree/bindings/pwm/ipq-pwm.yaml | 69 +++++++++++++++++++ > > 1 file changed, 69 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > > > > diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > > new file mode 100644 > > index 000000000000..ee2bb03a1223 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > > @@ -0,0 +1,69 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Qualcomm IPQ6018 PWM controller > > + > > +maintainers: > > + - Baruch Siach <baruch@tkos.co.il> > > + > > +properties: > > + "#pwm-cells": > > + const: 2 > > + > > + compatible: > > + const: qcom,ipq6018-pwm > > + > > + offset: > > + description: | > > '|' maintains the formatting of the text, you don't need that. > > > + Offset of PWM register in the TCSR block. > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + const: core > > With a single clock, it's nice to skip the -names. > > > + > > + assigned-clocks: > > + maxItems: 1 > > + > > + assigned-clock-rates: > > + maxItems: 1 > > These (assigned-*) are generic properties that may be used on a lot of > nodes, should they really be part of the individual binding, Rob? They are allowed on any node with 'clocks', so you don't need them. However, if you know there's 1 entry only, then I'd keep that. Or was 'maxItems: 1' just copied because I see that alot. Rob
diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml new file mode 100644 index 000000000000..ee2bb03a1223 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml @@ -0,0 +1,69 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm IPQ6018 PWM controller + +maintainers: + - Baruch Siach <baruch@tkos.co.il> + +properties: + "#pwm-cells": + const: 2 + + compatible: + const: qcom,ipq6018-pwm + + offset: + description: | + Offset of PWM register in the TCSR block. + maxItems: 1 + + clocks: + maxItems: 1 + + clock-names: + const: core + + assigned-clocks: + maxItems: 1 + + assigned-clock-rates: + maxItems: 1 + +required: + - "#pwm-cells" + - compatible + - clocks + - clock-names + - assigned-clocks + - assigned-clock-rates + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/qcom,gcc-ipq6018.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + tcsr: syscon@1937000 { + compatible = "syscon", "simple-mfd"; + reg = <0x0 0x01937000 0x0 0x21000>; + + pwm: pwm { + #pwm-cells = <2>; + compatible = "qcom,ipq6018-pwm"; + offset = <0xa010>; + clocks = <&gcc GCC_ADSS_PWM_CLK>; + clock-names = "core"; + assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>; + assigned-clock-rates = <100000000>; + status = "disabled"; + }; + }; + };
DT binding for the PWM block in Qualcomm IPQ6018 SoC. Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- v6: Device node is child of TCSR; remove phandle (Rob Herring) Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König) v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn Andersson, Kathiravan T) v4: Update the binding example node as well (Rob Herring's bot) v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring) v2: Make #pwm-cells const (Rob Herring) --- .../devicetree/bindings/pwm/ipq-pwm.yaml | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/ipq-pwm.yaml