diff mbox series

[2/3] dt-bindings: iio: gyro: adxrs290: Add required interrupts property

Message ID 20200825124711.11455-3-nish.malpani25@gmail.com
State New
Headers show
Series iio: gyro: adxrs290: Add triggered buffer & debugfs support | expand

Commit Message

Nishant Malpani Aug. 25, 2020, 12:47 p.m. UTC
Append 'interrupts' as a required property and provide a suitable example
for using a GPIO interrupt line.

Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
---
 .../devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml   | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Jonathan Cameron Aug. 29, 2020, 4:48 p.m. UTC | #1
On Tue, 25 Aug 2020 18:17:10 +0530
Nishant Malpani <nish.malpani25@gmail.com> wrote:

> Append 'interrupts' as a required property and provide a suitable example
> for using a GPIO interrupt line.
> 
> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
Hi Nishant,

I don't understand why the interrupt is 'required'.  Also note you should
never be adding required properties to an existing binding.  It's possible
someone already used the binding as it stands and shipped a board with
it burnt in a firmware. (bit unlikely but you never know!)

Jonathan

> ---
>  .../devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml   | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> index 61adb2c2454b..cae593dd1ba7 100644
> --- a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> +++ b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> @@ -28,17 +28,23 @@ properties:
>  
>    spi-cpha: true
>  
> +  interrupts:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
>    - spi-max-frequency
>    - spi-cpol
>    - spi-cpha
> +  - interrupts

Why?  Device works fine without one being supplied. 
It's not uncommon on embedded boards to not wire up interrupts
due to a lack of pins and just rely on polling.

>  
>  additionalProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
>      spi {
>          #address-cells = <1>;
>          #size-cells = <0>;
> @@ -48,6 +54,8 @@ examples:
>                     spi-max-frequency = <5000000>;
>                     spi-cpol;
>                     spi-cpha;
> +                   interrupt-parent = <&gpio>;
> +                   interrupts = <25 IRQ_TYPE_EDGE_RISING>;
>          };
>      };
>  ...
Nishant Malpani Sept. 3, 2020, 12:46 p.m. UTC | #2
Hello,

On Sat, Aug 29, 2020 at 10:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 25 Aug 2020 18:17:10 +0530
> Nishant Malpani <nish.malpani25@gmail.com> wrote:
>
> > Append 'interrupts' as a required property and provide a suitable example
> > for using a GPIO interrupt line.
> >
> > Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
> Hi Nishant,
>
> I don't understand why the interrupt is 'required'.  Also note you should
> never be adding required properties to an existing binding.  It's possible
> someone already used the binding as it stands and shipped a board with
> it burnt in a firmware. (bit unlikely but you never know!)
>
> Jonathan
>

You're right; I hadn't thought of it that way. Will remove the
'interrupts' property from the 'required' ist in v2.

> > ---
> >  .../devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml   | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> > index 61adb2c2454b..cae593dd1ba7 100644
> > --- a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> > +++ b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
> > @@ -28,17 +28,23 @@ properties:
> >
> >    spi-cpha: true
> >
> > +  interrupts:
> > +    maxItems: 1
> > +
> >  required:
> >    - compatible
> >    - reg
> >    - spi-max-frequency
> >    - spi-cpol
> >    - spi-cpha
> > +  - interrupts
>
> Why?  Device works fine without one being supplied.
> It's not uncommon on embedded boards to not wire up interrupts
> due to a lack of pins and just rely on polling.
>

I was under the impression that the triggered-buffer way of capturing
data would more reasonable to the consumers of a gyroscope. But what
you point out makes total sense. Thanks for pointing out. Will fix it
in v2.

With regards,
Nishant Malpani

> >
> >  additionalProperties: false
> >
> >  examples:
> >    - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> >      spi {
> >          #address-cells = <1>;
> >          #size-cells = <0>;
> > @@ -48,6 +54,8 @@ examples:
> >                     spi-max-frequency = <5000000>;
> >                     spi-cpol;
> >                     spi-cpha;
> > +                   interrupt-parent = <&gpio>;
> > +                   interrupts = <25 IRQ_TYPE_EDGE_RISING>;
> >          };
> >      };
> >  ...
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
index 61adb2c2454b..cae593dd1ba7 100644
--- a/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
+++ b/Documentation/devicetree/bindings/iio/gyroscope/adi,adxrs290.yaml
@@ -28,17 +28,23 @@  properties:
 
   spi-cpha: true
 
+  interrupts:
+    maxItems: 1
+
 required:
   - compatible
   - reg
   - spi-max-frequency
   - spi-cpol
   - spi-cpha
+  - interrupts
 
 additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
     spi {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -48,6 +54,8 @@  examples:
                    spi-max-frequency = <5000000>;
                    spi-cpol;
                    spi-cpha;
+                   interrupt-parent = <&gpio>;
+                   interrupts = <25 IRQ_TYPE_EDGE_RISING>;
         };
     };
 ...