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

[Jonathan Cameron]

On Sat, 16 May 2026 12:11:16 -0500
David Lechner <dlechner@xxxxxxxxxxxx> wrote:

> On 5/15/26 8:31 AM, Radu Sabau via B4 Relay wrote:
> > From: Radu Sabau <radu.sabau@xxxxxxxxxx>
> > 
> > Add support for the Analog Devices AD4691 family of high-speed,
> > low-power multichannel SAR ADCs: AD4691 (16-ch, 500 kSPS),
> > AD4692 (16-ch, 1 MSPS), AD4693 (8-ch, 500 kSPS) and
> > AD4694 (8-ch, 1 MSPS).
> > 
> > The driver 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.
> > 
> > The chip idles in Autonomous Mode so that single-shot read_raw can use
> > the internal oscillator without disturbing the hardware configuration.
> > 
> > Three voltage supply domains are managed: avdd (required), vio, and a
> > reference supply on either the REF pin (ref-supply, external buffer)
> > or the REFIN pin (refin-supply, uses the on-chip reference buffer;
> > REFBUF_EN is set accordingly). Hardware reset is performed via
> > the reset controller framework; a software reset through SPI_CONFIG_A
> > is used as fallback when no hardware reset is available.
> > 
> > Accumulator channel masking for single-shot reads uses ACC_MASK_REG via
> > an ADDR_DESCENDING SPI write, which covers both mask bytes in a single
> > 16-bit transfer.
> > 
> > IIO_CHAN_INFO_SAMP_FREQ is exposed as info_mask_separate. The oscillator
> > is shared hardware — writing any channel's sampling_frequency attribute
> > sets it for all others — but per-channel attributes are used throughout
> > the series to avoid an ABI change when per-channel oversampling ratios
> > are introduced in a later commit, at which point the effective output
> > rate (osc_freq / osr[N]) becomes genuinely per-channel.
> > 
> > Reviewed-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
One follow up as I was commenting on same code...

> > diff --git a/drivers/iio/adc/ad4691.c b/drivers/iio/adc/ad4691.c
> > new file mode 100644
> > index 000000000000..ba77e1bfef16
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad4691.c

> > +static int ad4691_get_sampling_freq(struct ad4691_state *st, int *val)
> > +{
> > +	unsigned int reg_val;
> > +	int ret;
> > +  
> 
> No mutex lock here? Maybe without OK since it is a read.

Agreed. It's not a bug, but also not a fast path and it will save reasoning
and need for comment to just take the lock.

> 
> > +	/*
> > +	 * AD4691_OSC_FREQ_REG is non-volatile and written during
> > +	 * ad4691_config(), so regmap returns the cached value here without
> > +	 * touching the SPI bus. No lock is needed.
> > +	 */
> > +	ret = regmap_read(st->regmap, AD4691_OSC_FREQ_REG, &reg_val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*val = ad4691_osc_freqs_Hz[FIELD_GET(AD4691_OSC_FREQ_MASK, reg_val)];
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int ad4691_set_sampling_freq(struct iio_dev *indio_dev, int freq)
> > +{
> > +	struct ad4691_state *st = iio_priv(indio_dev);
> > +	unsigned int start = ad4691_samp_freq_start(st->info);
> > +
> > +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> > +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> > +		return -EBUSY;
> > +
> > +	for (unsigned int i = start; i < ARRAY_SIZE(ad4691_osc_freqs_Hz); i++) {
> > +		if (ad4691_osc_freqs_Hz[i] != freq)
> > +			continue;  
> 
> mutex lock?
Agreed. Whilst the direct mode acquire will serialize that's an internal implementation
detail.  Where a driver needs to ensure some sequences are not interrupted
(like I think for the single short read?) then it should take the local
lock.


> 
> > +		return regmap_update_bits(st->regmap, AD4691_OSC_FREQ_REG,
> > +					  AD4691_OSC_FREQ_MASK, i);
> > +	}
> > +
> > +	return -EINVAL;
> > +}