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

[Conor Dooley]

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@xxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxx>
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> > > ---
> > >  .../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: signature.asc
Description: PGP signature