Received-SPF: pass (google.com: domain of linux-kernel+bounces-192611-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 28 May 2024 17:04:40 +0200
From: Francesco Dolcini <francesco@dolcini.it>
To: Conor Dooley <conor@kernel.org>
Cc: Francesco Dolcini <francesco@dolcini.it>,
	=?iso-8859-1?Q?Jo=E3o_Paulo_Gon=E7alves?= <jpaulo.silvagoncalves@gmail.com>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	=?iso-8859-1?Q?Jo=E3o_Paulo_Gon=E7alves?= <joao.goncalves@toradex.com>,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Francesco Dolcini <francesco.dolcini@toradex.com>
Subject: Re: [PATCH v1 1/2] dt-bindings: iio: adc: add ti,ads1119
Message-ID: <20240528150440.GA15947@francesco-nb>
References: <20240527154050.24975-1-francesco@dolcini.it>
 <20240527154050.24975-2-francesco@dolcini.it>
 <20240527-ecosystem-mountable-d9a6eebc7607@spud>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240527-ecosystem-mountable-d9a6eebc7607@spud>

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