diff mbox series

[v6,3/4] dt-bindings: pwm: add IPQ6018 binding

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

Commit Message

Baruch Siach July 22, 2021, 10:01 a.m. UTC
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

Comments

Rob Herring (Arm) July 23, 2021, 11:03 p.m. UTC | #1
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
> 
>
Bjorn Andersson July 25, 2021, 6:27 p.m. UTC | #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
>
Baruch Siach July 26, 2021, 4:08 a.m. UTC | #3
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
Bjorn Andersson July 26, 2021, 5:48 p.m. UTC | #4
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
Rob Herring July 26, 2021, 8:38 p.m. UTC | #5
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 mbox series

Patch

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";
+            };
+        };
+    };