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

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

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@gmail.com>
> ---
> 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@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: 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. :)

> +
> +  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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/20190902005938.7734-1-rodrigorsdc%40gmail.com.