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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The oversampling ratio (OSR) is only applied to channel 0, leaving all other channels unconfigured.
- [Medium] Decreasing the oversampling ratio can incorrectly fail with -EINVAL, effectively trapping the device in a high OSR state.
- [Medium] Mismatch between the commit message intent (per-channel OSR) and the actual implementation (device-wide shared OSR).
--

commit cb227edc48341e4c564eb44a5dcf61a6d76e273a
Author: Radu Sabau <radu.sabau@analog.com>

iio: adc: ad4691: add oversampling support
    
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.

>     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.
>     
>     in_voltageN_sampling_frequency represents the effective output rate for
>     channel N, defined as osc_freq / osr[N]. The chip has one internal
>     oscillator shared by all channels; each channel independently
>     accumulates osr[N] oscillator cycles before producing a result.

[Severity: Medium]
Is the intent to support per-channel oversampling ratios as described here?

The commit message refers to osr[N], but the implementation introduces a
single global state variable st->osr, exposes the sysfs attributes as
info_mask_shared_by_all in the channel macros, and applies this shared
variable device-wide.

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> index 6563488ab8b89..64238f8428417 100644
> --- a/drivers/iio/adc/ad4691.c
> +++ b/drivers/iio/adc/ad4691.c
[ ... ]
> @@ -642,7 +856,7 @@ static int ad4691_single_shot_read(struct iio_dev *indio_dev,
>  				   struct iio_chan_spec const *chan, int *val)
>  {
>  	struct ad4691_state *st = iio_priv(indio_dev);
> -	unsigned int reg_val, osc_idx, period_us;
> +	unsigned int reg_val, period_us;
>  	int ret;
>  
>  	guard(mutex)(&st->lock);
> @@ -662,7 +876,11 @@ static int ad4691_single_shot_read(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  
> -	ret = regmap_read(st->regmap, AD4691_OSC_FREQ_REG, &reg_val);
> +	ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(0), st->osr);

[Severity: High]
Does this apply the oversampling ratio only to channel 0?

Since AD4691_ACC_DEPTH_IN(n) targets a distinct register address per channel,
hardcoding 0 here means for single-shot reads on channels other than 0,
the OSR is never configured, yet the driver waits the full oversampled
duration and expects accumulated data.

> +	if (ret)
> +		return ret;
> +
> +	ret = ad4691_write_osc_freq(st);
>  	if (ret)
>  		return ret;
[ ... ]
> @@ -730,6 +965,33 @@ static int ad4691_write_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		return ad4691_set_sampling_freq(st, val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {
> +		unsigned int old_effective, found, osr_idx;
> +
> +		osr_idx = ad4691_osr_index(val);
> +		if (ad4691_oversampling_ratios[osr_idx] != val)
> +			return -EINVAL;
> +
> +		/*
> +		 * Hold st->lock while computing the new oscillator frequency
> +		 * and updating both target_osc_freq_Hz and osr atomically:
> +		 * read_raw for SAMP_FREQ reads both fields under the lock and
> +		 * must see a consistent pair (new osc ↔ new osr).
> +		 *
> +		 * Snap target_osc_freq_Hz to the largest table entry that is
> +		 * both <= old_effective * new_osr and evenly divisible by
> +		 * new_osr, preserving an integer read-back of
> +		 * in_voltage_sampling_frequency after the OSR change.
> +		 */
> +		guard(mutex)(&st->lock);
> +		old_effective = st->target_osc_freq_Hz / st->osr;
> +		found = ad4691_find_osc_freq(st, old_effective * (unsigned int)val, val);
> +		if (!found)
> +			return -EINVAL;

[Severity: Medium]
Could this reject valid oversampling ratio changes with -EINVAL, effectively
trapping the device in a high OSR state?

If the device is currently at a high OSR (e.g., 32), the old_effective rate
is very low. Attempting to decrease the OSR directly to 1 yields a low
needed oscillator frequency (old_effective * new_osr).

Since ad4691_find_osc_freq() only returns frequencies less than or equal to
this needed oscillator rate, and the minimum supported frequency is
25,000 Hz, the search will fail and return 0 if the needed rate is less
than 25,000. This would force users to use a non-intuitive step-down
workaround to lower the OSR.

> +		st->target_osc_freq_Hz = found;
> +		st->osr = val;
> +		return 0;
> +	}
>  	default:
>  		return -EINVAL;
>  	}
[ ... ]
> @@ -943,6 +1209,10 @@ static int ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unoptimize;
>  
> +	ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(0), st->osr);

[Severity: High]
Does this hardcoded channel 0 write leave the remaining active channels
unconfigured?

Since this doesn't iterate over the active scan mask, channels 1-15 will
always operate with the default OSR of 1 during buffered reads, causing
incorrect hardware accumulation.

> +	if (ret)
> +		goto err_unoptimize;
> +
>  	ret = ad4691_enter_conversion_mode(st);
>  	if (ret)
>  		goto err_unoptimize;
[ ... ]
> @@ -1122,6 +1392,10 @@ static int ad4691_cnv_burst_offload_buffer_postenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		return ret;
>  
> +	ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(0), st->osr);

[Severity: High]
As with ad4691_cnv_burst_buffer_preenable() above, this also hardcodes
channel 0. Should this iterate over the active scan mask instead?

> +	if (ret)
> +		return ret;
> +
>  	ret = ad4691_enter_conversion_mode(st);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-ad4692-multichannel-sar-adc-driver-v13-0-1b7626d3b35c@analog.com?part=5