Message-ID: <33509253-b633-46a5-a760-2a6416d835ee@baylibre.com>
Date: Thu, 9 Jan 2025 13:09:29 -0500
From: Trevor Gamblin <tgamblin@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Michael Hennerich <michael.hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>, Lars-Peter Clausen <lars@...afoo.de>,
 Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 0/2] iio: adc: ad4695: add oversampling support


On 2024-12-19 10:46, Jonathan Cameron wrote:
> On Tue, 17 Dec 2024 16:47:27 -0500
> Trevor Gamblin <tgamblin@...libre.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).
Hi Jonathan,
> 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.).

Thanks for providing all of the info above. I think that clears things 
up enough for this series (i.e. that the current implementation is the 
right way to do it). I will take a note about adding some helper 
functions for this, and about updating rtq6056, although I don't have 
access to hardware to test for the latter case. v2 should be coming soon.

- Trevor

Powered by blists - more mailing lists