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

[David Lechner <dlechner@baylibre.com>]

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.

> 
> OSR defaults to 1 (no accumulation) for all channels.
> 

...

> @@ -499,7 +570,7 @@ static int ad4691_get_sampling_freq(struct ad4691_state *st, int *val)
>  	if (ret)
>  		return ret;
>  
> -	*val = ad4691_osc_freqs_Hz[FIELD_GET(AD4691_OSC_FREQ_MASK, reg_val)];
> +	*val = ad4691_osc_freqs_Hz[FIELD_GET(AD4691_OSC_FREQ_MASK, reg_val)] / osr;

I guess we don't have to worry about fractional values here?

>  	return IIO_VAL_INT;
>  }
>  
> @@ -536,6 +607,11 @@ static int ad4691_read_avail(struct iio_dev *indio_dev,
>  		*type = IIO_VAL_INT;
>  		*length = ARRAY_SIZE(ad4691_osc_freqs_Hz) - start;
>  		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> +		*vals = ad4691_oversampling_ratios;
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(ad4691_oversampling_ratios);
> +		return IIO_AVAIL_LIST;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -566,6 +642,11 @@ static int ad4691_single_shot_read(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  
> +	ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(chan->channel),
> +			   st->osr[chan->channel]);
> +	if (ret)
> +		return ret;
> +
>  	ret = regmap_read(st->regmap, AD4691_OSC_FREQ_REG, &reg_val);
>  	if (ret)
>  		return ret;
> @@ -575,8 +656,9 @@ static int ad4691_single_shot_read(struct iio_dev *indio_dev,
>  		return ret;
>  
>  	osc_idx = FIELD_GET(AD4691_OSC_FREQ_MASK, reg_val);
> -	/* Wait 2 oscillator periods for the conversion to complete. */
> -	period_us = DIV_ROUND_UP(2UL * USEC_PER_SEC, ad4691_osc_freqs_Hz[osc_idx]);
> +	/* Wait osr oscillator periods for all accumulator samples to complete. */

Why did we need to way 2 before and only 1 now when OSR == 1?

> +	period_us = DIV_ROUND_UP((unsigned long)st->osr[chan->channel] * USEC_PER_SEC,
> +				 ad4691_osc_freqs_Hz[osc_idx]);