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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Mar 10, 2026 at 04:32:23PM +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.
> 
> Two buffered operating modes are supported, auto-detected from the
> device tree:
> 
>     - CNV Clock Mode:  an external PWM drives the CNV pin; the sampling
>                        rate is controlled via the PWM period. Requires a
>                        reference clock and a DATA_READY interrupt.
> 
>     - Manual Mode:     CNV is tied to SPI CS; each SPI transfer triggers
>                        a conversion and returns the previous result
>                        (pipelined). No external clock or interrupt needed.
> 
> In both modes the chip idles in Autonomous Mode so that single-shot
> read_raw can use the internal oscillator without disturbing the
> hardware configuration.

...

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>

> +#include <linux/kernel.h>

Just no.

> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/util_macros.h>
> +#include <linux/units.h>
> +#include <linux/unaligned.h>

...

> +#define AD4691_VREF_MIN				2400000
> +#define AD4691_VREF_MAX				5250000

What are the units? _uV ?

#define AD4691_VREF_uV_MIN				2400000
#define AD4691_VREF_uV_MAX				5250000

...

> +#define AD4691_NOOP				0x00
> +#define AD4691_ADC_CHAN(ch)			((0x10 + (ch)) << 3)
> +
> +#define AD4691_STATUS_REG			0x14
> +#define AD4691_CLAMP_STATUS1_REG		0x1A
> +#define AD4691_CLAMP_STATUS2_REG		0x1B
> +#define AD4691_DEVICE_SETUP			0x20
> +#define AD4691_REF_CTRL				0x21
> +#define AD4691_OSC_FREQ_REG			0x23
> +#define AD4691_STD_SEQ_CONFIG			0x25
> +#define AD4691_SPARE_CONTROL			0x2A
> +
> +#define AD4691_OSC_EN_REG			0x180

Make all register offsets equal in width, id est the above will be 0x023
and so on.

> +#define AD4691_STATE_RESET_REG			0x181
> +#define AD4691_ADC_SETUP			0x182
> +#define AD4691_ACC_MASK1_REG			0x184
> +#define AD4691_ACC_MASK2_REG			0x185
> +#define AD4691_ACC_COUNT_LIMIT(n)		(0x186 + (n))
> +#define AD4691_ACC_COUNT_VAL			0x3F
> +#define AD4691_GPIO_MODE1_REG			0x196
> +#define AD4691_GPIO_MODE2_REG			0x197
> +#define AD4691_GPIO_READ			0x1A0
> +#define AD4691_ACC_STATUS_FULL1_REG		0x1B0
> +#define AD4691_ACC_STATUS_FULL2_REG		0x1B1
> +#define AD4691_ACC_STATUS_OVERRUN1_REG		0x1B2
> +#define AD4691_ACC_STATUS_OVERRUN2_REG		0x1B3
> +#define AD4691_ACC_STATUS_SAT1_REG		0x1B4
> +#define AD4691_ACC_STATUS_SAT2_REG		0x1BE
> +#define AD4691_ACC_SAT_OVR_REG(n)		(0x1C0 + (n))
> +#define AD4691_AVG_IN(n)			(0x201 + (2 * (n)))
> +#define AD4691_AVG_STS_IN(n)			(0x222 + (3 * (n)))
> +#define AD4691_ACC_IN(n)			(0x252 + (3 * (n)))
> +#define AD4691_ACC_STS_DATA(n)			(0x283 + (4 * (n)))

...

> +enum ad4691_ref_ctrl {
> +	AD4691_VREF_2P5 = 0,
> +	AD4691_VREF_3P0,
> +	AD4691_VREF_3P3,
> +	AD4691_VREF_4P096,
> +	AD4691_VREF_5P0,

Is it HW related? Make sure you have _all_ of that being assigned explicitly.
Otherwise drop the standard assignment (if it's Linux enum). Ditto rule of
thumb to *all* enums in the code here and in the future.

This can be written on your internal Wiki page for the internal review rounds.

> +};

...

> +	int num_channels;
> +	int max_rate;

Why are they signed?

...

> +static const struct ad4691_chip_info ad4691_ad4691 = {
> +	.channels = ad4691_channels,
> +	.manual_channels = ad4691_manual_channels,
> +	.name = "ad4691",
> +	.num_channels = ARRAY_SIZE(ad4691_channels),
> +	.max_rate = 500000,

500 * HZ_PER_KHZ

> +};
> +
> +static const struct ad4691_chip_info ad4691_ad4692 = {
> +	.channels = ad4691_channels,
> +	.manual_channels = ad4691_manual_channels,
> +	.name = "ad4692",
> +	.num_channels = ARRAY_SIZE(ad4691_channels),
> +	.max_rate = 1000000,

1 * HZ_PER_MHZ

> +};
> +
> +static const struct ad4691_chip_info ad4691_ad4693 = {
> +	.channels = ad4693_channels,
> +	.manual_channels = ad4693_manual_channels,
> +	.name = "ad4693",
> +	.num_channels = ARRAY_SIZE(ad4693_channels),
> +	.max_rate = 500000,

...and so on...

> +};
> +
> +static const struct ad4691_chip_info ad4691_ad4694 = {
> +	.channels = ad4693_channels,
> +	.manual_channels = ad4693_manual_channels,
> +	.name = "ad4694",
> +	.num_channels = ARRAY_SIZE(ad4693_channels),
> +	.max_rate = 1000000,
> +};

...

> +struct ad4691_state {
> +	const struct ad4691_chip_info	*chip;

> +	struct spi_device		*spi;

Why? regmap has reference to struct device. This should be enough.

> +	struct regmap			*regmap;
> +
> +	unsigned long			ref_clk_rate;
> +	struct pwm_device		*conv_trigger;
> +
> +	enum ad4691_adc_mode		adc_mode;
> +
> +	int				vref;

Units? _uV?

	int				vref_uV;

> +	u64				cnv_period;
> +	/*
> +	 * Synchronize access to members of the driver state, and ensure
> +	 * atomicity of consecutive SPI operations.
> +	 */
> +	struct mutex			lock;
> +};

...

> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_SAT_OVR_REG(15):
> +		ret = spi_write_then_read(st->spi, tx, 2, rx, 1);
> +		if (!ret)
> +			*val = rx[0];

Regular pattern, please.

		if (ret)
			return ret;
		...
		return 0;

> +		return ret;
> +	case AD4691_STD_SEQ_CONFIG:
> +	case AD4691_AVG_IN(0) ... AD4691_AVG_IN(15):
> +		ret = spi_write_then_read(st->spi, tx, 2, rx, 2);
> +		if (!ret)
> +			*val = get_unaligned_be16(rx);
> +		return ret;
> +	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(st->spi, tx, 2, rx, 3);
> +		if (!ret)
> +			*val = get_unaligned_be24(rx);
> +		return ret;
> +	case AD4691_ACC_STS_DATA(0) ... AD4691_ACC_STS_DATA(15):
> +		ret = spi_write_then_read(st->spi, tx, 2, rx, 4);
> +		if (!ret)
> +			*val = get_unaligned_be32(rx);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +	.cache_type = REGCACHE_RBTREE,

Why not MAPLE?

...

> +static int ad4691_get_sampling_freq(struct ad4691_state *st)
> +{
> +	if (st->adc_mode == AD4691_MANUAL_MODE) {
> +		return DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> +					     ktime_to_ns(st->sampling_period));
> +	}

Unneeded {}, why 64-bit division?

> +
> +	return DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC,
> +				     pwm_get_period(st->conv_trigger));

Ditto.

> +}

...

> +static int ad4691_pwm_get(struct spi_device *spi, struct ad4691_state *st)

Is spi used at all here? I do not see it. Why not drop it and take struct
device from regmap? Same question to the entire code.

...

> +		st->sampling_period = ns_to_ktime(DIV_ROUND_CLOSEST_ULL
> +			(NSEC_PER_SEC, freq));

Bad indentation.

...

> +	if (!freq || freq > st->chip->max_rate)
> +		return -EINVAL;

ERANGE ?

...

> +	unsigned long conv_us = DIV_ROUND_UP(2UL * USEC_PER_SEC,

Why UL?

> +					     st->chip->max_rate);

Also it's harder to read. Split this assignment to be closer for its first
user.

...

> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK1_REG, mask & 0xFF);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4691_ACC_MASK2_REG, (mask >> 8) & 0xFF);
> +	if (ret)
> +		return ret;

Why not bulk write?

...

> +static int ad4691_gpio_setup(struct ad4691_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct gpio_desc *reset;
> +
> +	reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset),
> +				     "Failed to get reset GPIO\n");
> +
> +	/* Reset delay required. See datasheet Table 5. */
> +	fsleep(300);
> +	gpiod_set_value(reset, 0);
> +
> +	return 0;
> +}

Use reset-gpio instead.

...

> +		ret = regmap_write(st->regmap, AD4691_REF_CTRL,
> +				   FIELD_PREP(AD4691_REF_CTRL_MASK,
> +					      AD4691_VREF_3P0));

Make the second line a bit longer. Ditto for the rest.

> +		break;
> +	case 3250001 ... 3750000:
> +		ret = regmap_write(st->regmap, AD4691_REF_CTRL,
> +				   FIELD_PREP(AD4691_REF_CTRL_MASK,
> +					      AD4691_VREF_3P3));
> +		break;
> +	case 3750001 ... 4500000:
> +		ret = regmap_write(st->regmap, AD4691_REF_CTRL,
> +				   FIELD_PREP(AD4691_REF_CTRL_MASK,
> +					      AD4691_VREF_4P096));
> +		break;
> +	case 4500001 ... AD4691_VREF_MAX:
> +		ret = regmap_write(st->regmap, AD4691_REF_CTRL,
> +				   FIELD_PREP(AD4691_REF_CTRL_MASK,
> +					      AD4691_VREF_5P0));
> +		break;

It's all the repetitions of the same regmap_write. Prepare value instead of ret
inside the switch and call regmap_write() only once after it.

...

> +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));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;

> +	st->spi = spi;

No need.

> +	st->regmap = devm_regmap_init(dev, NULL, st, &ad4691_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to initialize regmap\n");
> +
> +	st->chip = spi_get_device_match_data(spi);

> +	if (!st->chip)
> +		return dev_err_probe(dev, -ENODEV, "Could not find chip info\n");

We agreed to avoid this dead code.

> +	ret = ad4691_regulator_get(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4691_gpio_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4691_config(st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->chip->name;
> +	indio_dev->info = &ad4691_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	indio_dev->channels = (st->adc_mode == AD4691_MANUAL_MODE)
> +		? st->chip->manual_channels : st->chip->channels;

Locate ? on the previous line

> +	indio_dev->num_channels = st->chip->num_channels;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}


-- 
With Best Regards,
Andy Shevchenko