Message-ID: <20240216142158.30e96c53@jic23-huawei>
Date: Fri, 16 Feb 2024 14:21:58 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Alisa-Dariana Roman <alisadariana@...il.com>,
 alexandru.tachici@...log.com, alisa.roman@...log.com, conor+dt@...nel.org,
 devicetree@...r.kernel.org, krzysztof.kozlowski+dt@...aro.org,
 krzysztof.kozlowski@...aro.org, lars@...afoo.de, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, michael.hennerich@...log.com,
 robh+dt@...nel.org, Nuno Sa <nuno.sa@...log.com>
Subject: Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support

On Thu, 15 Feb 2024 11:13:19 -0600
David Lechner <dlechner@...libre.com> wrote:

> On Thu, Feb 15, 2024 at 7:22 AM Alisa-Dariana Roman
> <alisadariana@...il.com> wrote:
> >
> > Hello and thank you for the feedback!
> >
> > On 09.02.2024 00:27, David Lechner wrote:  
> > > On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
> > > <alisadariana@...il.com> wrote:  
> > >>
> > >> Unlike the other AD719Xs, AD7194 has configurable differential
> > >> channels. The default configuration for these channels can be changed
> > >> from the devicetree.  
> >
> > ...
> >  
> > >>
> > >> +static const struct iio_info ad7194_info = {
> > >> +       .read_raw = ad7192_read_raw,
> > >> +       .write_raw = ad7192_write_raw,
> > >> +       .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> > >> +       .read_avail = ad7192_read_avail,
> > >> +       .validate_trigger = ad_sd_validate_trigger,
> > >> +       .update_scan_mode = ad7192_update_scan_mode,
> > >> +};  
> > >
> > > Isn't this identical to ad7192_info and ad7195_info now that .attrs is
> > > removed? It seems like we could consolidate here.  
> >
> > Those are not exactly identical since: 92 has bridge switch attribute,
> > 95 has bridge switch and ac excitation attributes and 94 has no custom
> > attributes. I used a different info structure for 94 in order to avoid
> > showing extra attributes.
> >  
> 
> Ah, I see what you mean. I didn't look close enough at the other patch
> removing one attribute to see that were still other attributes.
> 
> > >  
> > >> +
> > >>   static const struct iio_info ad7195_info = {
> > >>          .read_raw = ad7192_read_raw,
> > >>          .write_raw = ad7192_write_raw,
> > >> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
> > >>          IIO_CHAN_SOFT_TIMESTAMP(14),
> > >>   };
> > >>
> > >> +static struct iio_chan_spec ad7194_channels[] = {
> > >> +       AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
> > >> +       AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
> > >> +       AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
> > >> +       AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
> > >> +       AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
> > >> +       AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
> > >> +       AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
> > >> +       AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
> > >> +       AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
> > >> +       AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
> > >> +       AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
> > >> +       AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
> > >> +       AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
> > >> +       AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
> > >> +       AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
> > >> +       AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
> > >> +       AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
> > >> +       AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
> > >> +       AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
> > >> +       AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
> > >> +       AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
> > >> +       AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
> > >> +       AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
> > >> +       AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
> > >> +       AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),  
> > >
> > > Shouldn't these be differential channels since they are
> > > pseudo-differential inputs measuring the difference between AINx and
> > > AINCOM?
> > >  
> > >> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> > >> +};  
> > >
> > > i.e. like this (where AINCOM is voltage0 AINx is voltagex)
> > >
> > > static struct iio_chan_spec ad7194_channels[] = {
> > >         AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
> > >         AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
> > >         AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
> > >         AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
> > >         AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
> > >         AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
> > >         AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
> > >         AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
> > >         AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
> > >         AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
> > >         AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
> > >         AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
> > >         AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
> > >         AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
> > >         AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
> > >         AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
> > >         AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
> > >         IIO_CHAN_SOFT_TIMESTAMP(17),
> > > };
> > >  
> >
> > I tried to follow the existing style of the driver: for each
> > pseudo-differential channel(AINx - AINCOM) there is an iio channel like
> > this in_voltagex_raw; and for each differential channel(AINx - AINy)
> > there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194
> > has 16 pseudo-differential channels/8 fully differential channels so I
> > thought the (AINx - AINCOM) channels should be static and only the 8
> > differential ones could be configured by the user from the devicetree by
> > choosing the input pins.
> >
> > The existing style of the driver, AD7192 has 4 pseudo differential
> > channels and 2 (non configurable) differential channels:
> > static const struct iio_chan_spec ad7192_channels[] = {
> >         AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M),
> >         AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M),
> >         AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP),
> >         AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M),
> >         AD719x_CHANNEL(4, 1, AD7192_CH_AIN1),
> >         AD719x_CHANNEL(5, 2, AD7192_CH_AIN2),
> >         AD719x_CHANNEL(6, 3, AD7192_CH_AIN3),
> >         AD719x_CHANNEL(7, 4, AD7192_CH_AIN4),
> >         IIO_CHAN_SOFT_TIMESTAMP(8),
> > };
> >
> > Would it be better to respect the existing style or to do like you
> > suggested and have a total of 16 differential channels that are
> > configurable from the device tree?  
> 
> Looking at Table 20 in the AD7192 datasheet, I can see why AD7192 was
> done this way since only certain combinations of inputs can be used
> together. (Although I think indexes 4 to 7 should really be
> differential because they are the difference between the input and
> AINCOM which may not be GND, but it is probably too late to fix that.)
Ground is never absolute anyway, but we could indeed be relative to something
that changes. Ah well - no one has asked for it on that part I guess
so not important.

> 
> Tables 22, 23 and 24 in the AD7194 datasheet show that this chip is
> much more configurable than AD7192 when it comes to assigning
> channels. There are basically no restrictions on which inputs can be
> used together. So I am still confident that my suggestion is the way
> to go for AD7194. (Although I didn't actually try it on hardware, so
> can't be 100% confident. But at least 90% confident :-p)
You would have to define a channel number for aincom.  There is an explicit
example in the datasheet of it being at 2.5V using a reference supply.

I wonder what expectation here is.  Allways a reference regulator on that pin, or
an actually varying input?  Maybe in long term we want to support both
options - so if aincom-supply is provided these are single ended with
an offset, but if not they are differential channels between channel X and
channel AINCOM.

Note though that this mode is described a pseudo differential which normally
means a fixed voltage on the negative.

So gut feeling from me is treat them as single ended and add an
aincom-supply + the offsets that result if that is provided in DT and
voltage from it is non 0.

What fun.

Jonathan

Powered by blists - more mailing lists