RE: [PATCH 2/2] bindings: iio: adc: Add documentation for ADS131E0x ADC driver

["Denis, Tomislav AVL DiTEST" <Tomislav.Denis@avl.com>]

Hi Jonathan,

Thanks for great review and hints that you gave me.
A few comments inline.

BR,

Tomislav

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: 28 November 2020 13:34
> To: Denis, Tomislav AVL DiTEST <Tomislav.Denis@avl.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH 2/2] bindings: iio: adc: Add documentation for ADS131E0x
> ADC driver
> 
> On Fri, 27 Nov 2020 20:42:40 +0100
> <tomislav.denis@avl.com> wrote:
> 
> > From: Tomislav Denis <tomislav.denis@avl.com>
> >
> > Add a device tree binding documentation for Texas Instruments
> > ADS131E0x ADC family driver.
> >
> > Signed-off-by: Tomislav Denis <tomislav.denis@avl.com>
> Hi Tomislav,
> 
> A few comments inline.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../devicetree/bindings/iio/adc/ti,ads131e08.yaml  | 145
> +++++++++++++++++++++
> >  MAINTAINERS                                        |   1 +
> >  2 files changed, 146 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/adc/ti,ads131e08.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/iio/adc/ti,ads131e08.yaml
> > b/Documentation/devicetree/bindings/iio/adc/ti,ads131e08.yaml
> > new file mode 100644
> > index 0000000..92da193
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads131e08.yaml
> > @@ -0,0 +1,145 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://urldefense.com/v3/__http://devicetree.org/schemas/iio/adc/ti,
> > +ads131e08.yaml*__;Iw!!Oq50-tQ!_S4huPtQ7bwYwG-
> J3eOtmYscvX_TjlRfR7tjpa6
> > +d2dqUQ67RUNI9X1VKpsiphO1jsQ$
> > +$schema:
> > +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
> > +aml*__;Iw!!Oq50-tQ!_S4huPtQ7bwYwG-
> J3eOtmYscvX_TjlRfR7tjpa6d2dqUQ67RUN
> > +I9X1VKpsi1XPH8nA$
> > +
> > +title: Texas Instruments ADS131E0x 4-, 6-, and 8-Channel ADCs
> 
> Not currently supporting 6 channel variants?

It should be working without problem! But since I don't have HW to test I've left it out. 

> 
> > +
> > +maintainers:
> > +  - Tomislav Denis <tomislav.denis@avl.com>
> > +
> > +description: |
> > +  The ADS131E0x are a family of multichannel, simultaneous sampling,
> > +  24-bit, delta-sigma, analog-to-digital converters (ADCs) with a
> > +  built-in programmable gain amplifier (PGA), internal reference
> > +  and an onboard oscillator.
> > +  The communication with ADC chip is via the SPI bus (mode 1).
> > +
> > +
> > + https://urldefense.com/v3/__https://www.ti.com/lit/ds/symlink/ads131
> > + e08.pdf__;!!Oq50-tQ!_S4huPtQ7bwYwG-
> J3eOtmYscvX_TjlRfR7tjpa6d2dqUQ67R
> > + UNI9X1VKpsjgTd5HaA$
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ti,ads131e04
> > +      - ti,ads131e08
> > +
> > +  reg:
> > +    description: |
> > +      SPI chip select number
> 
> That is entirely standard so no real need to put a description of reg for an spi
> device.
> 
> > +    maxItems: 1
> > +
> > +  spi-cpha: true
> > +
> > +  clocks:
> > +    description: |
> > +      Device tree identifier to the clock source (2.048 MHz)
> > +      Note: clock source is selected using CLKSEL pin
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: adc-clk
> > +
> > +  interrupts:
> > +    description: |
> > +      IRQ line for the ADC data ready
> > +    maxItems: 1
> > +
> > +  vref-supply:
> > +    description: |
> > +      Optional external voltage reference. Has to be supplied, if
> > +      ti,vref-sel equals 2
> > +
> > +  ti,vref-sel:
> > +    description: |
> > +      Select the voltage reference source
> > +      Valid values are:
> > +      0: Internal reference 2.4V
> > +      1: Internal reference 4V
> > +      2: External reference source (vref-supply is required)
> 
> With optional external references we normally just use their presense to indicate
> that they should be used.
> 
> You'll still need a parameter to pick the internal reference though.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1, 2]
> > +    default: 0
> > +
> > +  ti,datarate:
> > +    description: |
> > +      ADC data rate in kSPS
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2, 4, 8, 16, 32, 64]
> > +    default: 1
> 
> Why is this a devicetree element rather than runtime controllable?

Number of data bytes per channel depends on data rate. To be able to change data rate 
dynamically would require to change scan_type.realbits also dynamically! I am not sure
if that make sense and also if it is possible to do it?

> 
> > +
> > +  ti,gain:
> > +    description: |
> > +      The gain value for the PGA function
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [1, 2, 4, 8, 12]
> > +    default: 1
> 
> Isn't this per channel?  Also this explanation should mention why it is a board
> related characteristic rather than a runtime tuneable (I'm fine with having it here,
> but good to add a bit of info on that to the description).
> 
> > +
> > +  ti,adc-channels:
> > +    description: |
> > +      List of single-ended channels muxed for this ADC
> > +      - 4 channels, numbered from 0 to 3 for ti,ads131e04
> > +      - 8 channels, numbered from 0 to 7 for ti,ads131e08
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> 
> We've fairly recently introduced a generic adc channel binding that I'd prefer is
> used in drivers going forwards.
> 
> See Documentation/device-tree/bindings/iio/adc.txt (or yaml if I've applied that
> patch before you get to this).
> 
> That adds a subnode per channel and gives us an easy way to then provide per
> channel parameters.  It's heavier weight than what you have here, but much
> more flexible.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - spi-cpha
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - ti,adc-channels
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: ti,ads131e04
> > +
> > +  - then:
> > +      properties:
> > +        ti,adc-channels:
> > +          minItems: 1
> > +          maxItems: 4
> > +          items:
> > +            minimum: 0
> > +            maximum: 3
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: ti,ads131e08
> > +
> > +  - then:
> > +      properties:
> > +        ti,adc-channels:
> > +          minItems: 1
> > +          maxItems: 8
> > +          items:
> > +            minimum: 0
> > +            maximum: 7
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    spidev@0 {
> > +      compatible = "ti,ads131e08";
> > +      reg = <0>;
> > +      spi-max-frequency = <1000000>;
> > +      spi-cpha;
> > +      clocks = <&clk2048k>;
> > +      clock-names = "adc-clk";
> > +      interrupt-parent = <&gpio5>;
> > +      interrupts = <28 IRQ_TYPE_EDGE_FALLING>;
> > +      vref-supply = <&vref_reg>;
> > +      ti,vref-sel = <2>;
> > +      ti,datarate = <1>;
> > +      ti,gain = <1>;
> > +      ti,adc-channels = <0 1 2 3 4 5 6 7>;
> > +    };
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS index 28bc5f9..0c351c7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17224,6 +17224,7 @@ TI ADS131E0X ADC SERIES DRIVER
> >  M:	Tomislav Denis <tomislav.denis@avl.com>
> >  L:	linux-iio@vger.kernel.org
> >  S:	Maintained
> > +F:	Documentation/devicetree/bindings/iio/adc/ti,ads131e08.yaml
> >  F:	drivers/iio/adc/ti-ads131e08.c
> >
> >  TI AM437X VPFE DRIVER