diff mbox series

[v2,1/2] dt-bindings: gpio: brcm: Add bindings for xgs-iproc

Message ID 20191017031051.20366-2-chris.packham@alliedtelesis.co.nz
State New
Headers show
Series gpio: brcm: XGS iProc GPIO driver | expand

Commit Message

Chris Packham Oct. 17, 2019, 3:10 a.m. UTC 
This GPIO controller is present on a number of Broadcom switch ASICs
with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
blocks but different enough to require a separate driver.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v2:
    - Document as DT schema
    - Include ngpios, #gpio-cells and gpio-controller properties

 .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml

Comments

Bartosz Golaszewski Oct. 17, 2019, 3:17 p.m. UTC | #1 
czw., 17 paź 2019 o 05:11 Chris Packham
<chris.packham@alliedtelesis.co.nz> napisał(a):
>
> This GPIO controller is present on a number of Broadcom switch ASICs
> with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
> blocks but different enough to require a separate driver.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>     Changes in v2:
>     - Document as DT schema
>     - Include ngpios, #gpio-cells and gpio-controller properties
>
>  .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
>
> diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> new file mode 100644
> index 000000000000..71998551209e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom XGS iProc GPIO controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description: |
> +  This controller is the Chip Common A GPIO present on a number of Broadcom
> +  switch ASICs with integrated SoCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,iproc-gpio-cca

I believe this should be:

    const: brcm,iproc-gpio-cca

Bart

> +
> +  reg:
> +    minItems: 2
> +    maxItems: 2
> +    description:
> +      The first region defines the base I/O address containing
> +      the GPIO controller registers. The second region defines
> +      the I/O address containing the Chip Common A interrupt
> +      registers.
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +      const: 2
> +
> +  ngpios:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 32
> +
> +  interrupt-controller:
> +    type: boolean
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +allOf:
> + - if:
> +     properties:
> +       interrupt-controller:
> +         contains:
> +           const: true
> +   then:
> +     required:
> +       - interrupts
> +       - '#interrupt-cells'
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    gpio@18000060 {
> +        compatible = "brcm,iproc-gpio-cca";
> +        #gpio-cells = <2>;
> +        reg = <0x18000060 0x50>,
> +              <0x18000000 0x50>;
> +        ngpios = <12>;
> +        gpio-controller;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +
> +...
> --
> 2.23.0
>
Rob Herring Oct. 17, 2019, 7:24 p.m. UTC | #2 
On Thu, Oct 17, 2019 at 04:10:50PM +1300, Chris Packham wrote:
> This GPIO controller is present on a number of Broadcom switch ASICs
> with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
> blocks but different enough to require a separate driver.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     Changes in v2:
>     - Document as DT schema
>     - Include ngpios, #gpio-cells and gpio-controller properties
> 
>  .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> new file mode 100644
> index 000000000000..71998551209e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom XGS iProc GPIO controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description: |
> +  This controller is the Chip Common A GPIO present on a number of Broadcom
> +  switch ASICs with integrated SoCs.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,iproc-gpio-cca

enum vs. const usage depends on whether you think you'll add more 
compatibles.

> +
> +  reg:
> +    minItems: 2
> +    maxItems: 2
> +    description:
> +      The first region defines the base I/O address containing
> +      the GPIO controller registers. The second region defines
> +      the I/O address containing the Chip Common A interrupt
> +      registers.

items:
  - description: the I/O address containing the GPIO controller 
      registers
  - description: the I/O address containing the Chip Common A interrupt 
      registers

And minItems/maxItems can be implicit.

> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +      const: 2
> +
> +  ngpios:
> +    $ref: /schemas/types.yaml#/definitions/uint32

Common property, doesn't need a type definition. Also, it would have to 
be under an 'allOf' to actually work.

> +    minimum: 0
> +    maximum: 32
> +
> +  interrupt-controller:
> +    type: boolean

Just 'interrupt-controller: true'

> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +allOf:
> + - if:
> +     properties:
> +       interrupt-controller:
> +         contains:
> +           const: true
> +   then:
> +     required:
> +       - interrupts
> +       - '#interrupt-cells'

This is mostly handled in the core schema already and 'dependencies' 
works better for this anyways. All you need here is:

dependencies:
  interrupt-controller: [ interrupts ]

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    gpio@18000060 {
> +        compatible = "brcm,iproc-gpio-cca";
> +        #gpio-cells = <2>;
> +        reg = <0x18000060 0x50>,
> +              <0x18000000 0x50>;
> +        ngpios = <12>;
> +        gpio-controller;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +
> +
> +...
> -- 
> 2.23.0
>
Bartosz Golaszewski Oct. 18, 2019, 5:59 a.m. UTC | #3 
czw., 17 paź 2019 o 21:24 Rob Herring <robh@kernel.org> napisał(a):
>
> On Thu, Oct 17, 2019 at 04:10:50PM +1300, Chris Packham wrote:
> > This GPIO controller is present on a number of Broadcom switch ASICs
> > with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
> > blocks but different enough to require a separate driver.
> >
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> >
> > Notes:
> >     Changes in v2:
> >     - Document as DT schema
> >     - Include ngpios, #gpio-cells and gpio-controller properties
> >
> >  .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
> >  1 file changed, 83 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > new file mode 100644
> > index 000000000000..71998551209e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > @@ -0,0 +1,83 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Broadcom XGS iProc GPIO controller
> > +
> > +maintainers:
> > +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> > +
> > +description: |
> > +  This controller is the Chip Common A GPIO present on a number of Broadcom
> > +  switch ASICs with integrated SoCs.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - brcm,iproc-gpio-cca
>
> enum vs. const usage depends on whether you think you'll add more
> compatibles.
>

What if you don't know yet? For instance we use a const compatible and
then a new chip is released for which we can reuse the driver? Is this
something that is expected to remain stable in the binding document?
The question is unrelated to this patch, I'm just unsure about my own
approach to writing yaml bindings.

Bart

> > +
> > +  reg:
> > +    minItems: 2
> > +    maxItems: 2
> > +    description:
> > +      The first region defines the base I/O address containing
> > +      the GPIO controller registers. The second region defines
> > +      the I/O address containing the Chip Common A interrupt
> > +      registers.
>
> items:
>   - description: the I/O address containing the GPIO controller
>       registers
>   - description: the I/O address containing the Chip Common A interrupt
>       registers
>
> And minItems/maxItems can be implicit.
>
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +      const: 2
> > +
> > +  ngpios:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
>
> Common property, doesn't need a type definition. Also, it would have to
> be under an 'allOf' to actually work.
>
> > +    minimum: 0
> > +    maximum: 32
> > +
> > +  interrupt-controller:
> > +    type: boolean
>
> Just 'interrupt-controller: true'
>
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#gpio-cells"
> > +  - gpio-controller
> > +
> > +allOf:
> > + - if:
> > +     properties:
> > +       interrupt-controller:
> > +         contains:
> > +           const: true
> > +   then:
> > +     required:
> > +       - interrupts
> > +       - '#interrupt-cells'
>
> This is mostly handled in the core schema already and 'dependencies'
> works better for this anyways. All you need here is:
>
> dependencies:
>   interrupt-controller: [ interrupts ]
>
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    gpio@18000060 {
> > +        compatible = "brcm,iproc-gpio-cca";
> > +        #gpio-cells = <2>;
> > +        reg = <0x18000060 0x50>,
> > +              <0x18000000 0x50>;
> > +        ngpios = <12>;
> > +        gpio-controller;
> > +        interrupt-controller;
> > +        #interrupt-cells = <2>;
> > +        interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> > +    };
> > +
> > +
> > +...
> > --
> > 2.23.0
> >
Rob Herring Oct. 18, 2019, 7:07 p.m. UTC | #4 
On Fri, Oct 18, 2019 at 1:00 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> czw., 17 paź 2019 o 21:24 Rob Herring <robh@kernel.org> napisał(a):
> >
> > On Thu, Oct 17, 2019 at 04:10:50PM +1300, Chris Packham wrote:
> > > This GPIO controller is present on a number of Broadcom switch ASICs
> > > with integrated SoCs. It is similar to the nsp-gpio and iproc-gpio
> > > blocks but different enough to require a separate driver.
> > >
> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > ---
> > >
> > > Notes:
> > >     Changes in v2:
> > >     - Document as DT schema
> > >     - Include ngpios, #gpio-cells and gpio-controller properties
> > >
> > >  .../bindings/gpio/brcm,xgs-iproc.yaml         | 83 +++++++++++++++++++
> > >  1 file changed, 83 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > > new file mode 100644
> > > index 000000000000..71998551209e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
> > > @@ -0,0 +1,83 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Broadcom XGS iProc GPIO controller
> > > +
> > > +maintainers:
> > > +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > +
> > > +description: |
> > > +  This controller is the Chip Common A GPIO present on a number of Broadcom
> > > +  switch ASICs with integrated SoCs.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - brcm,iproc-gpio-cca
> >
> > enum vs. const usage depends on whether you think you'll add more
> > compatibles.
> >
>
> What if you don't know yet? For instance we use a const compatible and
> then a new chip is released for which we can reuse the driver?

Then you just change it to an enum (or oneOf if the new compatible has
a fallback to the old one). Not really a big deal.

> Is this
> something that is expected to remain stable in the binding document?

No, only in the sense we want to minimize changes.

> The question is unrelated to this patch, I'm just unsure about my own
> approach to writing yaml bindings.

We could perhaps just say single entries should always be 'const'
because then we could write a meta-schema enforcing that:

properties:
  enum:
    minItems: 2

I don't think we should be that strict though unless it becomes a
frequent review topic. So either way is fine, it's up to your
judgement, and let's stop talking about it before I change my mind. :)

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
new file mode 100644
index 000000000000..71998551209e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/brcm,xgs-iproc.yaml
@@ -0,0 +1,83 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/brcm,xgs-iproc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom XGS iProc GPIO controller
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+description: |
+  This controller is the Chip Common A GPIO present on a number of Broadcom
+  switch ASICs with integrated SoCs.
+
+properties:
+  compatible:
+    enum:
+      - brcm,iproc-gpio-cca
+
+  reg:
+    minItems: 2
+    maxItems: 2
+    description:
+      The first region defines the base I/O address containing
+      the GPIO controller registers. The second region defines
+      the I/O address containing the Chip Common A interrupt
+      registers.
+
+  gpio-controller: true
+
+  '#gpio-cells':
+      const: 2
+
+  ngpios:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 32
+
+  interrupt-controller:
+    type: boolean
+
+  '#interrupt-cells':
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - gpio-controller
+
+allOf:
+ - if:
+     properties:
+       interrupt-controller:
+         contains:
+           const: true
+   then:
+     required:
+       - interrupts
+       - '#interrupt-cells'
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    gpio@18000060 {
+        compatible = "brcm,iproc-gpio-cca";
+        #gpio-cells = <2>;
+        reg = <0x18000060 0x50>,
+              <0x18000000 0x50>;
+        ngpios = <12>;
+        gpio-controller;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+    };
+
+
+...