[PATCH v1 0/2] iio: adc: ti-ads1119: Add driver

[PATCH v1 0/2] iio: adc: ti-ads1119: Add driver

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

The ADS1119 is a precision, 16-bit, analog-to-digital converter (ADC)
that features two differential or four single-ended inputs through a
flexible input multiplexer (MUX), rail-to-rail input
buffers, a programmable gain stage, a voltage reference, and an
oscillator.

Apart from normal single conversion, the driver also supports
continuous conversion mode using a triggered buffer. However, in this
mode only one channel can be scanned at a time, and it only uses the data
ready interrupt as a trigger. This is because the device channels are
multiplexed, and using its own data ready interrupt as a trigger guarantees
the signal sampling frequency.

João Paulo Gonçalves (2):
  dt-bindings: iio: adc: add ti,ads1119
  iio: adc: ti-ads1119: Add driver

 .../bindings/iio/adc/ti,ads1119.yaml          | 122 +++
 MAINTAINERS                                   |   8 +
 drivers/iio/adc/Kconfig                       |  13 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/ti-ads1119.c                  | 815 ++++++++++++++++++
 5 files changed, 959 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
 create mode 100644 drivers/iio/adc/ti-ads1119.c

-- 
2.39.2

[PATCH v1 1/2] dt-bindings: iio: adc: add ti,ads1119

[Francesco Dolcini]

From: João Paulo Gonçalves <joao.goncalves@toradex.com>

Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
with I2C interface.

Datasheet: https://www.ti.com/lit/gpn/ads1119
Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 .../bindings/iio/adc/ti,ads1119.yaml          | 122 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 129 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
new file mode 100644
index 000000000000..ab4f01199dbe
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
@@ -0,0 +1,122 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,ads1119.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments ADS1119 ADC
+
+maintainers:
+  - João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
+
+description: |
+  The TI ADS1119 is a precision 16-bit ADC over I2C that offers single-ended and
+  differential measurements using a multiplexed input. It features a programmable
+  gain, a programmable sample rate, an internal oscillator and voltage reference,
+  and a 50/60Hz rejection filter.
+
+properties:
+  compatible:
+    const: ti,ads1119
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  vref-supply:
+    description:
+      ADC external reference voltage (VREF).
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  "#io-channel-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+patternProperties:
+  "^channel@([0-6])$":
+    $ref: adc.yaml
+    type: object
+    description: |
+      ADC channels.
+
+    properties:
+      reg:
+        description: |
+          0: Voltage over AIN0 and AIN1.
+          1: Voltage over AIN2 and AIN3.
+          2: Voltage over AIN1 and AIN2.
+          3: Voltage over AIN0 and GND.
+          4: Voltage over AIN1 and GND.
+          5: Voltage over AIN2 and GND.
+          6: Voltage over AIN3 and GND.
+        items:
+          - minimum: 0
+            maximum: 6
+
+      ti,gain:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: |
+          PGA gain value.
+        enum: [1, 4]
+        default: 1
+
+      ti,datarate:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: |
+          Data acquisition rate in samples per second.
+        enum: [20, 90, 330, 1000]
+        default: 20
+
+    required:
+      - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@40 {
+            compatible = "ti,ads1119";
+            reg = <0x40>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+            #io-channel-cells = <1>;
+
+            channel@0 {
+                reg = <0>;
+                ti,gain = <4>;
+                ti,datarate = <330>;
+            };
+
+            channel@4 {
+                reg = <4>;
+                ti,datarate = <1000>;
+            };
+
+            channel@5 {
+                reg = <5>;
+                ti,gain = <4>;
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d6c90161c7bf..f1b2c4b815e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22380,6 +22380,13 @@ M:	Robert Richter <rric@kernel.org>
 S:	Odd Fixes
 F:	drivers/gpio/gpio-thunderx.c
 
+TI ADS1119 ADC DRIVER
+M:	Francesco Dolcini <francesco@dolcini.it>
+M:	João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
+
 TI ADS7924 ADC DRIVER
 M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
 L:	linux-iio@vger.kernel.org
-- 
2.39.2

Re: [PATCH v1 1/2] dt-bindings: iio: adc: add ti,ads1119

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5076 bytes --]

On Mon, May 27, 2024 at 05:40:49PM +0200, Francesco Dolcini wrote:
> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> with I2C interface.
> 
> Datasheet: https://www.ti.com/lit/gpn/ads1119
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  .../bindings/iio/adc/ti,ads1119.yaml          | 122 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> new file mode 100644
> index 000000000000..ab4f01199dbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> @@ -0,0 +1,122 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/ti,ads1119.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments ADS1119 ADC
> +
> +maintainers:
> +  - João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
> +
> +description: |

Most of the |s you have in this file are not needed - you don't have any
formatting to preserve in all but the channels.

> +  The TI ADS1119 is a precision 16-bit ADC over I2C that offers single-ended and
> +  differential measurements using a multiplexed input. It features a programmable
> +  gain, a programmable sample rate, an internal oscillator and voltage reference,
> +  and a 50/60Hz rejection filter.
> +
> +properties:
> +  compatible:
> +    const: ti,ads1119
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  vref-supply:
> +    description:
> +      ADC external reference voltage (VREF).
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 0
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +patternProperties:
> +  "^channel@([0-6])$":
> +    $ref: adc.yaml
> +    type: object
> +    description: |
> +      ADC channels.
> +
> +    properties:
> +      reg:
> +        description: |
> +          0: Voltage over AIN0 and AIN1.
> +          1: Voltage over AIN2 and AIN3.
> +          2: Voltage over AIN1 and AIN2.
> +          3: Voltage over AIN0 and GND.
> +          4: Voltage over AIN1 and GND.
> +          5: Voltage over AIN2 and GND.
> +          6: Voltage over AIN3 and GND.

Take a look at diff-channels.

> +        items:
> +          - minimum: 0
> +            maximum: 6
> +
> +      ti,gain:

What makes this a property of the hardware?
Also, missing unit.

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: |
> +          PGA gain value.
> +        enum: [1, 4]
> +        default: 1
> +
> +      ti,datarate:

Ditto here, why's this a property of the hardware? Usually this gets set
from sysfs..

Thanks,
Conor.

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: |
> +          Data acquisition rate in samples per second.
> +        enum: [20, 90, 330, 1000]
> +        default: 20
> +
> +    required:
> +      - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@40 {
> +            compatible = "ti,ads1119";
> +            reg = <0x40>;
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            #io-channel-cells = <1>;
> +
> +            channel@0 {
> +                reg = <0>;
> +                ti,gain = <4>;
> +                ti,datarate = <330>;
> +            };
> +
> +            channel@4 {
> +                reg = <4>;
> +                ti,datarate = <1000>;
> +            };
> +
> +            channel@5 {
> +                reg = <5>;
> +                ti,gain = <4>;
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d6c90161c7bf..f1b2c4b815e2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22380,6 +22380,13 @@ M:	Robert Richter <rric@kernel.org>
>  S:	Odd Fixes
>  F:	drivers/gpio/gpio-thunderx.c
>  
> +TI ADS1119 ADC DRIVER
> +M:	Francesco Dolcini <francesco@dolcini.it>
> +M:	João Paulo Gonçalves <jpaulo.silvagoncalves@gmail.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> +
>  TI ADS7924 ADC DRIVER
>  M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
>  L:	linux-iio@vger.kernel.org
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 1/2] dt-bindings: iio: adc: add ti,ads1119

[Rob Herring (Arm)]

On Mon, 27 May 2024 17:40:49 +0200, Francesco Dolcini wrote:
> From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> 
> Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> with I2C interface.
> 
> Datasheet: https://www.ti.com/lit/gpn/ads1119
> Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  .../bindings/iio/adc/ti,ads1119.yaml          | 122 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 129 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml: ^channel@([0-6])$: Missing additionalProperties/unevaluatedProperties constraint

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240527154050.24975-2-francesco@dolcini.it

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v1 1/2] dt-bindings: iio: adc: add ti,ads1119

[Francesco Dolcini]

On Mon, May 27, 2024 at 05:29:37PM +0100, Conor Dooley wrote:
> On Mon, May 27, 2024 at 05:40:49PM +0200, Francesco Dolcini wrote:
> > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > 
> > Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> > with I2C interface.
> > 
> > Datasheet: https://www.ti.com/lit/gpn/ads1119
> > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> >  .../bindings/iio/adc/ti,ads1119.yaml          | 122 ++++++++++++++++++
> >  MAINTAINERS                                   |   7 +
> >  2 files changed, 129 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > new file mode 100644
> > index 000000000000..ab4f01199dbe
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > @@ -0,0 +1,122 @@

...

> > +patternProperties:
> > +  "^channel@([0-6])$":
> > +    $ref: adc.yaml
> > +    type: object
> > +    description: |
> > +      ADC channels.
> > +
> > +    properties:
> > +      reg:
> > +        description: |
> > +          0: Voltage over AIN0 and AIN1.
> > +          1: Voltage over AIN2 and AIN3.
> > +          2: Voltage over AIN1 and AIN2.
> > +          3: Voltage over AIN0 and GND.
> > +          4: Voltage over AIN1 and GND.
> > +          5: Voltage over AIN2 and GND.
> > +          6: Voltage over AIN3 and GND.
> 
> Take a look at diff-channels.

Yes, we looked at this and at the beginning we did not think this was a
right idea. This is pretty much copying what is done in
Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml.

We could describe this using the diff-channels, however the MUX in the
ADS1119 cannot do any combination, but only a subset (AIN0-AIN1,
AIN2-AIN3 and AIN1-AIN2).

Are you aware of a way to validate this in the DT?
Would something like that work?

adc@40 {
    compatible = "ti,ads1119";
    reg = <0x40>;
    #address-cells = <1>;
    #size-cells = <0>;
    #io-channel-cells = <1>;
    
    channel@0 {
        reg = <0>;
        diff-channels = <3 4>;
	label = "AIN0_AIN1"
    };
    
    channel@1 {
        reg = <1>;
        diff-channels = <5 6>;
	label = "AIN2_AIN3"
    };
    
    channel@2 {
        reg = <2>;
        diff-channels = <4 5>;
	label = "AIN1_AIN2"
    };
    
    channel@3 {
        reg = <3>;
	label = "AIN0"
    };
    
    channel@4 {
        reg = <4>;
	label = "AIN1"
    };
    
    channel@5 {
        reg = <5>;
	label = "AIN2"
    };
    
    channel@6 {
        reg = <6>;
	label = "AIN3"
    };
};


> > +        items:
> > +          - minimum: 0
> > +            maximum: 6
> > +
> > +      ti,gain:
> 
> What makes this a property of the hardware?
> Also, missing unit.

This is a hardware gain from the ADC and it is dimensionless.

> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description:
> > +          PGA gain value.
> > +        enum: [1, 4]
> > +        default: 1
> > +
> > +      ti,datarate:
> 
> Ditto here, why's this a property of the hardware? Usually this gets set
> from sysfs..

The sample rate is a hardware property, you can configure the ADC device
to do the acquisition at a specific rate.

Both these properties are inspired from
Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml.

We could do what you are suggesting here. I am just a concerned on how
this interacts with the iio/afe/ bindings. Specifically, how could I
configure the gain or the data rate when this ADC is used by a
voltage-divider? Maybe iio-rescale driver needs to be extended for such
use case?

Francesco

Re: [PATCH v1 1/2] dt-bindings: iio: adc: add ti,ads1119

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5581 bytes --]

On Tue, May 28, 2024 at 05:04:40PM +0200, Francesco Dolcini wrote:
> On Mon, May 27, 2024 at 05:29:37PM +0100, Conor Dooley wrote:
> > On Mon, May 27, 2024 at 05:40:49PM +0200, Francesco Dolcini wrote:
> > > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > > 
> > > Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> > > with I2C interface.
> > > 
> > > Datasheet: https://www.ti.com/lit/gpn/ads1119
> > > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > ---
> > >  .../bindings/iio/adc/ti,ads1119.yaml          | 122 ++++++++++++++++++
> > >  MAINTAINERS                                   |   7 +
> > >  2 files changed, 129 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > > new file mode 100644
> > > index 000000000000..ab4f01199dbe
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > > @@ -0,0 +1,122 @@
> 
> ...
> 
> > > +patternProperties:
> > > +  "^channel@([0-6])$":
> > > +    $ref: adc.yaml
> > > +    type: object
> > > +    description: |
> > > +      ADC channels.
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description: |
> > > +          0: Voltage over AIN0 and AIN1.
> > > +          1: Voltage over AIN2 and AIN3.
> > > +          2: Voltage over AIN1 and AIN2.
> > > +          3: Voltage over AIN0 and GND.
> > > +          4: Voltage over AIN1 and GND.
> > > +          5: Voltage over AIN2 and GND.
> > > +          6: Voltage over AIN3 and GND.
> > 
> > Take a look at diff-channels.
> 
> Yes, we looked at this and at the beginning we did not think this was a
> right idea. This is pretty much copying what is done in
> Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml.
> 
> We could describe this using the diff-channels, however the MUX in the
> ADS1119 cannot do any combination, but only a subset (AIN0-AIN1,
> AIN2-AIN3 and AIN1-AIN2).
> 
> Are you aware of a way to validate this in the DT?

I'm not sure of a neat way to restrict the options like that, no.

> Would something like that work?

This looks fine to me, but a look from Jonathan would be good.

> 
> adc@40 {
>     compatible = "ti,ads1119";
>     reg = <0x40>;
>     #address-cells = <1>;
>     #size-cells = <0>;
>     #io-channel-cells = <1>;
>     
>     channel@0 {
>         reg = <0>;
>         diff-channels = <3 4>;
> 	label = "AIN0_AIN1"
>     };
>     
>     channel@1 {
>         reg = <1>;
>         diff-channels = <5 6>;
> 	label = "AIN2_AIN3"
>     };
>     
>     channel@2 {
>         reg = <2>;
>         diff-channels = <4 5>;
> 	label = "AIN1_AIN2"
>     };
>     
>     channel@3 {
>         reg = <3>;
> 	label = "AIN0"
>     };
>     
>     channel@4 {
>         reg = <4>;
> 	label = "AIN1"
>     };
>     
>     channel@5 {
>         reg = <5>;
> 	label = "AIN2"
>     };
>     
>     channel@6 {
>         reg = <6>;
> 	label = "AIN3"
>     };
> };
> 
> 
> > > +        items:
> > > +          - minimum: 0
> > > +            maximum: 6
> > > +
> > > +      ti,gain:
> > 
> > What makes this a property of the hardware?
> > Also, missing unit.
> 
> This is a hardware gain from the ADC and it is dimensionless.

Maybe I phrased my question incorrectly. I'll try again:
What makes the gain a fixed property of the hardware?

I guess I was expecting to see a gain in decibels, but w/e.
What do 1 and 4 represent here?

> > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > +        description:
> > > +          PGA gain value.
> > > +        enum: [1, 4]
> > > +        default: 1
> > > +
> > > +      ti,datarate:
> > 
> > Ditto here, why's this a property of the hardware? Usually this gets set
> > from sysfs..
> 
> The sample rate is a hardware property, you can configure the ADC device
> to do the acquisition at a specific rate.

Same thing here, poorly phrased question from me I think. Why is this is
a fixed property of the hardware, rather than something that a user may
want to control? Last time I saw one of these kind of properties,
Jonathan commented:
| It's unusual for sampling rate to be a property of the hardware and hence
| suitable for DT binding. Normally we make this a userspace control instead.
| If there is a reason for doing it from DT, that wants to be mentioned here.

> Both these properties are inspired from
> Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml.
> 
> We could do what you are suggesting here. I am just a concerned on how
> this interacts with the iio/afe/ bindings. Specifically, how could I
> configure the gain or the data rate when this ADC is used by a
> voltage-divider? Maybe iio-rescale driver needs to be extended for such
> use case?

For configuring the gain in that scenario, you probably need Jonathan or
Peter to comment on, I'm not sure how the sysfs controls work for that.
I'm not sure what a voltage divider would have to do with the data rate,
so I guess this is something related to how the sysfs files are
structured? Again, it'll probably be Jonathan or one of the guys that
actually deals with the userspace side of things (I haven't beyond a
trivial application) that'll have to answer that.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 1/2] dt-bindings: iio: adc: add ti,ads1119

[Jonathan Cameron]

On Tue, 28 May 2024 17:14:38 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Tue, May 28, 2024 at 05:04:40PM +0200, Francesco Dolcini wrote:
> > On Mon, May 27, 2024 at 05:29:37PM +0100, Conor Dooley wrote:  
> > > On Mon, May 27, 2024 at 05:40:49PM +0200, Francesco Dolcini wrote:  
> > > > From: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > > > 
> > > > Add devicetree bindings for Texas Instruments ADS1119 16-bit ADC
> > > > with I2C interface.
> > > > 
> > > > Datasheet: https://www.ti.com/lit/gpn/ads1119
> > > > Signed-off-by: João Paulo Gonçalves <joao.goncalves@toradex.com>
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > ---
> > > >  .../bindings/iio/adc/ti,ads1119.yaml          | 122 ++++++++++++++++++
> > > >  MAINTAINERS                                   |   7 +
> > > >  2 files changed, 129 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > > > new file mode 100644
> > > > index 000000000000..ab4f01199dbe
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1119.yaml
> > > > @@ -0,0 +1,122 @@  
> > 
> > ...
> >   
> > > > +patternProperties:
> > > > +  "^channel@([0-6])$":
> > > > +    $ref: adc.yaml
> > > > +    type: object
> > > > +    description: |
> > > > +      ADC channels.
> > > > +
> > > > +    properties:
> > > > +      reg:
> > > > +        description: |
> > > > +          0: Voltage over AIN0 and AIN1.
> > > > +          1: Voltage over AIN2 and AIN3.
> > > > +          2: Voltage over AIN1 and AIN2.
> > > > +          3: Voltage over AIN0 and GND.
> > > > +          4: Voltage over AIN1 and GND.
> > > > +          5: Voltage over AIN2 and GND.
> > > > +          6: Voltage over AIN3 and GND.  
> > > 
> > > Take a look at diff-channels.  
> > 
> > Yes, we looked at this and at the beginning we did not think this was a
> > right idea. This is pretty much copying what is done in
> > Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml.
That's a very old binding :(  We are stuck with a bunch of legacy unfortunately.

> > 
> > We could describe this using the diff-channels, however the MUX in the
> > ADS1119 cannot do any combination, but only a subset (AIN0-AIN1,
> > AIN2-AIN3 and AIN1-AIN2).

That's fairly common. So far it's been restricted by documentation
rather than the binding enforcing it.

> > 
> > Are you aware of a way to validate this in the DT?  
> 
> I'm not sure of a neat way to restrict the options like that, no.
Hmm. It's not super tidy but I think it can be done.

Firstly 3-6 are single-channel anyway. There are some ways of ensuring
we have only single-channel or differential-channels in a given channel node.
I took a quick look and the one binding I have that does both today doesn't
actually restrict that :( 
This one that we have under review does though: 
https://lore.kernel.org/all/20240531-ad4111-v4-1-64607301c057@analog.com/
(in that case the mux is fully flexible so it doesn't have this next bit)


    diff-channels:
      oneOf:
        - items:
            const: 0
            const: 1
        - items
            const: 2
            const: 3
        - items
            const: 1
            const: 2

> 
> > Would something like that work?  
> 
> This looks fine to me, but a look from Jonathan would be good.
> 
> > 
> > adc@40 {
> >     compatible = "ti,ads1119";
> >     reg = <0x40>;
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> >     #io-channel-cells = <1>;
> >     
> >     channel@0 {
> >         reg = <0>;
> >         diff-channels = <3 4>;
> > 	label = "AIN0_AIN1"
> >     };
> >     
> >     channel@1 {
> >         reg = <1>;
> >         diff-channels = <5 6>;
> > 	label = "AIN2_AIN3"
> >     };
> >     
> >     channel@2 {
> >         reg = <2>;
> >         diff-channels = <4 5>;
> > 	label = "AIN1_AIN2"
> >     };
> >     
> >     channel@3 {
> >         reg = <3>;
> > 	label = "AIN0"

Use the new single-channel so that 'reg' isn't a weird
magic value. In combined cases with single ended and differential
channels, it's better to just use reg as an index rather than assign
it any real meaning.

> >     };
> >     
> >     channel@4 {
> >         reg = <4>;
> > 	label = "AIN1"
> >     };
> >     
> >     channel@5 {
> >         reg = <5>;
> > 	label = "AIN2"
> >     };
> >     
> >     channel@6 {
> >         reg = <6>;
> > 	label = "AIN3"
> >     };
> > };
> > 
> >   
> > > > +        items:
> > > > +          - minimum: 0
> > > > +            maximum: 6
> > > > +
> > > > +      ti,gain:  
> > > 
> > > What makes this a property of the hardware?
> > > Also, missing unit.  
> > 
> > This is a hardware gain from the ADC and it is dimensionless.  
> 
> Maybe I phrased my question incorrectly. I'll try again:
> What makes the gain a fixed property of the hardware?
> 
> I guess I was expecting to see a gain in decibels, but w/e.
> What do 1 and 4 represent here?

Agreed - this is normally userspace controlled not dt.
> 
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        description:
> > > > +          PGA gain value.
> > > > +        enum: [1, 4]
> > > > +        default: 1
> > > > +
> > > > +      ti,datarate:  
> > > 
> > > Ditto here, why's this a property of the hardware? Usually this gets set
> > > from sysfs..  
> > 
> > The sample rate is a hardware property, you can configure the ADC device
> > to do the acquisition at a specific rate.  
> 
> Same thing here, poorly phrased question from me I think. Why is this is
> a fixed property of the hardware, rather than something that a user may
> want to control? Last time I saw one of these kind of properties,
> Jonathan commented:
> | It's unusual for sampling rate to be a property of the hardware and hence
> | suitable for DT binding. Normally we make this a userspace control instead.
> | If there is a reason for doing it from DT, that wants to be mentioned here.
> 
> > Both these properties are inspired from
> > Documentation/devicetree/bindings/iio/adc/ti,ads1015.yaml.
> > 
> > We could do what you are suggesting here. I am just a concerned on how
> > this interacts with the iio/afe/ bindings. Specifically, how could I
> > configure the gain or the data rate when this ADC is used by a
> > voltage-divider? Maybe iio-rescale driver needs to be extended for such
> > use case?  
> 
> For configuring the gain in that scenario, you probably need Jonathan or
> Peter to comment on, I'm not sure how the sysfs controls work for that.
> I'm not sure what a voltage divider would have to do with the data rate,
> so I guess this is something related to how the sysfs files are
> structured?

So this is a question of providing services.  The ADC is servicing the
measurement of the analog front end.  So the analog front end can control
the features of how it is serviced byt the ADC.  Those are just pass
through controls to the underlying device (with some corner cases
where the AFE is part of the parameter dealt with)

If the AFE driver doesn't yet support this, then fine to extend it.
Mostly they pass through standard ABI but so far read only.
For simple cases like this should be fine to add the write path.
Gain control will be a little fiddly as you'll need to remove the AFE component
to work out what to write to the ADC + figure out how to provide
_available info so that userspace can understand that it can control this.

If you care about the voltage-divider and buffered capture there will
be more to do as that path isn't wired up yet I think.

Jonathan

> Again, it'll probably be Jonathan or one of the guys that
> actually deals with the userspace side of things (I haven't beyond a
> trivial application) that'll have to answer that.
> 
> Thanks,
> Conor.