Message-ID: <35944d57af0ed62124e1e7cea544ef357fad1659.camel@gmail.com>
Date: Mon, 30 Sep 2024 09:05:04 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, David Lechner
 <dlechner@...libre.com>
Cc: Antoniu Miclaus <antoniu.miclaus@...log.com>, Lars-Peter Clausen	
 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley	 <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>, Olivier Moysan
	 <olivier.moysan@...s.st.com>, Uwe Kleine-König	
 <ukleinek@...nel.org>, Andy Shevchenko <andy@...nel.org>, Marcelo Schmitt	
 <marcelo.schmitt@...log.com>, João Paulo
 Gonçalves	 <joao.goncalves@...adex.com>, Mike Looijmans
 <mike.looijmans@...ic.nl>,  Dumitru Ceclan <mitrutzceclan@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, 
 Alisa-Dariana Roman <alisadariana@...il.com>, Sergiu Cuciurean
 <sergiu.cuciurean@...log.com>, Dragos Bogdan	 <dragos.bogdan@...log.com>,
 linux-iio@...r.kernel.org, 	linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, 	linux-pwm@...r.kernel.org
Subject: Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:
> 
> > 
> > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > +                          IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > +       {},
> > > +};
> > > +
> > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > +                          IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > +       {},
> > > +};  
> > 
> > Usually, something like this packet format would be automatically
> > selected when buffered reads are enabled based on what other features
> > it provides are needed, i.e only enable the status bits when events
> > are enabled.
> > 
> > (For those that didn't read the datasheet, the different packet
> > formats basically enable extra status bits per sample. And in the case
> > of oversampling, one of the formats also selects a reduced number of
> > sample bits.)
> > 
> > We have quite a few parts in the pipline right like this one that have
> > per-sample status bits. In the past, these were generally handled with
> > IIO events, but this doesn't really work for these high-speed backends
> > since the data is being piped directly to DMA and we don't look at
> > each sample in the ADC driver. So it would be worthwhile to try to
> > find some general solution here for handling this sort of thing.

I did not read the datasheet that extensively but here it goes my 2 cents
(basically my internal feedback on this one):

So this packet format thingy may be a very "funny" discussion if we really need
to support it. I'm not sure how useful it is the 32 bits format rather than
being used in test pattern. I'm not seeing too much benefit on the channel id or
span id information (we can already get that info with other attributes). The
OR/UR is the one that could be more useful but is there someone using it? Do we
really need to have it close to the sample? If not, there's the status register
and... Also, I think this can be implemented using IIO events (likely what we
will be asked). So what comes to mind could be:

For test_pattern (could be implemented as ext_info or an additional channel I
think - not for now I guess) we can easily look at our word side and dynamically
set the proper packet size. So, to me, this is effectively the only place where
32bits would make sense (assuming we don't implement OR/UR for now).
For oversampling we can have both 20/24 bit averaged data. But from the
datasheet:

"Oversampling is useful in applications requiring lower noise and higher dynamic
range per output data-word, which the AD4858 supports with 24-bit output
resolution and reduced average output data rates"

So from the above it looks like it could make sense to default to 24bit packets
if oversampling is enabled.

Now the question is OR/UR. If that is something we can support with events, we
could see when one of OR/UR is being requested to either enable 24 packets (no
oversampling) or 32 bit packets (oversampling on).



> 
> We have previously talked about schemes to describe metadata
> alongside channels. I guess maybe it's time to actually look at how
> that works.  I'm not sure dynamic control of that metadata
> is going to be easy to do though or if we even want to
> (as opposed to always on or off for a particular device).
> 

Indeed this is something we have been discussing and the ability to have status
alongside a buffered samples is starting to be requested more and more. Some
parts do have the status bit alongside the sample (meaning in the same register
read) which means it basically goes with the sample as part of it's
storage_bits. While not ideal, an application caring about those bits still has
access to the complete raw sample and can access them. It gets more complicated
where the status (sometimes a per device status register) is located in another
register. I guess we can have two case:

1) A device status which applies for all channels being sampled;
2) A per channel status (where the .metada approach could make sense).

But I'm not sure how we could define something like this other than assuming
that raw status data is being sent to userspace (given that every device has
it's own custom status bits and quirks).

- Nuno Sá  

Powered by blists - more mailing lists