Re: [PATCH v2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

Hi,

Could anyone with some experience in devicetree give us a hint on what
to do here?

I have built a binding doc using the same spi-cpha property.
https://github.com/analogdevicesinc/linux/commit/bb2945e489dfdf2faa0255dd2cf09ae4ee77d826

On 09/13, Ardelean, Alexandru wrote:
> On Thu, 2019-09-12 at 18:39 -0300, Rodrigo Carvalho wrote:
> > This patch add device tree binding documentation for ADIS16240.
> > 
> > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@gmail.com>
> > ---
> > V2:
> >   - Remove true constant for spi-cpha and spi-cpol
> >   - Add description field for spi-cpha and spi-cpol
> >   - Add maxItems field for spi-cpha and spi-cpol
> > 
> >  .../bindings/iio/accel/adi,adis16240.yaml     | 61 +++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > new file mode 100644
> > index 000000000000..4b1bd2419604
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> > +
> > +maintainers:
> > +  - Alexandru Ardelean <alexandru.ardelean@analog.com>
> > +
> > +description: |
> > +  ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> > +  SPI interface.
> > +    https://www.analog.com/en/products/adis16240.html
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adis16240
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-cpha:
> > +    description: |
> > +      See Documentation/devicetree/bindings/spi/spi-controller.yaml
> > +    maxItems: 1
> 
> Description for standard properties is not required.
> 
> For spi-cpha/cpol just "true" seems sufficient.
> 
> So
> 
>      spi-cpha: true
> 
>      spi-cpol: true
> 
I'm not Rob, but I think it is not necessary to explicitly say the
property is true. In this case, it should be enough if it is present. If
needed to know whether some property is "true" or not, one can use the
of_property_read_bool function. For the AD7292 driver on it was enough
to just add the property names. The spi-chpa did not need any further
care. Without the spi-cpha property, the AD7292 vendor ID came as 0x0C
(one bit shifted to the right).

Rodrigo is participating at FLUSP students group. It would be good if we
could test whether these properties are really needed. However, I don't
think we have the ADIS16240 part. Would anyone test it?

> > +
> > +  spi-cpol: |
> > +    description: |
> > +      See Documentation/devicetree/bindings/spi/spi-controller.yaml
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> 
> If spi-cpha & spi-cpol are true, they should typically be also required.
> Though, I think Rob would answer things better here.
> 
Some feedback about the need (or not) to link to the spi-controller doc
would also be appreciated.

> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spi0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        /* Example for a SPI device node */
> > +        accelerometer@0 {
> > +            compatible = "adi,adis16240";
> > +            reg = <0>;
> > +            spi-max-frequency = <2500000>;
> > +            spi-cpol;
> > +            spi-cpha;
> > +            interrupt-parent = <&gpio0>;
> > +            interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +        };
> > +    };

Thanks,

Marcelo