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

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

On Fri, Mar 20, 2026 at 01:03:56PM +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.

...

> +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;

> +	put_unaligned_be16(0x8000 | reg, tx);

I would expect that the config will have read_flag_mask set and here you just
use it. But it's fine like now, just add a comment

	/* Set bit 15 to mark the operation READ */

or something similar.

> +	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);
> +		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_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);

You can use the approach from below

	ret = devm_regulator_get_enable(dev, "ldo-in");
	if (ret == -ENODEV)
		st->ldo_en = true;
	else if (ret)
		return dev_err_probe(dev, ret, "Failed to get and enable LDO-IN\n");
	// no other branches assuming ldo_en = false due to kzalloc():ed memory.


> +	ret = devm_regulator_get_enable(dev, "vio");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get and enable VIO\n");
> +
> +	st->vref_uV = devm_regulator_get_enable_read_voltage(dev, "ref");

> +	if (st->vref_uV >= 0) {
> +		st->refbuf_en = false;

Do you need this? Isn't 'st' allocated with kzalloc() or alike?

> +	} else 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;
> +}

...

> +	if (!rst)

It's not an error check, so I would invert the condition.

> +		/* No hardware reset available, fall back to software reset. */
> +		return regmap_write(st->regmap, AD4691_SPI_CONFIG_A_REG,
> +				    AD4691_SW_RESET);
> +
> +	reset_control_assert(rst);
> +	/* Reset delay required. See datasheet Table 5. */
> +	fsleep(300);
> +	reset_control_deassert(rst);

	if (rst) {
		reset_control_assert(rst);
		/* Reset delay required. See datasheet Table 5. */
		fsleep(300);
		reset_control_deassert(rst);

		return 0;
	}

	/* No hardware reset available, fall back to software reset. */
	return regmap_write(st->regmap, AD4691_SPI_CONFIG_A_REG, AD4691_SW_RESET);


...

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

regmap_update_bits()?
regmap_assign_bits()?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write REF_CTRL\n");
> +
> +	ret = regmap_write(st->regmap, AD4691_DEVICE_SETUP,
> +			   st->ldo_en ? AD4691_LDO_EN : 0);

Ditto.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to write DEVICE_SETUP\n");
> +
> +	/*
> +	 * Set the internal oscillator to the highest valid rate for this chip.
> +	 * Index 0 (1 MHz) is valid only for AD4692/AD4694; AD4691/AD4693 start
> +	 * at index 1 (500 kHz).
> +	 */
> +	ret = regmap_write(st->regmap, AD4691_OSC_FREQ_REG,
> +			   (st->info->max_rate == HZ_PER_MHZ) ? 0 : 1);

Ditto.

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

-- 
With Best Regards,
Andy Shevchenko