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

Message

Trevor Gamblin Dec. 17, 2024, 9:47 p.m. UTC

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). 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. 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
rtq6056 seems at least partially broken because it only takes
oversampling ratio into account when getting the sampling frequency (but
not when setting it). Instead of per-driver implementation, these three
drivers might serve as inspiration for changes to how oversampling is
handled in IIO?

This series depends on David's recent SPI engine changes for adding
offload support:

https://lore.kernel.org/all/[e-mail]/

Signed-off-by: Trevor Gamblin <[e-mail]>
---
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,

Comments

Jonathan Cameron Dec. 19, 2024, 3:46 p.m. UTC | #1

On Tue, 17 Dec 2024 16:47:27 -0500
Trevor Gamblin <[e-mail]> 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/[e-mail]/
> 
> Signed-off-by: Trevor Gamblin <[e-mail]>
> ---
> 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,