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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Mon, 23 Mar 2026 09:39:44 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 3/23/26 7:22 AM, Archit Anant wrote:
> > On Sat, Mar 21, 2026 at 11:57 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> >>
> >> 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;  
> > ...  
> >> Then no need to undo anything by hand in remove() and no need to keep
> >> a pointer to any regulators around for later.  
> > 
> > I completely agree that devm_regulator_get_enable_read_voltage() is
> > the cleanest approach.
> > 
> > However, as I noted briefly in the v5 changelog (which I should have
> > highlighted better), the driver currently relies on those regulator
> > pointers (st->reg and st->vref) in the ad799x_suspend() and
> > ad799x_resume() callbacks.
> > 
> > If we drop the pointers from the state structure, we lose the ability
> > to disable the regulators during system sleep.
> > 
> > If keeping power management active during suspend is still desired for
> > this driver, I believe we are forced to keep the pointers and use the
> > devm_add_action_or_reset() pattern.  
> 
> Yes, this is the best we can do with current regulator APIs.
> 
> > 
> > If you prefer, I can drop the manual regulator control from the PM
> > callbacks entirely (or drop the PM callbacks altogether), which would
> > allow us to use the cleaner devm_regulator_get_enable_read_voltage()
> > helper.  
> 
> We can't do this unless we can be 100% sure we don't break existing
> users who might be depending on power management working as-is.
Yeah, I missed the use in suspend / resume for some reason.

Not much we can do to improve things :(

Jonathan

> 
> > 
> > Let me know which path you prefer for v6!
> >   
> 
> 
>