Re: [PATCH v2 4/7] iio: core: Add cleanup.h support for iio_device_claim_*()

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 23 Dec 2025 11:23:24 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 12/11/25 8:45 PM, Kurt Borja wrote:
> > Add guard classes for iio_device_claim_*() conditional locks. This will
> > aid drivers write safer and cleaner code when dealing with some common
> > patterns.
> >   
> 
> 
> > These classes are not meant to be used directly by drivers (hence the
> > __priv__ prefix). Instead, documented wrapper macros are provided to
> > enforce the use of ACQUIRE() or guard() semantics and avoid the
> > problematic scoped guard.  
> 
> Would be useful to repeat this in a comment in the code.
> 
Given David did a much more thorough review than me I've just
added a few comments on top.

Jonathan

> > + * Context: Can sleep
> > + */
> > +#define IIO_DEV_ACQUIRE_DIRECT_MODE(_dev, _var) \
> > +	ACQUIRE(__priv__iio_dev_mode_lock_try_direct, _var)(_dev)
> > +
> > +/**
> > + * IIO_DEV_ACQUIRE_BUFFER_MODE(_dev, _var) - Tries to acquire the buffer mode
> > + *                                           lock with automatic release
> > + * @_dev: IIO device instance
> > + * @_var: Dummy variable identifier to store acquire result
> > + *
> > + * Tries to acquire the direct mode lock and automatically releases it at the
> > + * end of the scope. It most be paired with IIO_DEV_ACQUIRE_ERR(), for example::
> > + *
> > + *	IIO_DEV_ACQUIRE_BUFFER_MODE(indio_dev, claim);
> > + *	if (IIO_DEV_ACQUIRE_ERR(&claim))
> > + *		return IRQ_HANDLED;
> > + *
> > + * Context: Can sleep
> > + */
> > +#define IIO_DEV_ACQUIRE_BUFFER_MODE(_dev, _var) \
> > +	ACQUIRE(__priv__iio_dev_mode_lock_try_buffer, _var)(_dev)

This one doesn't actually have any users I think?  Do we want to introduce
it without them? I'm not sure we do.

> > +
> > +/**
> > + * IIO_DEV_ACQUIRE_ERR() - ACQUIRE_ERR() wrapper
> > + * @_var: Dummy variable passed to IIO_DEV_ACQUIRE_*_MODE()
> > + *
> > + * Return: true on success, false on error  
> 
> This could be clarified more. Based on the example, this sounds
> backwards.
> 
> 	Returns: true if acquiring the mode failed, otherwise false.
> 
> > + */
> > +#define IIO_DEV_ACQUIRE_ERR(_var_ptr) \
> > +	ACQUIRE_ERR(__priv__iio_dev_mode_lock_try_buffer, _var_ptr)  
> 
> There is no error code here, so calling it "ERR" seems wrong.
> Maybe IIO_DEV_ACQUIRE_FAILED()?
> 
> > +
> > +/**
> > + * IIO_DEV_GUARD_ANY_MODE - Acquires the mode lock with automatic release
> > + * @_dev: IIO device instance  
> 
> It would be helpful to explain more about the use case here and that this
> is used rarely.
> 
> The point to get across is that it is used when we need to do something
> that doesn't depend on the current mode, but would be affected by a mode
> switch. So it guards against changing the mode without caring what the
> current mode is. If it is in buffer mode, it stays in buffer mode, otherwise
> direct mode is claimed.
> 
> > + *
> > + * Acquires the mode lock with cleanup guard() semantics. It is usually paired
> > + * with iio_buffer_enabled().
> > + *
> > + * This should *not* be used to protect internal driver state and it's use in
> > + * general is *strongly* discouraged. Use any of the IIO_DEV_ACQUIRE_*_MODE()
> > + * variants.  
> 
> Might as well add Context: here like the others.
> 
> > + */
> > +#define IIO_DEV_GUARD_ANY_MODE(_dev) \  
> 
> Accordingly, I would be inclined to call it IIO_DEV_GUARD_CURRENT_MODE()

Agreed. That's a clearer name. 

> 
> > +	guard(__priv__iio_dev_mode_lock)(_dev)
> > +
> >  extern const struct bus_type iio_bus_type;
> >  
> >  /**
> >   
>