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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240528150440.GA15947@francesco-nb>
Date: Tue, 28 May 2024 17:04:40 +0200
From: Francesco Dolcini <francesco@...cini.it>
To: Conor Dooley <conor@...nel.org>
Cc: Francesco Dolcini <francesco@...cini.it>,
	João Paulo Gonçalves <jpaulo.silvagoncalves@...il.com>,
	Jonathan Cameron <jic23@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	João Paulo Gonçalves <joao.goncalves@...adex.com>,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Francesco Dolcini <francesco.dolcini@...adex.com>
Subject: Re: [PATCH v1 1/2] dt-bindings: iio: adc: add ti,ads1119

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@...adex.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@...adex.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@...adex.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