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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 7 May 2026 09:26:00 +0000
"Sabau, Radu bogdan" <Radu.Sabau@analog.com> wrote:

> Addressing Sashiko's review for initial driver's patch.
> 
> > -----Original Message-----
> > From: Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org>
> > 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.

If you really want to also add || COMPILE_TEST because
I want maximum build coverage and for that I don't mind if the driver
can actually probe or not.

> 
> > +	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]).

Don't.  That would mean an ABI change mid way through the series which we
don't want. Just add a note to the patch description.

...


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

I'm not that fussed about driver reload bugs like this one but why
not fix it I guess.  Lots of cases of this in upstream code though.

> 
> > +	}
> > +  
> 
> ...
> 
> > +
> > +	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().

Andy, you seeing this one?  Looks like we are putting these checks back in again.
Whilst anyone forcing a bind like this is onto a looser anyway we shouldn't
crash due to a null dereference.

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