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

[Sabau, Radu bogdan]

Addressing Sashiko's review for initial driver's patch.

> -----Original Message-----
> From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@xxxxxxxxxx>
> Sent: Thursday, April 30, 2026 1:17 PM

...

> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 60038ae8dfc4..3685a03aa8dc 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -139,6 +139,17 @@ config AD4170_4
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad4170-4.
> 
> +config AD4691
> +	tristate "Analog Devices AD4691 Family ADC Driver"
> +	depends on SPI

"Should this driver also depend on REGULATOR? In ad4691_regulator_setup(),
it relies on devm_regulator_get_enable_read_voltage() to obtain its reference
voltage. If the kernel is compiled without CONFIG_REGULATOR, this function
is stubbed to return -ENODEV, causing probe to fail unconditionally."

Will add "depends on REGULATOR". Without it,
devm_regulator_get_enable_read_voltage() can be stubbed and
return -ENODEV, making the driver unusable regardless of hardware.

> +	select REGMAP
> +	help
> +	  Say yes here to build support for Analog Devices AD4691 Family

...

> +};
> +
> +#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),
> 	\

"Since ad4691_set_sampling_freq() modifies the AD4691_OSC_FREQ_REG register
which controls the global internal oscillator frequency for the entire ADC,
should IIO_CHAN_INFO_SAMP_FREQ be exposed as info_mask_shared_by_all
instead of info_mask_separate? Changing the frequency for one channel
currently seems to silently alter it for all channels."

Agreed for this commit. With no per-channel OSR in the base driver
all channels share a single oscillator register, so info_mask_shared_by_all
is the correct annotation. Changing accordingly. The OSR commit switches it
back to info_mask_separate because at that point the effective rate is
genuinely per-channel (osc_freq / osr[N]).

> +		.channel = ch,						\
> +		.scan_index = ch,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\
> +		},							\
> +	}
> +

...

> +};
> +
> +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;
> +
> +	/* Set bit 15 to mark the operation as READ. */
> +	put_unaligned_be16(0x8000 | reg, tx);
> +
> +	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, sizeof(tx), rx, 1);

"Will this result in a truncated 1-byte read for AD4691_ACC_MASK_REG (0x185)?
AD4691_ACC_MASK_REG falls into the range between AD4691_SPARE_CONTROL and
AD4691_ACC_SAT_OVR_REG(15). In ad4691_reg_write(), AD4691_ACC_MASK_REG is
handled explicitly alongside AD4691_STD_SEQ_CONFIG to perform a 16-bit
write, but it seems missing from the 2-byte read block here."

Valid bug. The write path already splits the range around ACC_MASK_REG; 
the read path was missing the same split. Fixed: ACC_MASK_REG now gets
a 2-byte read alongside STD_SEQ_CONFIG.

> +		if (ret)
> +			return ret;

...

> +
> +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) {
> +		/*
> +		 * reset_gpio_probe() already drives the pin asserted, so the
> +		 * device is held in reset before we get here.
> +		 * devm_reset_control_get_optional_exclusive_deasserted()
> cannot
> +		 * be used because it deasserts immediately without delay; the
> +		 * datasheet (Table 5) requires a ≥300 µs reset pulse width
> +		 * before deassertion.
> +		 */
> +		fsleep(300);
> +		return reset_control_deassert(rst);

"Could this sequence break on driver reload?
When the driver is unloaded, the reset line remains in its deasserted state
because there is no cleanup action to re-assert it. Upon reloading, the 300
microsecond sleep will occur while the pin is already deasserted, meaning
the hardware might not receive the required reset pulse.
Should reset_control_assert() be called explicitly prior to the delay?"

Valid. reset_control_put() does not re-assert the line at unload, so
on reload the pin is already deasserted when we enter the 300 µs sleep.
Adding reset_control_assert(rst) before fsleep(300) to guarantee a proper
reset pulse on every probe including reloads.

> +	}
> +

...

> +
> +	st = iio_priv(indio_dev);
> +	st->info = spi_get_device_match_data(spi);

"Is it possible for st->info to be NULL here?
If the driver is manually bound to a device with a non-matching name using
the sysfs bind interface in combination with driver_override,
spi_get_device_match_data() could return NULL, which would cause a NULL
pointer dereference later in the probe sequence."

Agreed, will add the NULL check with -ENODEV immediately after
spi_get_device_match_data().

> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;