Re: [PATCH 0/2] iio: adc: ad4695: add oversampling support

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 17 Dec 2024 16:47:27 -0500
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> Add driver logic and documentation for the oversampling feature of the
> AD469x parts from Analog Devices. For now, this only works with offload
> support, and takes advantage of that mode's higher performance to make
> oversampling possible on multiple channels with varying sampling
> frequencies. Some significant rework of the driver had to be done in
> order to conditionally support this feature, including use of
> iio_scan_types to help determine the appropriate spi message
> configurations depending on oversampling ratio.
> 
> A dilemma that came up during development of this feature was whether or
> not the implementation of oversampling ratios against sampling frequency
> was actually correct. More specifically, it's unclear if the sampling
> frequency attribute is supposed to be the conversion rate or the data
> read rate (according to the IIO subsystem). 

Intent was it's the rate at which the reading becomes available to the
software (which I think you are calling data read rate).

In global schemes, it's all of the scan (normally they are
self clocked with delays between scans) in per channel schemes intent
was to provide the sampling rate at which that channel was sampled
if no others were being read (assuming the times sum up in a linear
way etc).

> If it's the former, then
> this implementation is probably incorrect. David Lechner pointed out
> during review that it would be easier if it were defined as the
> conversion rate and that it was userspace's responsibility to handle
> oversampling ratio, but that might also require more work in the IIO
> subsystem.

IIRC there are devices out there were oversampling rate is on top of
their controlled read out rate (which is more about delays between
starting sampling than about how long it takes).  I think one of those
drove the implementation of oversampling in the first place.
It's also not the case that an oversampling rate of x4 always means
that the time to data availability is necessarily 1/4 (can pipeline
some stages depending on the sensor design).

My thinking (it was a long time ago) was that userspace wouldn't want
to deal with the scaling.  We only fairly recently defined the
clear concept of per channel sampling frequencies. Before that they
were (almost?) all global.

Another aspect is that when we originally added this, it was new to userspace
so having defined sampling frequency, we couldn't have it's meaning
changing because oversampling was say a minimum of 2 on a device.

> Two other ADC drivers that were referenced for inspiration
> when working through this were the ad7380 and the rtq6056. The ad7380
> has a global oversampling setting rather than per-channel, and the

I'm not sure it being global matters a lot for question of whether
it takes into account oversampling or not.

> rtq6056 seems at least partially broken because it only takes
> oversampling ratio into account when getting the sampling frequency (but
> not when setting it). 

That is definitely broken and could do with a fix.  We should always
read back more or less the same value written (subject to rounding
etc in some cases).


> Instead of per-driver implementation, these three
> drivers might serve as inspiration for changes to how oversampling is
> handled in IIO?

It is very tricky to make any modifications (other than the fix above)
without making a mess of the ABI.

Or are you suggesting we could so something in the IIO core?  Maybe
some helper functions are appropriate (similar to Matti's GTS stuff
for gains where they are a mixture of various factors on light sensors
and similar.).

Jonathan


> 
> This series depends on David's recent SPI engine changes for adding
> offload support:
> 
> https://lore.kernel.org/all/20241211-dlech-mainline-spi-engine-offload-2-v6-0-88ee574d5d03@baylibre.com/
> 
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
> Trevor Gamblin (2):
>       iio: adc: ad4695: add offload-based oversampling support
>       doc: iio: ad4695: describe oversampling support
> 
>  Documentation/iio/ad4695.rst |  36 ++++-
>  drivers/iio/adc/ad4695.c     | 378 +++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 383 insertions(+), 31 deletions(-)
> ---
> base-commit: 0c6c3bf84f541fb4ec7097baf9eac10136f98c62
> change-id: 20241217-ad4695-oversampling-2946fbe3aff3
> 
> Best regards,