Re: [PATCH v7 5/6] iio: adc: ad4691: add oversampling support

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, Apr 14, 2026 at 10:02:31AM -0500, David Lechner wrote:
> On 4/14/26 9:25 AM, Sabau, Radu bogdan wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Jonathan Cameron <jic23@kernel.org>
> >> Sent: Sunday, April 12, 2026 8:58 PM
> >> To: David Lechner <dlechner@baylibre.com>
> >> Cc: Sabau, Radu bogdan <Radu.Sabau@analog.com>; Lars-Peter Clausen
> >> <lars@metafoo.de>; Hennerich, Michael <Michael.Hennerich@analog.com>;
> >> Sa, Nuno <Nuno.Sa@analog.com>; Andy Shevchenko <andy@kernel.org>;
> >> Rob Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>;
> >> Conor Dooley <conor+dt@kernel.org>; Uwe Kleine-König
> >> <ukleinek@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown
> >> <broonie@kernel.org>; Linus Walleij <linusw@kernel.org>; Bartosz
> >> Golaszewski <brgl@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>;
> >> Jonathan Corbet <corbet@lwn.net>; Shuah Khan
> >> <skhan@linuxfoundation.org>; linux-iio@vger.kernel.org;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> >> pwm@vger.kernel.org; linux-gpio@vger.kernel.org; linux-doc@vger.kernel.org
> >> Subject: Re: [PATCH v7 5/6] iio: adc: ad4691: add oversampling support
> >>
> >> [External]
> >>
> >> On Fri, 10 Apr 2026 16:15:20 -0500
> >> David Lechner <dlechner@baylibre.com> wrote:
> >>
> >>> On 4/9/26 10:28 AM, Radu Sabau via B4 Relay wrote:
> >>>> From: Radu Sabau <radu.sabau@analog.com>
> >>>>
> >>>> Add per-channel oversampling ratio (OSR) support for CNV burst mode.
> >>>> The accumulator depth register (ACC_DEPTH_IN) is programmed with the
> >>>> selected OSR at buffer enable time and before each single-shot read.
> >>>>
> >>>> Supported OSR values: 1, 2, 4, 8, 16, 32.
> >>>>
> >>>> Introduce AD4691_MANUAL_CHANNEL() for manual mode channels,
> >> which do
> >>>> not expose the oversampling ratio attribute since OSR is not applicable
> >>>> in that mode. A separate manual_channels array is added to
> >>>> struct ad4691_channel_info and selected at probe time; offload paths
> >>>> reuse the same arrays with num_channels capping access before the soft
> >>>> timestamp entry.
> >>>>
> >>>> The reported sampling frequency accounts for the active OSR:
> >>>> effective_freq = oscillator_freq / osr
> >>>
> >>> Technically, the way this is implemented is fine according to IIO ABI
> >>> rules. Writing any attribute can cause others to change. It does
> >>> introduce a potential pitfall though. Currently, changing the OSR will
> >>> change the sampling frequency, so you have to always write
> >> oversampling_ratio
> >>> first, then write sampling_frequency to get what you asked for. If you want
> >>> to change the OSR and keep the same sample rate, you still have to write
> >> both
> >>> attributes again.
> >>>
> >>> In other drivers, I've implemented it so that the requested sampling
> >> frequency
> >>> is stored any you always get the closest sampling frequency available based
> >> on
> >>> the oversampling ratio. This way, it doesn't matter which order you write
> >>> the attributes. In that case, the actual periodic trigger source isn't set up
> >>> until we actually start sampling.
> >>>
> >> Agreed. This is more intuitive. Now generally the userspace should
> >> be sanity checking the value anyway as limitations may mean the new
> >> sampling frequency is not particularly close to the original one but
> >> at least it increases the chances of getting the expected value somewhat!
> >>
> >> So to me this is a nice useability improvement given the code to implement
> >> it tends not to be too complex.
> >>
> > 
> > Hi David, Jonathan,
> > 
> > What I understand from this is that the osr should be taken into account when writing
> > the sampling frequency as well, right? Here's what I understand:
> > 
> > If the user wants a 125kHz freq with 4 OSR, then when internal osc will be written
> > to 500kHz before single-shot read, buffer preenable/postenable.
> > However, if the user wants a 500kHz frequency with 4 OSR, that would mean a 2MHz
> > Internal osc freq, which is impossible.
> 
> It is up to the user to request something that is legal. They should know this
> from reading the datasheet.
> 
> > 
> > More than this, if the OSR is 32 the maximum effective rate would be 31250, so 25kHz
> > would make it the closes available one. If the user would select 1MHz from the available
> > list it would be weird I would say. So perhaps a solution for this is to display the avail list
> > depending on the set OSR value.
> 
> Yes, the available list should reflect the current state of any other attributes
> that affect it.

IMO, the above makes total sense to me.

- Nuno Sá

> 
> > 
> > Linking the two together is perhaps wrong to begin with from my end, since in this
> > driver's case, the per-channel sampling frequency is controlled by the internal oscillator
> > which has static available values. So perhaps sampling frequency should be separate, and
> > OSR separate as well, which would make everything cleaner.
> > 
> > Indeed, the effective rate is changed by OSR, but perhaps that is something the user
> > should be aware of, since the sampling frequency is the rate at which the channel samples
> > (1 sample per period) and OSR is how many times the channel samples upon a final sample
> > is to be read. The user already has to take this into account when setting the buffer
> > sampling frequency, so it would make sense to take this into account here too.
> 
> We can't change the definition of the IIO ABI just to make one driver simpler
> to implement. The OSR and sample rate can't be completely independent.
> 
> If you want to leave it the way it is currently implemented though, that is fine.
> 
> > 
> > Please let me know you thoughts on this,
> > Radu
>