Re: [PATCH RFC v6 09/10] iio: adc: ad7380: add support for rolling average oversampling mode

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 9 May 2024 17:01:14 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Wed, May 8, 2024 at 6:26 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Mon, 6 May 2024 10:04:10 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >  
> > > On Mon, May 6, 2024 at 9:17 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Wed, 01 May 2024 16:55:42 +0200
> > > > Julien Stephan <jstephan@baylibre.com> wrote:
> > > >  
> > > > > Adds support for rolling average oversampling mode.
> > > > >
> > > > > Rolling oversampling mode uses a first in, first out (FIFO) buffer of
> > > > > the most recent samples in the averaging calculation, allowing the ADC
> > > > > throughput rate and output data rate to stay the same, since we only need
> > > > > to take only one sample for each new conversion.
> > > > >
> > > > > The FIFO length is 8, thus the available oversampling ratios are 1, 2, 4, 8
> > > > > in this mode (vs 1,  2, 4, 8, 16, 32 for the normal average)  
> > > >
> > > > Ah. I should have read on!
> > > >  
> > > > >
> > > > > In order to be able to change the averaging mode, this commit also adds
> > > > > the new "oversampling_mode" and "oversampling_mode_available" custom
> > > > > attributes along with the according documentation file in
> > > > > Documentation/ABI/testing/sysfs-bus-iio-adc-ad7380 since no standard
> > > > > attributes correspond to this use case.  
> > > >
> > > > This comes to the comment I stuck in the previous patch.
> > > >
> > > > To most people this is not a form of oversampling because the data rate
> > > > remains unchanged. It's a cheap low pass filter (boxcar) Google pointed me at:
> > > > https://dsp.stackexchange.com/questions/9966/what-is-the-cut-off-frequency-of-a-moving-average-filter
> > > >
> > > > in_voltage_low_pass_3db_frequency would be the most appropriate standard
> > > > ABI for this if we do treat it as a low pass filter control.
> > > >
> > > > I'm not necessarily saying we don't want new ABI for this, but I would
> > > > like to consider the pros and cons of just using the 3db frequency.
> > > >
> > > > So would that work for this part or am I missing something?
> > > >  
> > >
> > > I like the idea. But from the link, it looks like the 3dB frequency
> > > depends on the sampling frequency which is unknown (e.g. could come
> > > from hrtimer trigger).
> > >
> > > Would it be reasonable to calculate the 3db frequency at the max
> > > sample rate that the chip allows and just use those numbers?
> > >  
> > Ah. So looking at datasheet the normal average oversampling is
> > self clocked, but this version is not.
> >
> > So, I'll ask the dumb question.  What is this feature for?
> > We have to pump the SPI bus anyway why not just do the maths in
> > userspace?  Oversampling is normally about data rate reduction
> > with a bonus in precision obtained.
> >
> > Jonathan
> >  
> 
> I asked the apps engineers and the answer I got is that it a way to
> enable oversampling while still maintaining a high sample rate.

If I read it correctly (and based on how this is done on some other
devices) it's exactly the same as not enabling this mode and in
software adding up the last 8 readings (so software oversampling).
Data rate is the same and arguably the larger spi transfers that
might result from their solution are more expensive than doing the
sum on the CPU.

A long time back there was IIRC some discussion of implementing
this sort of filtering as a pluggable component between an IIO
device driver and the kfifo - idea being to reduce data being
passed to userspace - there would be no point in doing
this variant though as the data rate isn't reduced.  It would be
easy to do as a buffer consumer IIO device that just does maths
on incoming data before providing it's own kfifo, but no one
ever implemented it.

> 
> Another thing to consider here is that we can only enable the extra
> resolution bits if oversampling is enabled (normal or rolling mode).
> The chip might not work right if we try to enable the extra bits
> without oversampling enabled.

I think the key question is where do those extra bits come from? 
If this is conventional oversampling then we just sum up the readings
and divide by how many there are.
(A1 + A2 + A3 + A4 + A5 + A6 + A7) / 8
So sum plus shift right 3.  However you don't actually do the div 8
you just change the scale to reflect that your new LSB is 1/8th of
that before giving greater precision. This is giving 2 extra bits, so
it's slightly worse than the 3 we'd get doing it in software (trade
off to keep the SPI transfers smaller).  So they are either dropping the
LSB or just possibly dithering it.

So far I'm not seeing a strong reason to support this functionality.

> 
> So my thinking is perhaps it is better to keep the rolling mode as
> oversampling rather than trying to call it a low pass filter. As you
> said, normal mode is about data rate reduction with bonus precision.
> Rolling average oversampling mode then would then just be for the
> bonus precision.

True if there actually is any. I took another close read through
the datasheet sections and the description aligns with my assumptions
above.

Jonathan