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

[David Lechner]

On 3/23/26 7:22 AM, Archit Anant wrote:
> 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.

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.

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