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

[Jonathan Cameron]

On Fri, 15 May 2026 16:31:31 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@xxxxxxxxxx> 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>
Just a couple of really trivial comments.

I might just have tweaked some of these whilst applying but seeing
as it seems you'll be doing a v12 - over to you :)

Jonathan
 

> 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

> +
> +#define AD4691_OSC_EN_REG			0x180
> +#define AD4691_STATE_RESET_REG			0x181
> +#define AD4691_STATE_RESET_ALL			0x01
Is this a value in a field or documented as whole register value?
I checked, it's  BIT(0) - rest of register is Reserved 0
Hence cleaner to use BIT(0) for this definition.

> +#define AD4691_ADC_SETUP			0x182
> +#define AD4691_ADC_MODE_MASK			GENMASK(1, 0)
> +#define AD4691_AUTONOMOUS_MODE			0x02
Personal preference would have been to fully define the field values
at this stage, but meh, you bring in the other used ones in later patches
so not critical.

> +
> +#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,						\
> +		.scan_index = ch,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\

Really trivial but I suspect that scan_index and most of scan_type aren't used
until the buffered support is added. So in ideal world add that extra stuff
in that patch.  .realbits is used here so fine to initialize that now.

> +		},							\
> +	}

> +
> +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;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* Use AUTONOMOUS mode for single-shot reads. */
> +	ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> +			   AD4691_STATE_RESET_ALL);

This identical to line below that you don't wrap. I don't mind which
but consistency is good.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_STD_SEQ_CONFIG,
> +			   BIT(chan->channel));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK_REG,
> +			   ~BIT(chan->channel) & GENMASK(15, 0));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD4691_OSC_FREQ_REG, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_OSC_EN_REG, 1);
> +	if (ret)
> +		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]);
> +	fsleep(period_us);
> +
> +	ret = regmap_write(st->regmap, AD4691_OSC_EN_REG, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD4691_AVG_IN(chan->channel), &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	*val = reg_val;
> +
> +	ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG, AD4691_STATE_RESET_ALL);
See above.

> +	if (ret)
> +		return ret;
> +
> +	return IIO_VAL_INT;
> +}


> +
> +static const struct of_device_id ad4691_of_match[] = {
> +	{ .compatible = "adi,ad4691", .data = &ad4691_chip_info },
> +	{ .compatible = "adi,ad4692", .data = &ad4692_chip_info },
> +	{ .compatible = "adi,ad4693", .data = &ad4693_chip_info },
> +	{ .compatible = "adi,ad4694", .data = &ad4694_chip_info },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad4691_of_match);
> +
> +static const struct spi_device_id ad4691_id[] = {
> +	{ "ad4691", (kernel_ulong_t)&ad4691_chip_info },
> +	{ "ad4692", (kernel_ulong_t)&ad4692_chip_info },
> +	{ "ad4693", (kernel_ulong_t)&ad4693_chip_info },
> +	{ "ad4694", (kernel_ulong_t)&ad4694_chip_info },

New thing to IIO, but as you are doing another spin anyway can you save
us a future patch.  Seems likely that similar to I2C that Uwe is
currently working on, we will eventually move away from stashing pointers
in that ulong_t.  So please use named initializers.  I'm not sure
why they've been standard for years for of_device_id but not spi_device_id!

Jonathan

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ad4691_id);