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

[Andy Shevchenko]

On Fri, Mar 27, 2026 at 01:07:58PM +0200, Radu Sabau via B4 Relay wrote:

> 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.

...

+ array_size.h

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>

Hmm... Is it used? Or perhaps you need only
dev_printk.h
device/devres.h
?

> +#include <linux/err.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +#include <linux/unaligned.h>

...

> +/*
> + * Internal oscillator frequency table. Index is the OSC_FREQ_REG[3:0] value.
> + * Index 0 (1 MHz) is only valid for AD4692/AD4694; AD4691/AD4693 support
> + * up to 500 kHz and use index 1 as their highest valid rate.
> + */
> +static const int ad4691_osc_freqs[] = {
> +	1000000,	/* 0x0: 1 MHz */
> +	500000,		/* 0x1: 500 kHz */
> +	400000,		/* 0x2: 400 kHz */
> +	250000,		/* 0x3: 250 kHz */
> +	200000,		/* 0x4: 200 kHz */
> +	167000,		/* 0x5: 167 kHz */
> +	133000,		/* 0x6: 133 kHz */
> +	125000,		/* 0x7: 125 kHz */
> +	100000,		/* 0x8: 100 kHz */
> +	50000,		/* 0x9: 50 kHz */
> +	25000,		/* 0xA: 25 kHz */
> +	12500,		/* 0xB: 12.5 kHz */
> +	10000,		/* 0xC: 10 kHz */
> +	5000,		/* 0xD: 5 kHz */
> +	2500,		/* 0xE: 2.5 kHz */
> +	1250,		/* 0xF: 1.25 kHz */

Instead of comments, make the code self-commented and robust:

/* ...the top comment... */
static const int ad4691_osc_freqs_Hz[] = {
	...
	[0xD] = 5000,
	[0xE] = 2500,
	[0xF] = 1250,
};

I would even use unit multipliers in some cases, but it might make the whole
table inconsistent, dunno.

> +};

>From this it will be visible that the table is in Hz and each value is properly
indexed, even shuffling won't break the code.

...

> +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;
> +	unsigned int i;
> +
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;

> +	for (i = start; i < ARRAY_SIZE(ad4691_osc_freqs); i++) {

	for (unsigned int i = start; i < ARRAY_SIZE(ad4691_osc_freqs); i++) {

> +		if (ad4691_osc_freqs[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;

Yeah, in the table it's written as 1000000... But as I mentioned above, using
unit multipliers _there_ maybe not a good idea.

> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = &ad4691_osc_freqs[start];
> +		*type = IIO_VAL_INT;
> +		*length = ARRAY_SIZE(ad4691_osc_freqs) - start;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +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;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);

> +	/*
> +	 * Use AUTONOMOUS mode for single-shot reads.
> +	 */

One line?

> +	ret = regmap_write(st->regmap, AD4691_STATE_RESET_REG,
> +			   AD4691_STATE_RESET_ALL);
> +	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,
> +			   (u16)~BIT(chan->channel));

Why do you need casting?

> +	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;
> +
> +	/*
> +	 * Wait for at least 2 internal oscillator periods for the
> +	 * conversion to complete.
> +	 */
> +	fsleep(DIV_ROUND_UP(2 * USEC_PER_SEC, ad4691_osc_freqs[FIELD_GET(AD4691_OSC_FREQ_MASK, reg_val)]));

Way too long line. Use temporary variables for that to make it easier to parse.
Also add a (short) comment on how the OSC periods are being calculated.

> +	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);
> +	if (ret)
> +		return ret;
> +
> +	return IIO_VAL_INT;
> +}

...

> +	ret = devm_regulator_get_enable(dev, "avdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get and enable AVDD\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vio");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get and enable VIO\n");

Can they be united to a bulk?

...

> +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 (GPIOD_OUT_HIGH).
> +		 * Wait for the reset pulse width required by the chip. See datasheet Table 5.

Too long, try to wrap around 80, and drop unneeded (confusing?) details.

> +		 */

		/*
		 * The GPIO is already asserted by reset_gpio_probe().
		 * 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);
> +}

...

> +	ret = regmap_update_bits(st->regmap, AD4691_REF_CTRL,
> +				 AD4691_REF_CTRL_MASK | AD4691_REFBUF_EN,
> +				 FIELD_PREP(AD4691_REF_CTRL_MASK, ref_val) |
> +				 (st->refbuf_en ? AD4691_REFBUF_EN : 0));

With temporary variable it will become something like

	/* ...Comment on what is this... */
	val = FIELD_PREP(...);
	FIELD_MODIFY(...);

	ret = regmap_update_bits(st->regmap, AD4691_REF_CTRL,
				 AD4691_REF_CTRL_MASK | AD4691_REFBUF_EN, val);


> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write REF_CTRL\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_update_bits(st->regmap, AD4691_OSC_FREQ_REG,
> +				 AD4691_OSC_FREQ_MASK,
> +				 (st->info->max_rate == 1 * HZ_PER_MHZ) ? 0 : 1);

_assign_bits?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write OSC_FREQ\n");

-- 
With Best Regards,
Andy Shevchenko