Re: [PATCH v5 3/4] iio: adc: ad799x: cache regulator voltages during probe

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 18 Mar 2026 14:57:14 +0530
Archit Anant <architanant5@gmail.com> wrote:

> Reading the regulator voltage via regulator_get_voltage() can be a slow
> operation.

Whilst that might be true, it isn't a reason for this change.
Sysfs reads that would cause it to be read are never a particularly
fast path anyway. So drop this first sentence.

> Since the reference voltages for this ADC are not expected to
> change at runtime, it is inefficient to query the regulator API every
> time userspace reads the IIO_CHAN_INFO_SCALE attribute.
> 
> Determine the active reference voltage (either VREF or VCC) during
> probe() and cache it in the state structure. This improves the
> performance of ad799x_read_raw() and removes the dependency on the
> regulator pointers during fast-path reads.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Archit Anant <architanant5@gmail.com>

A suggested alternative approach inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad799x.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index 7504bcf627da..ae2ad4bd37cc 100644
> --- a/drivers/iio/adc/ad799x.c
> +++ b/drivers/iio/adc/ad799x.c
> @@ -30,6 +30,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/bitops.h>
> +#include <linux/units.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -135,6 +136,9 @@ struct ad799x_state {
>  	u16				config;
>  
>  	unsigned int			transfer_size;
> +
> +	int				vref_uV;
> +
>  	IIO_DECLARE_BUFFER_WITH_TS(__be16, rx_buf, AD799X_MAX_CHANNELS);
>  };
>  
> @@ -302,14 +306,7 @@ static int ad799x_read_raw(struct iio_dev *indio_dev,
>  			GENMASK(chan->scan_type.realbits - 1, 0);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		if (st->vref)
> -			ret = regulator_get_voltage(st->vref);
> -		else
> -			ret = regulator_get_voltage(st->reg);
> -
> -		if (ret < 0)
> -			return ret;
> -		*val = ret / 1000;
> +		*val = st->vref_uV / MILLI;
>  		*val2 = chan->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	}
> @@ -828,9 +825,20 @@ static int ad799x_probe(struct i2c_client *client)
>  			ret = regulator_enable(st->vref);
>  			if (ret)
>  				goto error_disable_reg;
> +			ret = regulator_get_voltage(st->vref);

For vref I don't think we need to keep the regulator around, so you should
be able to use devm_regulator_get_enable_read_voltage() with checking
for -ENODEV to identify it simply isn't there.

It would need a tiny bit of reordering though or a custom
devm_add_action_or_reset() registered callback to ensure that regulator
disable for vcc happens in reverse sequence of what happens on setup.

Anyone think there are actually ordering constraints on these regulators?
Would be fairly unusual for this sort of device, but not impossible.
If not, cleanest option might be;

	ret = devm_regulator_get_enable_read_voltage(dev, "vref");
	if (ret < 0 && ret != -ENODEV){ //get the actual error out the way first.
		return ret;

	if (ret == -ENODEV) {
		ret = devm_regulator_get_enable_read_voltage(dev, "vcc");
		if (ret < 0)
			return ret;

		st->vref_uv = ret;
	} else {
		st->vref_uv = ret;
		ret = devm_regulator_get_enabled(dev, "vcc);
		if (ret)
			return ret;
	}

Then no need to undo anything by hand in remove() and no need to keep
a pointer to any regulators around for later.


> +			if (ret < 0)
> +				goto error_disable_vref;
> +			st->vref_uV = ret;
>  		}
>  	}
>  
> +	if (!st->vref) {
> +		ret = regulator_get_voltage(st->reg);
> +		if (ret < 0)
> +			goto error_disable_reg;
> +		st->vref_uV = ret;
> +	}
> +
>  	st->client = client;
>  
>  	indio_dev->name = id->name;