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

[Archit Anant]

On Sat, Mar 21, 2026 at 11:57 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Wed, 18 Mar 2026 14:57:14 +0530
> Archit Anant <architanant5@xxxxxxxxx> 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@xxxxxxxxxx>
> > Suggested-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > Signed-off-by: Archit Anant <architanant5@xxxxxxxxx>
>
> 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.

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.

Let me know which path you prefer for v6!

-- 
Sincerely,
Archit Anant