diff mbox series

[v2,1/2] dt-bindings: iio: accel: Add binding documentation for ADXL313

Message ID ad64c93df8c43c66dcb64fe8ec0c0f6b91b3c697.1628143857.git.lucas.p.stankus@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio: accel: Add support for ADXL313 accelerometer | expand

Commit Message

Lucas Stankus Aug. 5, 2021, 6:29 a.m. UTC 
Add device tree binding documentation for ADXL313 3-axis accelerometer.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 .../bindings/iio/accel/adi,adxl313.yaml       | 90 +++++++++++++++++++
 1 file changed, 90 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml

Comments

Rob Herring (Arm) Aug. 6, 2021, 5:07 p.m. UTC | #1 
On Thu, 05 Aug 2021 03:29:37 -0300, Lucas Stankus wrote:
> Add device tree binding documentation for ADXL313 3-axis accelerometer.
> 
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
>  .../bindings/iio/accel/adi,adxl313.yaml       | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml: properties:interrupt-names:items: {'enum': ['INT1', 'INT2']} is not of type 'array'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml: ignoring, error in schema: properties: interrupt-names: items
warning: no schema found in file: ./Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
Documentation/devicetree/bindings/iio/accel/adi,adxl313.example.dt.yaml:0:0: /example-0/i2c0/accelerometer@53: failed to match any schema with compatible: ['adi,adxl313']
Documentation/devicetree/bindings/iio/accel/adi,adxl313.example.dt.yaml:0:0: /example-1/spi/accelerometer@0: failed to match any schema with compatible: ['adi,adxl313']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1513753

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring (Arm) Aug. 6, 2021, 6:10 p.m. UTC | #2 
On Thu, Aug 05, 2021 at 03:29:37AM -0300, Lucas Stankus wrote:
> Add device tree binding documentation for ADXL313 3-axis accelerometer.
> 
> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
>  .../bindings/iio/accel/adi,adxl313.yaml       | 90 +++++++++++++++++++
>  1 file changed, 90 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> new file mode 100644
> index 000000000000..fea03b6790f3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> @@ -0,0 +1,90 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/adi,adxl313.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADXL313 3-Axis Digital Accelerometer
> +
> +maintainers:
> +  - Lucas Stankus <lucas.p.stankus@gmail.com>
> +
> +description: |
> +  Analog Devices ADXL313 3-Axis Digital Accelerometer that supports
> +  both I2C & SPI interfaces.
> +    https://www.analog.com/en/products/adxl313.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adxl313
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-3wire: true
> +
> +  spi-cpha: true
> +
> +  spi-cpol: true

These 3 generally shouldn't be needed, but can be set from the driver. 
If they are valid, is any combination of them really valid?

> +
> +  spi-max-frequency: true
> +
> +  vs-supply:
> +    description: Regulator that supplies power to the accelerometer
> +
> +  vdd-supply:
> +    description: Regulator that supplies the digital interface supply voltage
> +
> +  interrupts:
> +    maxItems: 2

This means there must be 2 entries. If 1 is valid, you need 'minItems'.

> +
> +  interrupt-names:
> +    maxItems: 2

You need 'minItems' too to fix the error.

> +    items:
> +      enum:
> +        - INT1
> +        - INT2
> +
> +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>;
> +
> +        /* Example for a I2C device node */
> +        accelerometer@53 {
> +            compatible = "adi,adxl313";
> +            reg = <0x53>;
> +            interrupt-parent = <&gpio0>;
> +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
> +        };
> +    };
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        /* Example for a SPI device node */
> +        accelerometer@0 {
> +            compatible = "adi,adxl313";
> +            reg = <0>;
> +            spi-max-frequency = <5000000>;
> +            spi-cpol;
> +            spi-cpha;
> +            interrupt-parent = <&gpio0>;
> +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
> +        };
> +    };
> -- 
> 2.32.0
> 
>
Lucas Stankus Aug. 7, 2021, 12:33 a.m. UTC | #3 
On Fri, Aug 6, 2021 at 3:10 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Aug 05, 2021 at 03:29:37AM -0300, Lucas Stankus wrote:
> > Add device tree binding documentation for ADXL313 3-axis accelerometer.
> >
> > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > ---
> >  .../bindings/iio/accel/adi,adxl313.yaml       | 90 +++++++++++++++++++
> >  1 file changed, 90 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> > new file mode 100644
> > index 000000000000..fea03b6790f3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> > @@ -0,0 +1,90 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/accel/adi,adxl313.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADXL313 3-Axis Digital Accelerometer
> > +
> > +maintainers:
> > +  - Lucas Stankus <lucas.p.stankus@gmail.com>
> > +
> > +description: |
> > +  Analog Devices ADXL313 3-Axis Digital Accelerometer that supports
> > +  both I2C & SPI interfaces.
> > +    https://www.analog.com/en/products/adxl313.html
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adxl313
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-3wire: true
> > +
> > +  spi-cpha: true
> > +
> > +  spi-cpol: true
>
> These 3 generally shouldn't be needed, but can be set from the driver.
> If they are valid, is any combination of them really valid?
>

Only the 3wire is optional, both cpha and cpol are required for proper
spi connection.

> > +
> > +  spi-max-frequency: true
> > +
> > +  vs-supply:
> > +    description: Regulator that supplies power to the accelerometer
> > +
> > +  vdd-supply:
> > +    description: Regulator that supplies the digital interface supply voltage
> > +
> > +  interrupts:
> > +    maxItems: 2
>
> This means there must be 2 entries. If 1 is valid, you need 'minItems'.
>

I'll add 'minItems' for the v3 then, thanks!

> > +
> > +  interrupt-names:
> > +    maxItems: 2
>
> You need 'minItems' too to fix the error.
>

Thank you again and sorry for not catching that error before submitting.

> > +    items:
> > +      enum:
> > +        - INT1
> > +        - INT2
> > +
> > +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>;
> > +
> > +        /* Example for a I2C device node */
> > +        accelerometer@53 {
> > +            compatible = "adi,adxl313";
> > +            reg = <0x53>;
> > +            interrupt-parent = <&gpio0>;
> > +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "INT1";
> > +        };
> > +    };
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        /* Example for a SPI device node */
> > +        accelerometer@0 {
> > +            compatible = "adi,adxl313";
> > +            reg = <0>;
> > +            spi-max-frequency = <5000000>;
> > +            spi-cpol;
> > +            spi-cpha;
> > +            interrupt-parent = <&gpio0>;
> > +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "INT1";
> > +        };
> > +    };
> > --
> > 2.32.0
> >
> >
Jonathan Cameron Aug. 8, 2021, 2:45 p.m. UTC | #4 
On Fri, 6 Aug 2021 21:33:44 -0300
Lucas Stankus <lucas.p.stankus@gmail.com> wrote:

> On Fri, Aug 6, 2021 at 3:10 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Aug 05, 2021 at 03:29:37AM -0300, Lucas Stankus wrote:  
> > > Add device tree binding documentation for ADXL313 3-axis accelerometer.
> > >
> > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> > > ---
> > >  .../bindings/iio/accel/adi,adxl313.yaml       | 90 +++++++++++++++++++
> > >  1 file changed, 90 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> > > new file mode 100644
> > > index 000000000000..fea03b6790f3
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
> > > @@ -0,0 +1,90 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/accel/adi,adxl313.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Analog Devices ADXL313 3-Axis Digital Accelerometer
> > > +
> > > +maintainers:
> > > +  - Lucas Stankus <lucas.p.stankus@gmail.com>
> > > +
> > > +description: |
> > > +  Analog Devices ADXL313 3-Axis Digital Accelerometer that supports
> > > +  both I2C & SPI interfaces.
> > > +    https://www.analog.com/en/products/adxl313.html
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,adxl313
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  spi-3wire: true
> > > +
> > > +  spi-cpha: true
> > > +
> > > +  spi-cpol: true  
> >
> > These 3 generally shouldn't be needed, but can be set from the driver.
> > If they are valid, is any combination of them really valid?
> >  
> 
> Only the 3wire is optional, both cpha and cpol are required for proper
> spi connection.

We've been round this one a few time, and last time we discussed the
cases where you'd need these in DT (because of inverters on the bus)
https://lore.kernel.org/linux-iio/20191204111231.GO1998@sirena.org.uk/
conclusion was, that we don't want to put the burden on the dt files
for those odd cases.  The equivalent for interrupt lines is interestingly
different because in those cases the two-cell version includes the
type of interrupt, so it makes little sense to push that down into the
drivers as well.

Mind you I'm not 100% sure how we would retrofit a binding if necessary
for the inverted cases.  Hope we don't hit one here :)

As you note, 3wire is needed in the binding because it's optional.

Jonathan


> 
> > > +
> > > +  spi-max-frequency: true
> > > +
> > > +  vs-supply:
> > > +    description: Regulator that supplies power to the accelerometer
> > > +
> > > +  vdd-supply:
> > > +    description: Regulator that supplies the digital interface supply voltage
> > > +
> > > +  interrupts:
> > > +    maxItems: 2  
> >
> > This means there must be 2 entries. If 1 is valid, you need 'minItems'.
> >  
> 
> I'll add 'minItems' for the v3 then, thanks!
> 
> > > +
> > > +  interrupt-names:
> > > +    maxItems: 2  
> >
> > You need 'minItems' too to fix the error.
> >  
> 
> Thank you again and sorry for not catching that error before submitting.
> 
> > > +    items:
> > > +      enum:
> > > +        - INT1
> > > +        - INT2
> > > +
> > > +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>;
> > > +
> > > +        /* Example for a I2C device node */
> > > +        accelerometer@53 {
> > > +            compatible = "adi,adxl313";
> > > +            reg = <0x53>;
> > > +            interrupt-parent = <&gpio0>;
> > > +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > > +            interrupt-names = "INT1";
> > > +        };
> > > +    };
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +    spi {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        /* Example for a SPI device node */
> > > +        accelerometer@0 {
> > > +            compatible = "adi,adxl313";
> > > +            reg = <0>;
> > > +            spi-max-frequency = <5000000>;
> > > +            spi-cpol;
> > > +            spi-cpha;
> > > +            interrupt-parent = <&gpio0>;
> > > +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > > +            interrupt-names = "INT1";
> > > +        };
> > > +    };
> > > --
> > > 2.32.0
> > >
> > >
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
new file mode 100644
index 000000000000..fea03b6790f3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl313.yaml
@@ -0,0 +1,90 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/accel/adi,adxl313.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADXL313 3-Axis Digital Accelerometer
+
+maintainers:
+  - Lucas Stankus <lucas.p.stankus@gmail.com>
+
+description: |
+  Analog Devices ADXL313 3-Axis Digital Accelerometer that supports
+  both I2C & SPI interfaces.
+    https://www.analog.com/en/products/adxl313.html
+
+properties:
+  compatible:
+    enum:
+      - adi,adxl313
+
+  reg:
+    maxItems: 1
+
+  spi-3wire: true
+
+  spi-cpha: true
+
+  spi-cpol: true
+
+  spi-max-frequency: true
+
+  vs-supply:
+    description: Regulator that supplies power to the accelerometer
+
+  vdd-supply:
+    description: Regulator that supplies the digital interface supply voltage
+
+  interrupts:
+    maxItems: 2
+
+  interrupt-names:
+    maxItems: 2
+    items:
+      enum:
+        - INT1
+        - INT2
+
+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>;
+
+        /* Example for a I2C device node */
+        accelerometer@53 {
+            compatible = "adi,adxl313";
+            reg = <0x53>;
+            interrupt-parent = <&gpio0>;
+            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT1";
+        };
+    };
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        /* Example for a SPI device node */
+        accelerometer@0 {
+            compatible = "adi,adxl313";
+            reg = <0>;
+            spi-max-frequency = <5000000>;
+            spi-cpol;
+            spi-cpha;
+            interrupt-parent = <&gpio0>;
+            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT1";
+        };
+    };