Re: [PATCH] iio: adc: ad799x: use devm_iio_device_register and devm buffer setup

[Jonathan Cameron]

On Sat, 28 Feb 2026 11:35:15 -0600
David Lechner <dlechner@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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