Hi Jonathan, Thanks for great review and hints that you gave me. A few comments inline. BR, Tomislav > -----Original Message----- > From: Jonathan Cameron <jic23@xxxxxxxxxx> > Sent: 28 November 2020 13:34 > To: Denis, Tomislav AVL DiTEST <Tomislav.Denis@xxxxxxx> > Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > 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@xxxxxxx> wrote: > > > From: Tomislav Denis <tomislav.denis@xxxxxxx> > > > > Add a device tree binding documentation for Texas Instruments > > ADS131E0x ADC family driver. > > > > Signed-off-by: Tomislav Denis <tomislav.denis@xxxxxxx> > 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@xxxxxxx> > > + > > +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@xxxxxxx> > > L: linux-iio@xxxxxxxxxxxxxxx > > S: Maintained > > +F: Documentation/devicetree/bindings/iio/adc/ti,ads131e08.yaml > > F: drivers/iio/adc/ti-ads131e08.c > > > > TI AM437X VPFE DRIVER
