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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 25 May 2026 13:10:11 +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 by asserting
> the reset-gpios GPIO line for at least 300 µs then deasserting it;

As Sashiko notes this doesn't quite correspond to the code which deasserts
then sleeps.  The comments in the code make it obvious that was intended
so this commit description probably just needs an update.

I guess you probably already saw and fixed this.

> a software reset via SPI_CONFIG_A is used as fallback when no reset
> GPIO is provided.
> 
> 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>

Otherwise, just one question inline.

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


> +
> +static bool ad4691_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case AD4691_STATUS_REG:
> +	case AD4691_CLAMP_STATUS1_REG:
> +	case AD4691_CLAMP_STATUS2_REG:
> +	case AD4691_GPIO_READ:
> +	case AD4691_ACC_STATUS_FULL1_REG ... AD4691_ACC_STATUS_SAT2_REG:
> +	case AD4691_ACC_SAT_OVR_REG(0) ... AD4691_ACC_SAT_OVR_REG(15):
> +		return true;
> +	default:
> +		break;
> +	}
> +
> +	/*
> +	 * Multi-byte registers have non-unit strides; only accept base
> +	 * addresses to prevent debugfs from triggering reads that cross
> +	 * register boundaries.
What does this have to do with preventing debugfs from doing anything.
It should allow reading of volatile registers. Why is it not enough to
make the non base aligned registers neither readable nor writeable?

> +	 */
> +	if (reg >= AD4691_AVG_IN(0) && reg <= AD4691_AVG_IN(15))
> +		return (reg - AD4691_AVG_IN(0)) % 2 == 0;
> +	if (reg >= AD4691_AVG_STS_IN(0) && reg <= AD4691_AVG_STS_IN(15))
> +		return (reg - AD4691_AVG_STS_IN(0)) % 3 == 0;
> +	if (reg >= AD4691_ACC_IN(0) && reg <= AD4691_ACC_IN(15))
> +		return (reg - AD4691_ACC_IN(0)) % 3 == 0;
> +	if (reg >= AD4691_ACC_STS_DATA(0) && reg <= AD4691_ACC_STS_DATA(15))
> +		return (reg - AD4691_ACC_STS_DATA(0)) % 4 == 0;
> +
> +	return false;
> +}
> +
> +static bool ad4691_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case 0 ... AD4691_OSC_FREQ_REG:
> +	case AD4691_SPARE_CONTROL ... AD4691_ACC_SAT_OVR_REG(15):
> +	case AD4691_STD_SEQ_CONFIG:
> +		return true;
> +	default:
> +		break;
> +	}
> +
> +	/* Multi-byte registers: only accept base addresses (see volatile_reg). */
> +	if (reg >= AD4691_AVG_IN(0) && reg <= AD4691_AVG_IN(15))
> +		return (reg - AD4691_AVG_IN(0)) % 2 == 0;
> +	if (reg >= AD4691_AVG_STS_IN(0) && reg <= AD4691_AVG_STS_IN(15))
> +		return (reg - AD4691_AVG_STS_IN(0)) % 3 == 0;
> +	if (reg >= AD4691_ACC_IN(0) && reg <= AD4691_ACC_IN(15))
> +		return (reg - AD4691_ACC_IN(0)) % 3 == 0;
> +	if (reg >= AD4691_ACC_STS_DATA(0) && reg <= AD4691_ACC_STS_DATA(15))
> +		return (reg - AD4691_ACC_STS_DATA(0)) % 4 == 0;
> +
> +	return false;
> +}

> +static int ad4691_reset(struct ad4691_state *st)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct reset_control *rst;
> +	int ret;
> +
> +	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) {
> +		/*
> +		 * Assert the reset line to guarantee a clean reset pulse on
> +		 * every probe, including driver reloads where the line may
> +		 * already be deasserted (reset_control_put() does not
> +		 * re-assert on release). tRESETL (minimum pulse width) = 10 ns
> +		 * (Table 5); kernel function-call overhead alone exceeds this,
> +		 * so no explicit delay is needed between assert and deassert.
> +		 */
> +		reset_control_assert(rst);
> +		ret = reset_control_deassert(rst);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* No hardware reset available, fall back to software reset. */
> +		ret = regmap_write(st->regmap, AD4691_SPI_CONFIG_A_REG,
> +				   AD4691_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Wait 300 µs (Table 5) for the device to complete its internal reset
> +	 * sequence before accepting SPI commands.
> +	 */
> +	fsleep(300);
This is what sashiko was moaning about.

> +	return 0;
> +}