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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 09 Apr 2026 18:28:23 +0300
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> 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.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>

Mostly minor stuff but the regulator bit needs another look.

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

> +static int ad4691_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct spi_device *spi = context;
> +	u8 tx[2], rx[4];
> +	int ret;
> +
> +	/* Set bit 15 to mark the operation as READ. */
> +	put_unaligned_be16(0x8000 | reg, tx);
> +
> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_SAT_OVR_REG(15):
> +		ret = spi_write_then_read(spi, tx, 2, rx, 1);

for tx size can use sizeof(tx)

> +		if (ret)
> +			return ret;
> +		*val = rx[0];
> +		return 0;
> +	case AD4691_STD_SEQ_CONFIG:
> +	case AD4691_AVG_IN(0) ... AD4691_AVG_IN(15):
> +		ret = spi_write_then_read(spi, tx, 2, rx, 2);
> +		if (ret)
> +			return ret;
> +		*val = get_unaligned_be16(rx);
> +		return 0;
> +	case AD4691_AVG_STS_IN(0) ... AD4691_AVG_STS_IN(15):
> +	case AD4691_ACC_IN(0) ... AD4691_ACC_IN(15):
> +		ret = spi_write_then_read(spi, tx, 2, rx, 3);
> +		if (ret)
> +			return ret;
> +		*val = get_unaligned_be24(rx);
> +		return 0;
> +	case AD4691_ACC_STS_DATA(0) ... AD4691_ACC_STS_DATA(15):
> +		ret = spi_write_then_read(spi, tx, 2, rx, 4);
> +		if (ret)
> +			return ret;
> +		*val = get_unaligned_be32(rx);
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4691_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct spi_device *spi = context;
> +	u8 tx[4];
> +
> +	put_unaligned_be16(reg, tx);
> +
> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_MASK_REG - 1:
> +	case AD4691_ACC_MASK_REG + 1 ... AD4691_GPIO_MODE2_REG:
> +		if (val > 0xFF)

U8_MAX from limits.h

> +			return -EINVAL;
> +		tx[2] = val;
> +		return spi_write_then_read(spi, tx, 3, NULL, 0);
> +	case AD4691_ACC_MASK_REG:
> +	case AD4691_STD_SEQ_CONFIG:
> +		if (val > 0xFFFF)

U16_MAX
 
> +			return -EINVAL;
> +		put_unaligned_be16(val, &tx[2]);
> +		return spi_write_then_read(spi, tx, 4, NULL, 0);
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +static int ad4691_set_sampling_freq(struct iio_dev *indio_dev, int freq)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int start = (st->info->max_rate == 1 * HZ_PER_MHZ) ? 0 : 1;

This appears in a couple of places. Maybe a little helper for
ad4691_samp_freq_array_start() would let you describe why in a comment
in a single location.

> +
> +	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;
> +		return regmap_update_bits(st->regmap, AD4691_OSC_FREQ_REG,
> +					  AD4691_OSC_FREQ_MASK, i);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ad4691_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type,
> +			     int *length, long mask)
> +{
> +	struct ad4691_state *st = iio_priv(indio_dev);
> +	unsigned int start = (st->info->max_rate == 1 * HZ_PER_MHZ) ? 0 : 1;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = &ad4691_osc_freqs_Hz[start];
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(ad4691_osc_freqs_Hz) - start;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}


> +
> +static int ad4691_regulator_setup(struct ad4691_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	int ret;
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad4691_supplies),
> +					     ad4691_supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get and enable supplies\n");
> +
> +	ret = devm_regulator_get_enable(dev, "ldo-in");
> +	if (ret == -ENODEV)
> +		st->ldo_en = true;

This seems odd.  ldo_en to me implies enabling the internal LDO, which I'd expect
to need the ldo-in supply.  Anyhow - that oddity made me dig...

The datasheet is rather confusingly worded but the diagrams seem clear.

I 'think' the only time the ldo should not be enabled is when VDD = 1.8V and
LDO_IN is then tied to 0 (which maybe we represent at no ldo-in regulator?)

When ldo-in is supplied and the LDO enabled, the LDO output is tied to the VDD pin
internally.  So you'd should not be feeding VDD in that case.  There is a note
that says that it'll survive being also powered to 1.8V or grounded, but seems
to strongly advise against doing that.

Anyhow, what you have here and the dt-binding don't seem to align with the datasheet.

Figure 54 shows this most clearly. Your dt binding doesn't include vdd which
is also not aligning with the ad4691 datasheet on the website.

> +	else if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get and enable LDO-IN\n");
> +
> +	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (st->vref_uV == -ENODEV) {
> +		st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "refin");
> +		st->refbuf_en = true;
> +	}
> +	if (st->vref_uV < 0)
> +		return dev_err_probe(dev, st->vref_uV,
> +				     "Failed to get reference supply\n");
> +
> +	if (st->vref_uV < AD4691_VREF_uV_MIN || st->vref_uV > AD4691_VREF_uV_MAX)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "vref(%d) must be in the range [%u...%u]\n",
> +				     st->vref_uV, AD4691_VREF_uV_MIN,
> +				     AD4691_VREF_uV_MAX);
> +
> +	return 0;
> +}
> +
> +static int ad4691_reset(struct ad4691_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct reset_control *rst;
> +
> +	rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset\n");
> +
> +	if (rst) {
> +		/*
> +		 * The GPIO is already asserted by reset_gpio_probe().

I thought assumption was that firmware (or driver going down) should always
leave the device in reset and hence could safely do 

devm_reset_control_get_optional_exclusive_deasserted() without
the need for the dance with asserting / sleep deasserting.

More than possible I've misunderstood this bit (or we have known firmware
out there that doesn't leave this in reset - in which case good to document
that here).

I don't mind the explicit code you have, just want to understand the reasoning
a little more.

FWIW, if anyone fancies taking a look, a few of the IIO drivers could
do to use the various 'extended' devm calls rather than doing their own
handling off assert and deassert via devm_add_action_or_rest()

> +		 * Wait for the reset pulse width required by the chip.
> +		 * See datasheet Table 5.
> +		 */
> +		fsleep(300);
> +		return reset_control_deassert(rst);
> +	}
> +
> +	/* No hardware reset available, fall back to software reset. */
> +	return regmap_write(st->regmap, AD4691_SPI_CONFIG_A_REG,
> +			    AD4691_SW_RESET);
> +}
> +
> +static int ad4691_config(struct ad4691_state *st)
> +{

...

> +	val = FIELD_PREP(AD4691_REF_CTRL_MASK, ref_val);
> +	if (st->refbuf_en)
> +		val |= AD4691_REFBUF_EN;
> +
> +	ret = regmap_update_bits(st->regmap, AD4691_REF_CTRL,
> +				 AD4691_REF_CTRL_MASK | AD4691_REFBUF_EN, val);

It doesn't hugely matter but given (at least for the AD4691) the other bits
are reserved and reset to 0 you could just write the whole thing and avoid
a bus read.  Doing it like this kind of implies there is something to
preserve. I was curious what hence looked it up.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write REF_CTRL\n");
> +
> +	ret = regmap_assign_bits(st->regmap, AD4691_DEVICE_SETUP,
> +				 AD4691_LDO_EN, st->ldo_en);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write DEVICE_SETUP\n");
> +
> +	/*
> +	 * Set the internal oscillator to the highest rate this chip supports.
> +	 * Index 0 (1 MHz) exceeds the 500 kHz max of AD4691/AD4693, so those
> +	 * chips start at index 1 (500 kHz).
> +	 */
> +	ret = regmap_assign_bits(st->regmap, AD4691_OSC_FREQ_REG,
> +				 AD4691_OSC_FREQ_MASK,

Similar to above. You could simplify to a write only given rest are reserved 0 bits.

Maybe it's simpler to keep them all the same though. Up to you.

> +				 (st->info->max_rate == 1 * HZ_PER_MHZ) ? 0 : 1);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write OSC_FREQ\n");
> +
> +	ret = regmap_update_bits(st->regmap, AD4691_ADC_SETUP,
> +				 AD4691_ADC_MODE_MASK, AD4691_AUTONOMOUS_MODE);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write ADC_SETUP\n");
> +
> +	return 0;
> +}
> +
> +static int ad4691_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4691_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));

	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));

> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->info = spi_get_device_match_data(spi);
> +
> +	ret = devm_mutex_init(dev, &st->lock);
...

thanks

Jonathan