[PATCH v5 0/4] iio: adc: ad799x: modernize resource management

[PATCH v5 0/4] iio: adc: ad799x: modernize resource management

[Archit Anant]

This series modernizes the ad799x driver by converting resource 
management to the devm_ infrastructure. This results in cleaner 
probe error handling, the removal of the ad799x_remove() function, 
and improved code maintainability.

Key changes in this series include:
- Static allocation of the rx_buf using IIO_DECLARE_BUFFER_WITH_TS()
  to avoid dynamic memory leaks during scan mode updates.
- Caching of VCC and VREF regulator voltages during probe() to 
  optimize read_raw() performance and simplify power management logic.
- Migration of regulators and mutex initialization to device-managed 
  resources (devm).
- Removal of the manual ad799x_remove() function and all goto-based 
  error cleanup labels in ad799x_probe().

These changes ensure the teardown sequence is always correct, 
following the reverse order of allocation, and remove unnecessary 
boilerplate code from the driver.

Changes in v5:
- Used devm_add_action_or_reset() for regulator disabling instead of
  devm_regulator_get_enable_read_voltage() as suggested by Jonathan 
  Cameron. This was necessary to preserve the regulator pointers (st->reg 
  and st->vref) required by the driver's suspend/resume power management 
  callbacks.
- Simplified voltage caching to use a single 'vref_uV' variable.
- Converted mutex_init() to devm_mutex_init().
- Resolved error path ordering issues by using devm_* exclusively, 
  allowing for the complete removal of the ad799x_remove() function.

Changes in v4:
- Simplified voltage caching to use a single 'vref_uV' variable as
suggested by David Lechner.
- Capitalized 'uV' in variable names as suggested by Andy Shevchenko.
- Removed redundant 'if (ret < 0)' check in read_raw() (dead check).
- Used devm_mutex_init() to replace manual mutex handling.
- Introduced a local 'dev' pointer in probe() to simplify function calls.
- Switched to IIO_DECLARE_BUFFER_WITH_TS() for the buffer declaration.
- Moved goto removals into the respective patches where devm conversion
occurs.

Changes in v3:
- Split the monolithic v2 patch into 4 logical commits as requested by
  Andy Shevchenko.

Changes in v2:
- Eliminated dynamic buffer allocation (feedback from David Lechner).
- Cached voltages to prevent devm/manual ordering hazards (feedback 
  from Jonathan Cameron).


Archit Anant (4):
  iio: adc: ad799x: use local device pointer in probe
  iio: adc: ad799x: use a static buffer for scan data
  iio: adc: ad799x: cache regulator voltages during probe
  iio: adc: ad799x: use devm_iio_device_register and drop remove()

 drivers/iio/adc/ad799x.c | 106 ++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 57 deletions(-)

-- 
2.39.5

[PATCH v5 1/4] iio: adc: ad799x: use local device pointer in probe

[Archit Anant]

Introduce a local device pointer 'dev' in ad799x_probe() and use it
throughout the function instead of accessing &client->dev repeatedly.

Signed-off-by: Archit Anant <architanant5@gmail.com>
---
 drivers/iio/adc/ad799x.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index 108bb22162ef..9825abc9285d 100644
--- a/drivers/iio/adc/ad799x.c
+++ b/drivers/iio/adc/ad799x.c
@@ -783,6 +783,7 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
 
 static int ad799x_probe(struct i2c_client *client)
 {
+	struct device *dev = &client->dev;
 	const struct i2c_device_id *id = i2c_client_get_device_id(client);
 	int ret;
 	int extra_config = 0;
@@ -791,7 +792,7 @@ static int ad799x_probe(struct i2c_client *client)
 	const struct ad799x_chip_info *chip_info =
 		&ad799x_chip_info_tbl[id->driver_data];
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (indio_dev == NULL)
 		return -ENOMEM;
 
@@ -807,7 +808,7 @@ static int ad799x_probe(struct i2c_client *client)
 
 	/* TODO: Add pdata options for filtering and bit delay */
 
-	st->reg = devm_regulator_get(&client->dev, "vcc");
+	st->reg = devm_regulator_get(dev, "vcc");
 	if (IS_ERR(st->reg))
 		return PTR_ERR(st->reg);
 	ret = regulator_enable(st->reg);
@@ -816,17 +817,17 @@ static int ad799x_probe(struct i2c_client *client)
 
 	/* check if an external reference is supplied */
 	if (chip_info->has_vref) {
-		st->vref = devm_regulator_get_optional(&client->dev, "vref");
+		st->vref = devm_regulator_get_optional(dev, "vref");
 		ret = PTR_ERR_OR_ZERO(st->vref);
 		if (ret) {
 			if (ret != -ENODEV)
 				goto error_disable_reg;
 			st->vref = NULL;
-			dev_info(&client->dev, "Using VCC reference voltage\n");
+			dev_info(dev, "Using VCC reference voltage\n");
 		}
 
 		if (st->vref) {
-			dev_info(&client->dev, "Using external reference voltage\n");
+			dev_info(dev, "Using external reference voltage\n");
 			extra_config |= AD7991_REF_SEL;
 			ret = regulator_enable(st->vref);
 			if (ret)
@@ -853,7 +854,7 @@ static int ad799x_probe(struct i2c_client *client)
 		goto error_disable_vref;
 
 	if (client->irq > 0) {
-		ret = devm_request_threaded_irq(&client->dev,
+		ret = devm_request_threaded_irq(dev,
 						client->irq,
 						NULL,
 						ad799x_event_handler,
-- 
2.39.5

[PATCH v5 2/4] iio: adc: ad799x: use a static buffer for scan data

[Archit Anant]

Currently, rx_buf is dynamically allocated using kmalloc() every time
ad799x_update_scan_mode() is called. This can lead to memory leaks if
the scan mask is updated multiple times.

Drop the dynamic allocation and replace it with a static buffer at the
end of the state structure using IIO_DECLARE_BUFFER_WITH_TS().
This eliminates the allocation overhead, prevents leaks, and removes
the need for manual kfree() on driver removal.

Signed-off-by: Archit Anant <architanant5@gmail.com>
---
 drivers/iio/adc/ad799x.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index 9825abc9285d..7504bcf627da 100644
--- a/drivers/iio/adc/ad799x.c
+++ b/drivers/iio/adc/ad799x.c
@@ -39,6 +39,7 @@
 #include <linux/iio/triggered_buffer.h>
 
 #define AD799X_CHANNEL_SHIFT			4
+#define AD799X_MAX_CHANNELS			8
 
 /*
  * AD7991, AD7995 and AD7999 defines
@@ -133,8 +134,8 @@ struct ad799x_state {
 	unsigned int			id;
 	u16				config;
 
-	u8				*rx_buf;
 	unsigned int			transfer_size;
+	IIO_DECLARE_BUFFER_WITH_TS(__be16, rx_buf, AD799X_MAX_CHANNELS);
 };
 
 static int ad799x_write_config(struct ad799x_state *st, u16 val)
@@ -217,7 +218,7 @@ static irqreturn_t ad799x_trigger_handler(int irq, void *p)
 	}
 
 	b_sent = i2c_smbus_read_i2c_block_data(st->client,
-			cmd, st->transfer_size, st->rx_buf);
+			cmd, st->transfer_size, (u8 *)st->rx_buf);
 	if (b_sent < 0)
 		goto out;
 
@@ -234,11 +235,6 @@ static int ad799x_update_scan_mode(struct iio_dev *indio_dev,
 {
 	struct ad799x_state *st = iio_priv(indio_dev);
 
-	kfree(st->rx_buf);
-	st->rx_buf = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
-	if (!st->rx_buf)
-		return -ENOMEM;
-
 	st->transfer_size = bitmap_weight(scan_mask,
 					  iio_get_masklength(indio_dev)) * 2;
 
@@ -896,7 +892,6 @@ static void ad799x_remove(struct i2c_client *client)
 	if (st->vref)
 		regulator_disable(st->vref);
 	regulator_disable(st->reg);
-	kfree(st->rx_buf);
 }
 
 static int ad799x_suspend(struct device *dev)
-- 
2.39.5

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

[Archit Anant]

Reading the regulator voltage via regulator_get_voltage() can be a slow
operation. 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>
---
 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);
+			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;
-- 
2.39.5

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

[Andy Shevchenko]

On Wed, Mar 18, 2026 at 02:57:14PM +0530, Archit Anant wrote:
> Reading the regulator voltage via regulator_get_voltage() can be a slow
> operation. 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.

...

>  	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;

Not sure I understand properly this MILLI in conjunction with FRACTIONAL_LOG2
below. Is this simply millivolts? Then we have "(MICRO / MILLI)" pattern as
of today to show that in a better way.

>  		*val2 = chan->scan_type.realbits;
>  		return IIO_VAL_FRACTIONAL_LOG2;

-- 
With Best Regards,
Andy Shevchenko

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

[Archit Anant]

On Wed, Mar 18, 2026 at 8:11 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Mar 18, 2026 at 02:57:14PM +0530, Archit Anant wrote:
> > Reading the regulator voltage via regulator_get_voltage() can be a slow
> > operation. 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.
>
> ...
>
> >       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;
>
> Not sure I understand properly this MILLI in conjunction with FRACTIONAL_LOG2
> below. Is this simply millivolts? Then we have "(MICRO / MILLI)" pattern as
> of today to show that in a better way.

My bad, I thought you wanted me to use either MICRO or MILLI i.e the
fitting one
instead of 1000. I see this is the preferred way to show unit conversions.
I'll redo in accordance with this pattern.

>
> >               *val2 = chan->scan_type.realbits;
> >               return IIO_VAL_FRACTIONAL_LOG2;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Sincerely,
Archit Anant

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

[Jonathan Cameron]

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;

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

[Andy Shevchenko]

On Sat, Mar 21, 2026 at 06:27:10PM +0000, Jonathan Cameron wrote:
> On Wed, 18 Mar 2026 14:57:14 +0530
> Archit Anant <architanant5@gmail.com> wrote:

...

> 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;

While the comment is true, I nevertheless prefer in this case (basically
regulator) to avoid double checking for ENODEV, id est...

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

...put it here as

	} else if (ret) {
		return 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.

-- 
With Best Regards,
Andy Shevchenko

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

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

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

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

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

[Jonathan Cameron]

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!
> >   
> 
> 
> 

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

[Archit Anant]

On Mon, Mar 23, 2026 at 11:25 PM Jonathan Cameron
<jonathan.cameron@huawei.com> wrote:
>
> On Mon, 23 Mar 2026 09:39:44 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
...
> > 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 :(
>

That settles it then, I'll implement Andy's style preference changes
and send a final v6 shortly.

-- 
Sincerely,
Archit Anant

[PATCH v5 4/4] iio: adc: ad799x: use devm_iio_device_register and drop remove()

[Archit Anant]

Convert the driver to use the device-managed versions of
iio_device_register(), iio_triggered_buffer_setup(), and mutex_init().

Use devm_add_action_or_reset() to ensure that the VCC and VREF
regulators are disabled safely and in the correct order during
driver teardown or probe failure.

Because all resources (buffer, regulators, IRQs, IIO device, mutex)
are now fully managed by the devm core, the unwinding order is
guaranteed to be correct (reverse order of allocation). We can now
safely remove all manual error handling goto labels in ad799x_probe()
and delete the ad799x_remove() function entirely.

This eliminates boilerplate code and prevents potential resource leaks.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Archit Anant <architanant5@gmail.com>
---
 drivers/iio/adc/ad799x.c | 62 ++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 37 deletions(-)

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index ae2ad4bd37cc..3d745612bb80 100644
--- a/drivers/iio/adc/ad799x.c
+++ b/drivers/iio/adc/ad799x.c
@@ -774,6 +774,11 @@ static const struct ad799x_chip_info ad799x_chip_info_tbl[] = {
 	},
 };
 
+static void ad799x_reg_disable(void *reg)
+{
+	regulator_disable(reg);
+}
+
 static int ad799x_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -808,13 +813,17 @@ static int ad799x_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	ret = devm_add_action_or_reset(dev, ad799x_reg_disable, st->reg);
+	if (ret)
+		return ret;
+
 	/* check if an external reference is supplied */
 	if (chip_info->has_vref) {
 		st->vref = devm_regulator_get_optional(dev, "vref");
 		ret = PTR_ERR_OR_ZERO(st->vref);
 		if (ret) {
 			if (ret != -ENODEV)
-				goto error_disable_reg;
+				return ret;
 			st->vref = NULL;
 			dev_info(dev, "Using VCC reference voltage\n");
 		}
@@ -824,10 +833,15 @@ static int ad799x_probe(struct i2c_client *client)
 			extra_config |= AD7991_REF_SEL;
 			ret = regulator_enable(st->vref);
 			if (ret)
-				goto error_disable_reg;
+				return ret;
+
+			ret = devm_add_action_or_reset(dev, ad799x_reg_disable, st->vref);
+			if (ret)
+				return ret;
+
 			ret = regulator_get_voltage(st->vref);
 			if (ret < 0)
-				goto error_disable_vref;
+				return ret;
 			st->vref_uV = ret;
 		}
 	}
@@ -835,7 +849,7 @@ static int ad799x_probe(struct i2c_client *client)
 	if (!st->vref) {
 		ret = regulator_get_voltage(st->reg);
 		if (ret < 0)
-			goto error_disable_reg;
+			return ret;
 		st->vref_uV = ret;
 	}
 
@@ -850,12 +864,12 @@ static int ad799x_probe(struct i2c_client *client)
 
 	ret = ad799x_update_config(st, st->chip_config->default_config | extra_config);
 	if (ret)
-		goto error_disable_vref;
+		return ret;
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
 		&ad799x_trigger_handler, NULL);
 	if (ret)
-		goto error_disable_vref;
+		return ret;
 
 	if (client->irq > 0) {
 		ret = devm_request_threaded_irq(dev,
@@ -867,39 +881,14 @@ static int ad799x_probe(struct i2c_client *client)
 						client->name,
 						indio_dev);
 		if (ret)
-			goto error_cleanup_ring;
+			return ret;
 	}
 
-	mutex_init(&st->lock);
-
-	ret = iio_device_register(indio_dev);
+	ret = devm_mutex_init(dev, &st->lock);
 	if (ret)
-		goto error_cleanup_ring;
-
-	return 0;
-
-error_cleanup_ring:
-	iio_triggered_buffer_cleanup(indio_dev);
-error_disable_vref:
-	if (st->vref)
-		regulator_disable(st->vref);
-error_disable_reg:
-	regulator_disable(st->reg);
-
-	return ret;
-}
-
-static void ad799x_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-	struct ad799x_state *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
+		return ret;
 
-	iio_triggered_buffer_cleanup(indio_dev);
-	if (st->vref)
-		regulator_disable(st->vref);
-	regulator_disable(st->reg);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static int ad799x_suspend(struct device *dev)
@@ -969,7 +958,6 @@ static struct i2c_driver ad799x_driver = {
 		.pm = pm_sleep_ptr(&ad799x_pm_ops),
 	},
 	.probe = ad799x_probe,
-	.remove = ad799x_remove,
 	.id_table = ad799x_id,
 };
 module_i2c_driver(ad799x_driver);
-- 
2.39.5

Re: [PATCH v5 4/4] iio: adc: ad799x: use devm_iio_device_register and drop remove()

[Andy Shevchenko]

On Wed, Mar 18, 2026 at 02:57:15PM +0530, Archit Anant wrote:
> Convert the driver to use the device-managed versions of
> iio_device_register(), iio_triggered_buffer_setup(), and mutex_init().
> 
> Use devm_add_action_or_reset() to ensure that the VCC and VREF
> regulators are disabled safely and in the correct order during
> driver teardown or probe failure.
> 
> Because all resources (buffer, regulators, IRQs, IIO device, mutex)
> are now fully managed by the devm core, the unwinding order is
> guaranteed to be correct (reverse order of allocation). We can now
> safely remove all manual error handling goto labels in ad799x_probe()
> and delete the ad799x_remove() function entirely.
> 
> This eliminates boilerplate code and prevents potential resource leaks.

...

>  		st->vref = devm_regulator_get_optional(dev, "vref");
>  		ret = PTR_ERR_OR_ZERO(st->vref);
>  		if (ret) {
>  			if (ret != -ENODEV)
> -				goto error_disable_reg;
> +				return ret;
>  			st->vref = NULL;
>  			dev_info(dev, "Using VCC reference voltage\n");
>  		}

		if (ret == -ENODEV) {
			st->vref = NULL;
			dev_info(dev, "Using VCC reference voltage\n");
		} else if (ret) {
			return ret;
		}


-- 
With Best Regards,
Andy Shevchenko