Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 30 Sep 2024 09:05:04 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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:

Definite preference for using events, but for a device doing DMA I'm not sure
how we can do that without requiring parsing all the data.

So we would need some metadata description to know it is there.

> 
> 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.

That sounds like what we do for the DMA oversampling cases that change
the resolutions.

> 
> 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. 

This has the advantage that if we come along later and define a metadata
in storage bits description it is backwards compatible.  We've been doing
this for years with some devices.

> 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).

If it's a separate register per channel and optional, we'd have to treat it as a metadata
channel as no guarantee it would be packed next to the main channel.

If we have a description of metadata additions in main channel storage, I'm not
against having a IIO_METADATA channel type. 

If it's a single channel I'm not sure how we'd make as channel description
general enough easily as we end up with every field possibly needed an association
with a different channel.

> 
> 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).
That is always fine.

Jonathan
> 
> - Nuno Sá