diff mbox series

[v1,3/3] dt-bindings: iio: accel: add dt-binding schema for msa311 accel driver

Message ID 20220419154555.24191-4-ddrokosov@sberdevices.ru (mailing list archive)
State Changes Requested
Headers show
Series iio: accel: add MSA311 accelerometer driver | expand

Commit Message

Dmitry Rokosov April 19, 2022, 3:45 p.m. UTC 
Introduce devicetree binding json-schema for MSA311 tri-axial,
low-g accelerometer driver.

Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
---
 .../bindings/iio/accel/memsensing,msa311.yaml      | 64 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml

Comments

Jonathan Cameron April 20, 2022, 8:51 a.m. UTC | #1 
On Tue, 19 Apr 2022 15:45:58 +0000
Rokosov Dmitry Dmitrievich <DDRokosov@sberdevices.ru> wrote:

> Introduce devicetree binding json-schema for MSA311 tri-axial,
> low-g accelerometer driver.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
Hi Dmitry and welcome to IIO.

Sign off generally matches the From: of the email.
Makes it easier for scripts to check than having different name forms.

> ---
>  .../bindings/iio/accel/memsensing,msa311.yaml      | 64 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> new file mode 100644
> index 00000000..3e4660f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/iio/accel/memsensing,msa311.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: MEMSensing digital 3-Axis accelerometer
> +
> +maintainers:
> +  - Dmitry Rokosov <ddrokosov@sberdevices.ru>
> +
> +description: |
> +  MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> +  sensitivity consumer applications. It has dynamical user selectable full
> +  scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> +  with output data rates from 1Hz to 1000Hz.
> +  Datasheet can be found at following URL
> +  https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> +
> +properties:
> +  compatible:
> +    const: memsensing,msa311
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  reg:
> +    maxItems: 1
> +    description: I2C registers address
> +
> +  interrupts:
> +    maxItems: 1
> +    description: optional I2C int pin can be freely mapped to specific func
> +
> +  interrupt-names:
> +    const: irq

For a single IRQ giving it a name isn't that useful so I would drop this.


> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>

Why have this?

> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        msa311: msa311@62 {
Generic naming required.
           accelerometer@62 {

> +            compatible = "memsensing,msa311";
> +            reg = <0x62>;
> +            interrupt-parent = <&gpio_intc>;
> +            interrupts = <29 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "irq";
> +            status = "okay";

We don't often have status in the examples. 


Otherwise looks good to me.

Thanks,

Jonathan

> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c75be17..4227914 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12482,6 +12482,7 @@ MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
>  M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
>  L:	linux-iio@vger.kernel.org
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
>  F:	drivers/iio/accel/msa311.c
>  
>  MEN A21 WATCHDOG DRIVER
Dmitry Rokosov April 21, 2022, 1:55 p.m. UTC | #2 
Hello Jonathan,

Thank you for the quick feedback.
I completely agree with all of your comments. Please find my notes below.

On Wed, Apr 20, 2022 at 09:51:27AM +0100, Jonathan Cameron wrote:
> On Tue, 19 Apr 2022 15:45:58 +0000
> Rokosov Dmitry Dmitrievich <DDRokosov@sberdevices.ru> wrote:
> 
> > Introduce devicetree binding json-schema for MSA311 tri-axial,
> > low-g accelerometer driver.
> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> Hi Dmitry and welcome to IIO.
> 
> Sign off generally matches the From: of the email.
> Makes it easier for scripts to check than having different name forms.
> 

Yes, looks like this problem is located on the corporative Exchange server side.
I use DavMail local proxy to send emails, so server changes From: statement.
I have already discussed it with our IT guys, it should be fixed starting from current email.

> > ---
> >  .../bindings/iio/accel/memsensing,msa311.yaml      | 64 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 65 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> > new file mode 100644
> > index 00000000..3e4660f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> > @@ -0,0 +1,64 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/iio/accel/memsensing,msa311.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: MEMSensing digital 3-Axis accelerometer
> > +
> > +maintainers:
> > +  - Dmitry Rokosov <ddrokosov@sberdevices.ru>
> > +
> > +description: |
> > +  MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > +  sensitivity consumer applications. It has dynamical user selectable full
> > +  scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > +  with output data rates from 1Hz to 1000Hz.
> > +  Datasheet can be found at following URL
> > +  https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: memsensing,msa311
> > +
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: I2C registers address
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +    description: optional I2C int pin can be freely mapped to specific func
> > +
> > +  interrupt-names:
> > +    const: irq
> 
> For a single IRQ giving it a name isn't that useful so I would drop this.
> 
> 

Based on Documentation/devicetree/bindings/example-schema.yaml I thought
interrupt-names must contain irq name regardless of interrupt count,
because it helps to understand right property name.
I'll delete it for v2.

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> 
> Why have this?
> 

I agree, this header is needed for GPIO constant values, which is not used here.
I'll remove it for the v2 then.

> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        msa311: msa311@62 {
> Generic naming required.
>            accelerometer@62 {
> 

Okay, I will change it in the next version.

> > +            compatible = "memsensing,msa311";
> > +            reg = <0x62>;
> > +            interrupt-parent = <&gpio_intc>;
> > +            interrupts = <29 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "irq";
> > +            status = "okay";
> 
> We don't often have status in the examples. 
> 

Sure, status is useful for real dts declarations.
It's there because I copied this declaration from by tested real board dts :-)

> 
> Otherwise looks good to me.
> 
> Thanks,
> 
> Jonathan
> 
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c75be17..4227914 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12482,6 +12482,7 @@ MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
> >  M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
> >  L:	linux-iio@vger.kernel.org
> >  S:	Maintained
> > +F:	Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> >  F:	drivers/iio/accel/msa311.c
> >  
> >  MEN A21 WATCHDOG DRIVER
>
Rob Herring April 26, 2022, 8:31 p.m. UTC | #3 
On Tue, Apr 19, 2022 at 03:45:58PM +0000, Rokosov Dmitry Dmitrievich wrote:
> Introduce devicetree binding json-schema for MSA311 tri-axial,
> low-g accelerometer driver.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> ---
>  .../bindings/iio/accel/memsensing,msa311.yaml      | 64 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> new file mode 100644
> index 00000000..3e4660f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/iio/accel/memsensing,msa311.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: MEMSensing digital 3-Axis accelerometer
> +
> +maintainers:
> +  - Dmitry Rokosov <ddrokosov@sberdevices.ru>
> +
> +description: |
> +  MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> +  sensitivity consumer applications. It has dynamical user selectable full
> +  scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> +  with output data rates from 1Hz to 1000Hz.
> +  Datasheet can be found at following URL
> +  https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> +
> +properties:
> +  compatible:
> +    const: memsensing,msa311
> +

> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0

These apply to 'reg' in a child node, but you don't have child nodes so 
drop them.

> +
> +  reg:
> +    maxItems: 1
> +    description: I2C registers address
> +
> +  interrupts:
> +    maxItems: 1
> +    description: optional I2C int pin can be freely mapped to specific func
> +
> +  interrupt-names:
> +    const: irq
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c0 {

i2c {

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        msa311: msa311@62 {

accelerometer@62 {

> +            compatible = "memsensing,msa311";
> +            reg = <0x62>;
> +            interrupt-parent = <&gpio_intc>;
> +            interrupts = <29 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "irq";
> +            status = "okay";
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c75be17..4227914 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12482,6 +12482,7 @@ MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
>  M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
>  L:	linux-iio@vger.kernel.org
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
>  F:	drivers/iio/accel/msa311.c
>  
>  MEN A21 WATCHDOG DRIVER
> -- 
> 2.9.5
>
Dmitry Rokosov April 27, 2022, 8:58 a.m. UTC | #4 
Hi Rob,

Thank you for review! I agree with all things you mentioned. It will be
fixed in the next v2 patchset.

On Tue, Apr 26, 2022 at 03:31:05PM -0500, Rob Herring wrote:
> On Tue, Apr 19, 2022 at 03:45:58PM +0000, Rokosov Dmitry Dmitrievich wrote:
> > Introduce devicetree binding json-schema for MSA311 tri-axial,
> > low-g accelerometer driver.
> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru>
> > ---
> >  .../bindings/iio/accel/memsensing,msa311.yaml      | 64 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 65 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> > new file mode 100644
> > index 00000000..3e4660f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> > @@ -0,0 +1,64 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/iio/accel/memsensing,msa311.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: MEMSensing digital 3-Axis accelerometer
> > +
> > +maintainers:
> > +  - Dmitry Rokosov <ddrokosov@sberdevices.ru>
> > +
> > +description: |
> > +  MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
> > +  sensitivity consumer applications. It has dynamical user selectable full
> > +  scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
> > +  with output data rates from 1Hz to 1000Hz.
> > +  Datasheet can be found at following URL
> > +  https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: memsensing,msa311
> > +
> 
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> 
> These apply to 'reg' in a child node, but you don't have child nodes so 
> drop them.
> 

Ack

> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: I2C registers address
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +    description: optional I2C int pin can be freely mapped to specific func
> > +
> > +  interrupt-names:
> > +    const: irq
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c0 {
> 
> i2c {
> 

Ack

> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        msa311: msa311@62 {
> 
> accelerometer@62 {
> 

Ack

> > +            compatible = "memsensing,msa311";
> > +            reg = <0x62>;
> > +            interrupt-parent = <&gpio_intc>;
> > +            interrupts = <29 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "irq";
> > +            status = "okay";
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c75be17..4227914 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12482,6 +12482,7 @@ MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
> >  M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
> >  L:	linux-iio@vger.kernel.org
> >  S:	Maintained
> > +F:	Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
> >  F:	drivers/iio/accel/msa311.c
> >  
> >  MEN A21 WATCHDOG DRIVER
> > -- 
> > 2.9.5
> >
Dmitry Rokosov May 4, 2022, 6:36 p.m. UTC | #5 
Hello Rob,

I've missed one note below, please check if possible.

On Tue, Apr 26, 2022 at 03:31:05PM -0500, Rob Herring wrote:
> On Tue, Apr 19, 2022 at 03:45:58PM +0000, Rokosov Dmitry Dmitrievich wrote:
> > +  "#address-cells":
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> 
> These apply to 'reg' in a child node, but you don't have child nodes so 
> drop them.
> 

I'm afraid, I made a mistake in the previous reply. Here I applied
address-cells and size-cells to show which type accelerometer's reg
property has, like this one:

    i2c {
        #address-cells = <1>;
        #size-cells = <0>;
    
        accelerometer@62 {
            compatible = "memsensing,msa311";
            reg = <0x62>;

Above accelerometer@62 is child node for i2c bus node, so address and
size cells mean accelerometer reg format. Am I missing something?
Jonathan Cameron May 4, 2022, 8:48 p.m. UTC | #6 
On Wed, 4 May 2022 18:36:33 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:

> Hello Rob,
> 
> I've missed one note below, please check if possible.
> 
> On Tue, Apr 26, 2022 at 03:31:05PM -0500, Rob Herring wrote:
> > On Tue, Apr 19, 2022 at 03:45:58PM +0000, Rokosov Dmitry Dmitrievich wrote:  
> > > +  "#address-cells":
> > > +    const: 1
> > > +
> > > +  "#size-cells":
> > > +    const: 0  
> > 
> > These apply to 'reg' in a child node, but you don't have child nodes so 
> > drop them.
> >   
> 
> I'm afraid, I made a mistake in the previous reply. Here I applied
> address-cells and size-cells to show which type accelerometer's reg
> property has, like this one:
> 
>     i2c {
>         #address-cells = <1>;
>         #size-cells = <0>;
>     
>         accelerometer@62 {
>             compatible = "memsensing,msa311";
>             reg = <0x62>;
> 
> Above accelerometer@62 is child node for i2c bus node, so address and
> size cells mean accelerometer reg format. Am I missing something?

They are characteristics of the i2c master and hence
are listed in those binding docs, not the one for the accelerometer.

Thanks,

Jonathan
>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
new file mode 100644
index 00000000..3e4660f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
@@ -0,0 +1,64 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/iio/accel/memsensing,msa311.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: MEMSensing digital 3-Axis accelerometer
+
+maintainers:
+  - Dmitry Rokosov <ddrokosov@sberdevices.ru>
+
+description: |
+  MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
+  sensitivity consumer applications. It has dynamical user selectable full
+  scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
+  with output data rates from 1Hz to 1000Hz.
+  Datasheet can be found at following URL
+  https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
+
+properties:
+  compatible:
+    const: memsensing,msa311
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  reg:
+    maxItems: 1
+    description: I2C registers address
+
+  interrupts:
+    maxItems: 1
+    description: optional I2C int pin can be freely mapped to specific func
+
+  interrupt-names:
+    const: irq
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        msa311: msa311@62 {
+            compatible = "memsensing,msa311";
+            reg = <0x62>;
+            interrupt-parent = <&gpio_intc>;
+            interrupts = <29 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "irq";
+            status = "okay";
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index c75be17..4227914 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12482,6 +12482,7 @@  MEMSENSING MICROSYSTEMS MSA311 ACCELEROMETER DRIVER
 M:	Dmitry Rokosov <ddrokosov@sberdevices.ru>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/iio/accel/memsensing,msa311.yaml
 F:	drivers/iio/accel/msa311.c
 
 MEN A21 WATCHDOG DRIVER