On Fri, 20 Mar 2026 13:03:56 +0200
Radu Sabau via B4 Relay [off-list ref] 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.

Signed-off-by: Radu Sabau <radu.sabau@analog.com>

Hi Radu,

Just a few minor comments from me.
I'd have tweak them whilst applying but looks like you are doing a v5 anyway
for other comments.

Jonathan

+#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,				\
+			.shift = 0,					\

Unless there is a reason to need the 0 to act as documentation we normally
assume it's the natural default and don't set it explicitly (let the C
struct initialization rules deal with setting it to 0 for us).

+		},							\
+	}

+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 == HZ_PER_MHZ) ? 0 : 1;

Use 1 * HZ_PER_MHZ

May seem pointless but that conversion scale macro on it's own looks
strange with respect to what is it converting.

+	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++) {
+		if ((int)ad4691_osc_freqs[i] == freq)

I would flip this around to reduce the indent of the path for when
we have a match.

		if ((int)ad4691_osc_freqs[i] != freq)
			continue;

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

+			return regmap_update_bits(st->regmap, AD4691_OSC_FREQ_REG,
+						  AD4691_OSC_FREQ_MASK, i);
+	}
+
+	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. The chip always
+	 * operates in AUTONOMOUS mode in this driver revision.
+	 */
+	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));
+	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)]));

Go long on this line for readability.
			    ad4691_osc_freqs[FIELD_GET(AD4691_OSC_FREQ_MASK, reg_val)]));
Is fine.  The whole 80-100 char range is fine to use if it improves redability and
here I think it does.

+
+	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;
+}
+
+static int ad4691_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	struct ad4691_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW: {
+		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
+

I think we should have convention of no blank line here.
The bit above is setting a variable then we are checking it for error.

+		if (IIO_DEV_ACQUIRE_FAILED(claim))
+			return -EBUSY;
+
+		return ad4691_single_shot_read(indio_dev, chan, val);
+	}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad4691_get_sampling_freq(st, val);
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_uV / (MICRO / MILLI);
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	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_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, "ldo-in");
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to get and enable LDO-IN\n");
+	st->ldo_en = (ret == -ENODEV);
+
+	ret = devm_regulator_get_enable(dev, "vio");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get and enable VIO\n");

Unless there is an ordering constraint for power supplies, I'd move this up to
just after avdd so the two non optional ones are done together.

+
+	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (st->vref_uV >= 0) {
+		st->refbuf_en = false;
+	} else if (st->vref_uV == -ENODEV) {
+		st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "refin");
+		st->refbuf_en = true;
+	}