Re: [PATCH v10 2/6] iio: adc: ad4691: add initial driver for AD4691 family

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sparse register arrays matched using broad GCC ranges cause unaligned multi-byte SPI reads.
- [High] `ad4691_get_sampling_freq()` performs an SPI read without acquiring the required synchronization lock.
- [Medium] SAMP_FREQ is incorrectly placed in info_mask_separate instead of info_mask_shared_by_all.
--

commit c07ff01f28d7750e0cbd4ed3b20b3bab0c7401f0
Author: Radu Sabau <radu.sabau@analog.com>
Subject: iio: adc: ad4691: add initial driver for AD4691 family

This commit adds support for the Analog Devices AD4691 family of high-speed,
low-power multichannel SAR ADCs. It implements a custom regmap layer over raw
SPI to handle the device's mixed 1/2/3/4-byte register widths and uses the
standard IIO read_raw/write_raw interface for single-channel reads.

> IIO_CHAN_INFO_SAMP_FREQ is exposed as info_mask_shared_by_all because
> the AD4691 family has a single internal oscillator whose frequency
> register is shared across all channels. Writing sampling_frequency for
> any one channel necessarily changes the conversion rate for every other
> channel, so the shared annotation correctly reflects the hardware
> behaviour.

The commit message explicitly states that IIO_CHAN_INFO_SAMP_FREQ is exposed
as info_mask_shared_by_all, but the code appears to add it to
info_mask_separate instead:

> diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> new file mode 100644
> index 0000000000000..5b72216bca80e
> --- /dev/null
> +++ b/drivers/iio/adc/ad4691.c
[ ... ]
> +#define AD4691_CHANNEL(ch)						\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)		\
> +				    | BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_separate_available =				\
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.channel = ch,						\

Does this incorrectly generate independent sysfs attributes for each channel?

If so, writing to one channel's attribute might alter the shared internal
oscillator, silently changing the conversion rate for all other channels.

[ ... ]
> +static int ad4691_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
[ ... ]
> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_MASK_REG - 1:
> +	case AD4691_ACC_MASK_REG + 1 ... AD4691_ACC_SAT_OVR_REG(15):
> +		ret = spi_write_then_read(spi, tx, sizeof(tx), rx, 1);
> +		if (ret)
> +			return ret;
> +		*val = rx[0];
> +		return 0;
> +	case AD4691_ACC_MASK_REG:
> +	case AD4691_STD_SEQ_CONFIG:
> +	case AD4691_AVG_IN(0) ... AD4691_AVG_IN(15):
> +		ret = spi_write_then_read(spi, tx, sizeof(tx), rx, 2);

Since AD4691_AVG_IN(n) evaluates to 0x201 + 2*n, the valid base addresses
are spaced by 2. Will the range match inadvertently cover the intermediate
unaligned addresses?

If so, iterating over the regmap (e.g., via the debugfs registers dump) could
cause unaligned multi-byte SPI reads that cross register boundaries.

[ ... ]
> +static bool ad4691_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_SAT_OVR_REG(15):
> +	case AD4691_STD_SEQ_CONFIG:
> +	case AD4691_AVG_IN(0) ... AD4691_AVG_IN(15):
> +	case AD4691_AVG_STS_IN(0) ... AD4691_AVG_STS_IN(15):
> +	case AD4691_ACC_IN(0) ... AD4691_ACC_IN(15):
> +	case AD4691_ACC_STS_DATA(0) ... AD4691_ACC_STS_DATA(15):

A similar question applies here for the sparse register arrays like
AD4691_AVG_STS_IN, AD4691_ACC_IN, and AD4691_ACC_STS_DATA. Could this cause
readable_reg() to return true for invalid unaligned addresses?

[ ... ]
> +static int ad4691_get_sampling_freq(struct ad4691_state *st, int *val)
> +{
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4691_OSC_FREQ_REG, &reg_val);

Is it necessary to acquire st->lock before performing this regmap_read()?

Looking at ad4691_single_shot_read(), it holds the lock during the conversion
sequence, including the fsleep() wait for the conversion to complete. If a
user reads the sampling frequency concurrently, could this SPI transaction
interleave with the conversion sequence?

(Note: I see this is fixed in a later commit that adds oversampling support by
removing the SPI read entirely, but mentioning it here for the current patch).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-ad4692-multichannel-sar-adc-driver-v10-0-e1fbb1744e38@analog.com?part=2