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

[PATCH v6 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 v6:
- Addressed style feedback from Andy Shevchenko: used '(MICRO / MILLI)'
  for clearer unit conversion and refactored the optional VREF error
  path for better readability.
- Removed the phrase "Reading the regulator voltage via
  regulator_get_voltage() can be a slow operation" from the commit
  message of patch 3/4 as requested by Jonathan Cameron.
- Confirmed with maintainers that the devm_add_action_or_reset() pattern
  must remain to preserve PM functionality.

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 | 110 ++++++++++++++++++---------------------
 1 file changed, 51 insertions(+), 59 deletions(-)

-- 
2.39.5

[PATCH v6 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 v6 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

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

[Andy Shevchenko]

On Thu, Mar 26, 2026 at 11:35:27PM +0530, Archit Anant wrote:
> 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.

...

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

Indentation also can be fixed while at it

	b_sent = i2c_smbus_read_i2c_block_data(st->client,
					       cmd, st->transfer_size,
					       (u8 *)st->rx_buf);

(no need to resend just because of this).

>  	if (b_sent < 0)
>  		goto out;

-- 
With Best Regards,
Andy Shevchenko

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

[Archit Anant]

Since the reference voltage for this ADC is not expected to
change at runtime, determine the active reference voltage (either VREF
or VCC) during probe() and cache it in a single variable 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..3cf7850357ab 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 / (MICRO / 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 v6 3/4] iio: adc: ad799x: cache regulator voltages during probe

[Andy Shevchenko]

On Thu, Mar 26, 2026 at 11:35:28PM +0530, Archit Anant wrote:
> Since the reference voltage for this ADC is not expected to
> change at runtime, determine the active reference voltage (either VREF
> or VCC) during probe() and cache it in a single variable in the state
> structure.
> 
> This improves the performance of ad799x_read_raw() and removes the
> dependency on the regulator pointers during fast-path reads.

...

>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/bitops.h>
> +#include <linux/units.h>

Can we have a prerequisite patch that sorts headers?

-- 
With Best Regards,
Andy Shevchenko

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

[Archit Anant]

On Fri, Mar 27, 2026 at 12:42 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Mar 26, 2026 at 11:35:28PM +0530, Archit Anant wrote:
> > Since the reference voltage for this ADC is not expected to
> > change at runtime, determine the active reference voltage (either VREF
> > or VCC) during probe() and cache it in a single variable in the state
> > structure.
> >
> > This improves the performance of ad799x_read_raw() and removes the
> > dependency on the regulator pointers during fast-path reads.
>
> ...
>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/bitops.h>
> > +#include <linux/units.h>
>
> Can we have a prerequisite patch that sorts headers?

Sure, I'll add a patch that sorts the headers.

>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Sincerely,
Archit Anant

[PATCH v6 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 | 66 ++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index 3cf7850357ab..c12bd7ed4dd7 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,15 +813,19 @@ 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;
+		if (ret == -ENODEV) {
 			st->vref = NULL;
 			dev_info(dev, "Using VCC reference voltage\n");
+		} else if (ret) {
+			return ret;
 		}
 
 		if (st->vref) {
@@ -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 v6 4/4] iio: adc: ad799x: use devm_iio_device_register and drop remove()

[Andy Shevchenko]

On Thu, Mar 26, 2026 at 11:35:29PM +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.

...

> -	mutex_init(&st->lock);
> -
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_mutex_init(dev, &st->lock);

Looking at how far from the start of probe this is done, it's prone to problems.
What we need is to decouple this change and move mutex to be enabled before any
interrupts or other async events may happen.

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


-- 
With Best Regards,
Andy Shevchenko

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

[Archit Anant]

On Fri, Mar 27, 2026 at 12:45 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Mar 26, 2026 at 11:35:29PM +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.
>
> ...
>
> > -     mutex_init(&st->lock);
> > -
> > -     ret = iio_device_register(indio_dev);
> > +     ret = devm_mutex_init(dev, &st->lock);
>
> Looking at how far from the start of probe this is done, it's prone to problems.
> What we need is to decouple this change and move mutex to be enabled before any
> interrupts or other async events may happen.
>

I understand how this can cause issues; I'll move the mutex lock up in
probe before any interrupts or other async events can occur and send a
new revision.

> >       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;
> > -}
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Sincerely,
Archit Anant