Re: [PATCH 1/2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Rodrigo Carvalho]

Hi,

Thanks for comments. I will send a v2.

Em dom, 8 de set de 2019 Ãs 07:52, Jonathan Cameron <jic23@xxxxxxxxxx> escreveu:
>
> On Mon, 2 Sep 2019 14:14:18 -0300
> Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote:
>
> > Hi Rodrigo,
> >
> > This dt doc looks overal fine IMHO.
> > I would just add some inline comments about the cpha and cpol
> > properties.
> >
> > On 09/01, Rodrigo Carvalho wrote:
> > > This patch add device tree binding documentation for ADIS16240.
> > >
> > > Signed-off-by: Rodrigo Ribeiro Carvalho <rodrigorsdc@xxxxxxxxx>
> > > ---
> > > I have doubt about what maintainer I may to put in that documentation. I
> > > put Alexandru as maintainer because he reviewed my last patch on this
> > > driver, so I think that he is a good candidate.
> > >  .../bindings/iio/accel/adi,adis16240.yaml     | 55 +++++++++++++++++++
> > >  1 file changed, 55 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..08019b51611c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > > @@ -0,0 +1,55 @@
> > > +# 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@xxxxxxxxxx>
> > > +
> > > +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: true
> > > +
> > > +  spi-cpol: true
> > Boolean properties don't require to be explicitly set. It would also be
> > nice to add a description pointing to the spi-bus documentation. Like
> > this:
> >
> >   spi-cpha:
> >     description: |
> >       See Documentation/devicetree/bindings/spi/spi-bus.txt
> >     maxItems: 1
> >
> >   spi-cpol:
> >     description: |
> >       See Documentation/devicetree/bindings/spi/spi-bus.txt
> >     maxItems: 1
> >
> > As far as I know, spi-cpol and spi-cpha stand for SPI chip polarity and
> > SPI chip phase respectively. By default, it is assumed that SPI
> > input/output data is available at uprising clock edges, however, some
> > chips may work with different configuration (taking input data and/or
> > push it out in falling edges). I'm not 100% sure but, from what I've
> > seen on IIO, cpol is set to invert the input/output logic (making IO be
> > taken on falling edges) while cpha is usually set when MISO valid out
> > data is available on SCLK falling edge. If anyone has more comments
> > about this please, add them here, I'm curious about it. :)
> >
>
> They may well be constant for a given device (some will cope with
> several combinations).  So the binding should reflect if they 'must'
> be set.
>
> Adding the cross reference is indeed nice.
>
> Jonathan
>
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +
> > > +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>;
> > > +        };
> > > +    };
> > > --
> > > 2.23.0.rc1
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+unsubscribe@xxxxxxxxxxxxxxxxx
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20190902005938.7734-1-rodrigorsdc%40gmail.com.
>