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

[Jonathan Cameron]

On Fri, 11 Oct 2024 12:23:49 +0200
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:

> On Sat, 2024-10-05 at 18:26 +0100, Jonathan Cameron wrote:
> > On Mon, 30 Sep 2024 09:05:04 +0200
> > Nuno Sá <noname.nuno@xxxxxxxxx> 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. 
> >   
> 
> I guess you mean that a complete solution would never only be a IIO_METADATA but
> also extending 'struct iio_scan_type'?

Yes we needs a way to refer to an existing scan element then we could add additional
channels and refer into them as needed.

> 
> > 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.  
> 
> Not sure I followed the above... You mean having a single channel (like one
> register) pointing to different channels? 

Both way's around occur.  Multiple channels, some normal, some metadata some
separate pointing to a single storage location and per channel meta data
for different 'signals' shoved in one register.

> 
> What I mean with 1) is for example what happens with IMUs (eg: adis16475). They
> have a DIAG_STAT register (which is pretty much device wide status/error
> information) that's also part of burst transfers (where we get our samples) and
> while some bits might not be meaningful for the sampling "session", others might
> make sense to reason about data integrity or correctness. 
> 
> For the above case, I think the IIO_METADATA channel type would fit.
That one is easy. Fiddly case is where we have say overflow bits for
multiple signals (i.e. pins) in a single dmabuffer element.
To make that work cleanly we need a way to not only describe the contents
but cross reference it to the related data.

We've discussed ways to group actual channel (i.e. current and power on same pin)
in the past but doing this for metadata that is packed in multiple channels
is going to be a real pain.

Basically it all needs to be very flexible and any attempt to support a subset
is likely to wall us into an inflexible ABI.

Jonathan

> 
> - Nuno Sá