RE: [PATCH v9 5/6] iio: adc: ad4691: add oversampling support

[Sabau, Radu bogdan]

Addressing Sashiko's review for the oversampling support patch.

> -----Original Message-----
> From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@xxxxxxxxxx>
> Sent: Thursday, April 30, 2026 1:17 PM

...

> +/* CNV burst mode channel — exposes oversampling ratio. */
>  #define AD4691_CHANNEL(ch)
> 	\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 	\
> +
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) | \
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_separate_available =
> 	\
> +
> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO) | \
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\

"The IIO_CHAN_INFO_SAMP_FREQ attribute is exported with info_mask_separate,
implying per-channel independence. However, writing to sampling_frequency
updates st->target_osc_freq_Hz, a global variable shared by all channels.
Does modifying this global variable on one channel silently alter the
effective sampling rate of all other channels? Should this use
info_mask_shared_by_all to comply with the IIO ABI?"

info_mask_separate is intentional here. The oscillator is shared, but
the effective output rate is genuinely per-channel: osc_freq / osr[N].
Writing in_voltageN_sampling_frequency sets the oscillator (which changes
the read-back for all channels), but reading it back for any channel always
returns osc_freq / osr[N] — a value specific to that channel's OSR. The
oscillator snaps to the largest table entry <= freq * osr[N] that is evenly
divisible by osr[N], so the read-back is always an integer for the channel
that drove the write. The same trade-off is present in ad4695.
info_mask_separate is the correct annotation for this per-channel view.

> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),
> 	\
> +		.channel = ch,						\
> +		.scan_index = ch,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\
> +			.endianness = IIO_BE,				\
> +		},							\
> +	}
> +

...

>  static int ad4691_read_avail(struct iio_dev *indio_dev,
> @@ -540,10 +655,30 @@ static int ad4691_read_avail(struct iio_dev
> *indio_dev,
>  	unsigned int start = ad4691_samp_freq_start(st->info);
> 
>  	switch (mask) {
> -	case IIO_CHAN_INFO_SAMP_FREQ:
> -		*vals = &ad4691_osc_freqs_Hz[start];
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int osr = st->osr[chan->channel];
> +		int n = 0;
> +
> +		/*
> +		 * Only oscillator frequencies evenly divisible by the channel's
> +		 * OSR yield an integer effective rate; expose those as effective
> +		 * rates (osc / osr) so the user works entirely in output-sample
> +		 * space.
> +		 */
> +		for (unsigned int i = start; i <
> ARRAY_SIZE(ad4691_osc_freqs_Hz); i++) {
> +			if (ad4691_osc_freqs_Hz[i] % osr != 0)
> +				continue;
> +			st->samp_freq_avail[n++] = ad4691_osc_freqs_Hz[i] /
> osr;
> +		}
> +		*vals = st->samp_freq_avail;

"The IIO core calls read_avail() without holding driver-level locks that
serialize sysfs reads. Can concurrent reads of
in_voltageN_sampling_frequency_available for channels with different OSR
values race and overwrite st->samp_freq_avail simultaneously?
This could result in interleaved or corrupted output being formatted by the
IIO core and sent to userspace. Is a per-channel array or synchronization
required here?"

Valid. Sysfs serializes reads to the same sysfs file but not across
different channel _available files. Concurrent reads of
in_voltage0_sampling_frequency_available and
in_voltage1_sampling_frequency_available (with different OSRs) raced on
the shared samp_freq_avail[] scratch array. Made it a 2D array
[16][N_OSC_ENTRIES] so each channel has its own sub-array; concurrent
reads for different channels no longer interfere.

>  		*type = IIO_VAL_INT;
> -		*length = ARRAY_SIZE(ad4691_osc_freqs_Hz) - start;
> +		*length = n;
> +		return IIO_AVAIL_LIST;

...

> @@ -634,9 +780,29 @@ static int ad4691_write_raw(struct iio_dev
> *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int val, int val2, long mask)
>  {
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		return ad4691_set_sampling_freq(indio_dev, val);
> +		return ad4691_set_sampling_freq(indio_dev, chan, val);
> +	case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {
> +		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +		if (IIO_DEV_ACQUIRE_FAILED(claim))
> +			return -EBUSY;
> +
> +		for (unsigned int i = 0; i <
> ARRAY_SIZE(ad4691_oversampling_ratios); i++) {
> +			if (ad4691_oversampling_ratios[i] != val)
> +				continue;
> +			/*
> +			 * Store the new OSR; target_osc_freq_Hz is
> unchanged.
> +			 * The effective rate read back via
> in_voltageN_sampling_frequency
> +			 * becomes target_osc_freq_Hz / new_osr
> automatically.
> +			 */
> +			st->osr[chan->channel] = val;

"Writing oversampling_ratio updates osr but leaves target_osc_freq_Hz
unchanged. Does this break the target_osc_freq_Hz % osr == 0 guarantee
enforced in ad4691_set_sampling_freq()?
Reading sampling_frequency after this could yield a truncated integer. If
that exact integer is written back, it might compute a smaller oscillator
requirement and snap the device to a different, lower oscillator frequency.
Does modifying oversampling_ratio need to recalculate target_osc_freq_Hz
to maintain state consistency?"

Valid. Storing a new OSR without updating target_osc_freq_Hz can
break the target_osc_freq_Hz % osr == 0 invariant. For example with
target_osc_freq_Hz = 250 kHz and new OSR = 32: read-back is
250000 / 32 = 7812 (truncated); writing that value back computes
needed_osc = 249984, snapping the oscillator to the next lower table
entry — a silent frequency change on what the user expected to be a
no-op write-back.

Fixed: the OVERSAMPLING_RATIO write path now calls ad4691_find_osc_freq()
with old_effective * new_osr as the ceiling, where
old_effective = target_osc_freq_Hz / old_osr. This preserves the
divisibility invariant and keeps the oscillator as close as possible to
the previous effective rate.

> +			return 0;
> +		}
> +		return -EINVAL;
> +	}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -691,6 +857,10 @@ static int ad4691_enter_conversion_mode(struct
> ad4691_state *st)
>  		return regmap_update_bits(st->regmap,
> AD4691_DEVICE_SETUP,
>  					  AD4691_MANUAL_MODE,
> AD4691_MANUAL_MODE);
> 
> +	ret = ad4691_write_osc_freq(st);
> +	if (ret)
> +		return ret;
> +
>  	ret = regmap_update_bits(st->regmap, AD4691_ADC_SETUP,
>  				 AD4691_ADC_MODE_MASK,
> AD4691_CNV_BURST_MODE);
>  	if (ret)
> @@ -844,6 +1014,12 @@ static int
> ad4691_cnv_burst_buffer_preenable(struct iio_dev *indio_dev)
>  	if (ret)
>  		goto err_unoptimize;
> 
> +	iio_for_each_active_channel(indio_dev, i) {
> +		ret = regmap_write(st->regmap, AD4691_ACC_DEPTH_IN(i),
> st->osr[i]);
> +		if (ret)
> +			goto err_unoptimize;
> +	}
> +

"If the IIO soft timestamp channel is enabled, its scan_index is 16.
iio_for_each_active_channel() iterates over all active channels including
the timestamp channel.
Will this loop process i = 16 and access st->osr[16] out of bounds, since
st->osr is an array of size 16?
Additionally, since AD4691_ACC_DEPTH_IN(16) resolves to 0x196
(AD4691_GPIO_MODE1_REG), will this silently corrupt the GPIO configuration
and potentially break the DATA_READY interrupt?
Furthermore, does the presence of the timestamp channel cause the loop
iteration to advance k to 17, making the state-reset SPI transfers appended
after the loop write to st->scan_xfers[34] and [35]? This would overflow
the scan_xfers array which is sized 34 and could corrupt the surrounding
struct ad4691_state."

Valid for the osr[] out-of-bounds and GPIO corruption concerns. With
the soft timestamp enabled (scan_index = 16), the loop would access
st->osr[16] out-of-bounds and write to AD4691_ACC_DEPTH_IN(16) = 0x196,
which is AD4691_GPIO_MODE1_REG, silently corrupting the GPIO configuration
and potentially breaking the DATA_READY interrupt.

Added the same guard used by the scan_xfers loops in the triggered-buffer
commit: if (i >= indio_dev->num_channels - 1) break.

The scan_xfers k-overflow concern is already handled by that existing guard
in a separate loop — it is not affected by the ACC_DEPTH_IN loop added
here.