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

This simplifies the error handling in ad799x_probe() by removing the
'error_cleanup_ring' goto label. It also removes the need to manually
call iio_device_unregister() and iio_triggered_buffer_cleanup() in
ad799x_remove().

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

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index 108bb22162ef..42712372acdb 100644
--- a/drivers/iio/adc/ad799x.c
+++ b/drivers/iio/adc/ad799x.c
@@ -847,7 +847,7 @@ static int ad799x_probe(struct i2c_client *client)
 	if (ret)
 		goto error_disable_vref;
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
 		&ad799x_trigger_handler, NULL);
 	if (ret)
 		goto error_disable_vref;
@@ -862,19 +862,17 @@ static int ad799x_probe(struct i2c_client *client)
 						client->name,
 						indio_dev);
 		if (ret)
-			goto error_cleanup_ring;
+			goto error_disable_vref;
 	}
 
 	mutex_init(&st->lock);
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(&client->dev, indio_dev);
 	if (ret)
-		goto error_cleanup_ring;
+		goto error_disable_vref;
 
 	return 0;
 
-error_cleanup_ring:
-	iio_triggered_buffer_cleanup(indio_dev);
 error_disable_vref:
 	if (st->vref)
 		regulator_disable(st->vref);
@@ -889,9 +887,6 @@ 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);
-
-	iio_triggered_buffer_cleanup(indio_dev);
 	if (st->vref)
 		regulator_disable(st->vref);
 	regulator_disable(st->reg);
-- 
2.39.5

On Sat, 28 Feb 2026 21:15:15 +0530
Archit Anant <architanant5@gmail.com> wrote:

> Convert the driver to use the device-managed versions of
> iio_device_register() and iio_triggered_buffer_setup().
> 
> This simplifies the error handling in ad799x_probe() by removing the
> 'error_cleanup_ring' goto label. It also removes the need to manually
> call iio_device_unregister() and iio_triggered_buffer_cleanup() in
> ad799x_remove().

It's also unfortunately broken.  There is a simple rule of thumb for
use of devm.  It must be used for 'everything' up to the point in probe()
where you decide to stop using it.  After that it cannot be used
at all.

The reason is that everything in the remove() callback occurs before the
unwinding of devm actions.  Thus if you mix and match, you get ordering
problems.

In this case you turn the power off before removing the user space
interfaces which is going to be rather unexpected if someone is still
using those interfaces.

Jonathan

> 
> Signed-off-by: Archit Anant <architanant5@gmail.com>
> ---
>  drivers/iio/adc/ad799x.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index 108bb22162ef..42712372acdb 100644
> --- a/drivers/iio/adc/ad799x.c
> +++ b/drivers/iio/adc/ad799x.c
> @@ -847,7 +847,7 @@ static int ad799x_probe(struct i2c_client *client)
>  	if (ret)
>  		goto error_disable_vref;
>  
> -	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>  		&ad799x_trigger_handler, NULL);
>  	if (ret)
>  		goto error_disable_vref;
> @@ -862,19 +862,17 @@ static int ad799x_probe(struct i2c_client *client)
>  						client->name,
>  						indio_dev);
>  		if (ret)
> -			goto error_cleanup_ring;
> +			goto error_disable_vref;
>  	}
>  
>  	mutex_init(&st->lock);
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
>  	if (ret)
> -		goto error_cleanup_ring;
> +		goto error_disable_vref;
>  
>  	return 0;
>  
> -error_cleanup_ring:
> -	iio_triggered_buffer_cleanup(indio_dev);
>  error_disable_vref:
>  	if (st->vref)
>  		regulator_disable(st->vref);
> @@ -889,9 +887,6 @@ 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);
> -
> -	iio_triggered_buffer_cleanup(indio_dev);
>  	if (st->vref)
>  		regulator_disable(st->vref);
>  	regulator_disable(st->reg);

On 2/28/26 9:45 AM, Archit Anant wrote:
> Convert the driver to use the device-managed versions of
> iio_device_register() and iio_triggered_buffer_setup().
> 
> This simplifies the error handling in ad799x_probe() by removing the
> 'error_cleanup_ring' goto label. It also removes the need to manually
> call iio_device_unregister() and iio_triggered_buffer_cleanup() in
> ad799x_remove().
> 
Since we are doing this, why not also handle the regulators and
rx_buf so that we can drop the remove() function completely?

Hi David,

On Sat, Feb 28, 2026 at 10:06 PM David Lechner <dlechner@baylibre.com> wrote:
>
> On 2/28/26 9:45 AM, Archit Anant wrote:
> > Convert the driver to use the device-managed versions of
> > iio_device_register() and iio_triggered_buffer_setup().
> >
> > This simplifies the error handling in ad799x_probe() by removing the
> > 'error_cleanup_ring' goto label. It also removes the need to manually
> > call iio_device_unregister() and iio_triggered_buffer_cleanup() in
> > ad799x_remove().
> >
> Since we are doing this, why not also handle the regulators and
> rx_buf so that we can drop the remove() function completely?

I completely agree that dropping the remove() function is the
ideal end state but I initially stopped short of doing that because of two
hurdles:

1. regulators: since ad799x_read_raw() needs the regulator pointers
to call regulator_get_voltage(), I couldn't simply use
devm_regulator_get_enable().

2. rx_buf: st->rx_buf is dynamically re-allocated (kfree then kmalloc)
inside ad799x_update_scan_mode() based on the scan mask. If I use
devm_kmalloc there, it would leak memory on every mask change.

To drop remove() completely, would you prefer I use devm_add_action_or_reset()
to register custom disable & free callbacks for the regulators and the
final state of rx_buf?

If that approach sounds good to you, I will gladly prepare a v2 that
eliminates the remove() function entirely.


-- 
Sincerely,
Archit Anant

On 2/28/26 11:19 AM, Archit Anant wrote:
> Hi David,
> 
> On Sat, Feb 28, 2026 at 10:06 PM David Lechner <dlechner@baylibre.com> wrote:
>>
>> On 2/28/26 9:45 AM, Archit Anant wrote:
>>> Convert the driver to use the device-managed versions of
>>> iio_device_register() and iio_triggered_buffer_setup().
>>>
>>> This simplifies the error handling in ad799x_probe() by removing the
>>> 'error_cleanup_ring' goto label. It also removes the need to manually
>>> call iio_device_unregister() and iio_triggered_buffer_cleanup() in
>>> ad799x_remove().
>>>
>> Since we are doing this, why not also handle the regulators and
>> rx_buf so that we can drop the remove() function completely?
> 
> I completely agree that dropping the remove() function is the
> ideal end state but I initially stopped short of doing that because of two
> hurdles:
> 
> 1. regulators: since ad799x_read_raw() needs the regulator pointers
> to call regulator_get_voltage(), I couldn't simply use
> devm_regulator_get_enable().

In this case, we can use devm_add_action_or_reset() to register the
disable callbacks.

> 
> 2. rx_buf: st->rx_buf is dynamically re-allocated (kfree then kmalloc)
> inside ad799x_update_scan_mode() based on the scan mask. If I use
> devm_kmalloc there, it would leak memory on every mask change.
> 
> To drop remove() completely, would you prefer I use devm_add_action_or_reset()
> to register custom disable & free callbacks for the regulators and the
> final state of rx_buf?
> 
> If that approach sounds good to you, I will gladly prepare a v2 that
> eliminates the remove() function entirely.
> 
> 

Here, we could also use devm_add_action_or_reset() or we could drop
the dynamic allocation altogether and make rx_buf large enough for
the largest transfer. The latter would be preferred as that is how
we usually do it in general.

#define AD799X_MAX_CHANNELS 8

...

struct ad799x_state {
	...
	unsigned int			transfer_size;
	IIO_DECLARE_DMA_BUFFER_WITH_TS(__be16, rx_buf, AD799X_MAX_CHANNELS);
};

Note, it is important for this to be the last field in the struct
for DMA alignment purposes.

On Sat, 28 Feb 2026 11:35:15 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 2/28/26 11:19 AM, Archit Anant wrote:
> > Hi David,
> > 
> > On Sat, Feb 28, 2026 at 10:06 PM David Lechner <dlechner@baylibre.com> wrote:  
> >>
> >> On 2/28/26 9:45 AM, Archit Anant wrote:  
> >>> Convert the driver to use the device-managed versions of
> >>> iio_device_register() and iio_triggered_buffer_setup().
> >>>
> >>> This simplifies the error handling in ad799x_probe() by removing the
> >>> 'error_cleanup_ring' goto label. It also removes the need to manually
> >>> call iio_device_unregister() and iio_triggered_buffer_cleanup() in
> >>> ad799x_remove().
> >>>  
> >> Since we are doing this, why not also handle the regulators and
> >> rx_buf so that we can drop the remove() function completely?  
> > 
> > I completely agree that dropping the remove() function is the
> > ideal end state but I initially stopped short of doing that because of two
> > hurdles:
> > 
> > 1. regulators: since ad799x_read_raw() needs the regulator pointers
> > to call regulator_get_voltage(), I couldn't simply use
> > devm_regulator_get_enable().  
> 
> In this case, we can use devm_add_action_or_reset() to register the
> disable callbacks.

It is vanishingly rare for for the voltages to change after driver probe,
so an alternative that is almost certainly fine is to read and cache the
voltage at probe.

> 
> > 
> > 2. rx_buf: st->rx_buf is dynamically re-allocated (kfree then kmalloc)
> > inside ad799x_update_scan_mode() based on the scan mask. If I use
> > devm_kmalloc there, it would leak memory on every mask change.
> > 
> > To drop remove() completely, would you prefer I use devm_add_action_or_reset()
> > to register custom disable & free callbacks for the regulators and the
> > final state of rx_buf?
> > 
> > If that approach sounds good to you, I will gladly prepare a v2 that
> > eliminates the remove() function entirely.
> > 
> >   
> 
> Here, we could also use devm_add_action_or_reset() or we could drop
> the dynamic allocation altogether and make rx_buf large enough for
> the largest transfer. The latter would be preferred as that is how
> we usually do it in general.

Agreed.

> 
> #define AD799X_MAX_CHANNELS 8
> 
> ...
> 
> struct ad799x_state {
> 	...
> 	unsigned int			transfer_size;
> 	IIO_DECLARE_DMA_BUFFER_WITH_TS(__be16, rx_buf, AD799X_MAX_CHANNELS);
> };
> 
> Note, it is important for this to be the last field in the struct
> for DMA alignment purposes.
> 
I2C driver, so no need. I2C by default bounce buffers everything anyway so we don't
need an aligned buffer (or the padding that results in at the end of the structure).

Jonathan

On 3/1/26 5:34 AM, Jonathan Cameron wrote:
> On Sat, 28 Feb 2026 11:35:15 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 2/28/26 11:19 AM, Archit Anant wrote:
>>> Hi David,
>>>
>>> On Sat, Feb 28, 2026 at 10:06 PM David Lechner <dlechner@baylibre.com> wrote:  
>>>>
>>>> On 2/28/26 9:45 AM, Archit Anant wrote:  
>>>>> Convert the driver to use the device-managed versions of
>>>>> iio_device_register() and iio_triggered_buffer_setup().
>>>>>
>>>>> This simplifies the error handling in ad799x_probe() by removing the
>>>>> 'error_cleanup_ring' goto label. It also removes the need to manually
>>>>> call iio_device_unregister() and iio_triggered_buffer_cleanup() in
>>>>> ad799x_remove().
>>>>>  
>>>> Since we are doing this, why not also handle the regulators and
>>>> rx_buf so that we can drop the remove() function completely?  
>>>
>>> I completely agree that dropping the remove() function is the
>>> ideal end state but I initially stopped short of doing that because of two
>>> hurdles:
>>>
>>> 1. regulators: since ad799x_read_raw() needs the regulator pointers
>>> to call regulator_get_voltage(), I couldn't simply use
>>> devm_regulator_get_enable().  
>>
>> In this case, we can use devm_add_action_or_reset() to register the
>> disable callbacks.
> 
> It is vanishingly rare for for the voltages to change after driver probe,
> so an alternative that is almost certainly fine is to read and cache the
> voltage at probe.

In this driver, we still need the handle to the regulator for suspend
resume, so that doesn't avoid the need for using devm_add_action_or_reset()
in this particular case, I don't think.

>> #define AD799X_MAX_CHANNELS 8
>>
>> ...
>>
>> struct ad799x_state {
>> 	...
>> 	unsigned int			transfer_size;
>> 	IIO_DECLARE_DMA_BUFFER_WITH_TS(__be16, rx_buf, AD799X_MAX_CHANNELS);
>> };
>>
>> Note, it is important for this to be the last field in the struct
>> for DMA alignment purposes.
>>
> I2C driver, so no need. I2C by default bounce buffers everything anyway so we don't
> need an aligned buffer (or the padding that results in at the end of the structure).

Thanks for the correction. I just saw an I2C driver the other day that
was incorrectly doing this then that lead me astray.