Re: [PATCH v6 1/3] iio: magnetometer: bmc150_magn: use automated cleanup for mutex

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Feb 10, 2026 at 06:39:43PM -0500, Neel Bullywon wrote:
> Use guard() and scoped_guard() to replace manual mutex lock/unlock
> calls. This simplifies error handling and ensures RAII-style cleanup.
> 
> scoped_guard() is used in read_raw IIO_CHAN_INFO_RAW case for a
> multi-statement mutex-protected block, as well as in remove,
> runtime_suspend, and suspend where a short mutex-protected scope
> is needed for a single function call.
> 
> guard() is used in write_raw and trig_reen where the mutex scope
> extends to the end of the function or case block. Case blocks in
> write_raw are wrapped in braces to ensure clear scope for the
> cleanup guards.
> 
> The trigger_handler function is left unchanged as mixing guard()
> with goto error paths can be fragile.

...

>  #include <linux/module.h>
>  #include <linux/i2c.h>
> +#include <linux/cleanup.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>

With a given context I would place the new one just before the delay.h.


...

>  	case IIO_CHAN_INFO_RAW:
>  		if (iio_buffer_enabled(indio_dev))
>  			return -EBUSY;
> -		mutex_lock(&data->mutex);
> -
> -		ret = bmc150_magn_set_power_state(data, true);
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			return ret;
> -		}
> +		scoped_guard(mutex, &data->mutex) {
> +			ret = bmc150_magn_set_power_state(data, true);
> +			if (ret < 0)
> +				return ret;
>  
> -		ret = bmc150_magn_read_xyz(data, values);
> -		if (ret < 0) {
> -			bmc150_magn_set_power_state(data, false);
> -			mutex_unlock(&data->mutex);
> -			return ret;
> -		}
> -		*val = values[chan->scan_index];
> +			ret = bmc150_magn_read_xyz(data, values);
> +			if (ret < 0) {
> +				bmc150_magn_set_power_state(data, false);
> +				return ret;
> +			}
> +			*val = values[chan->scan_index];
>  
> -		ret = bmc150_magn_set_power_state(data, false);
> -		if (ret < 0) {
> -			mutex_unlock(&data->mutex);
> -			return ret;
> +			ret = bmc150_magn_set_power_state(data, false);
> +			if (ret < 0)
> +				return ret;
>  		}
> -
> -		mutex_unlock(&data->mutex);
>  		return IIO_VAL_INT;

Actually looking again at this, I think we may use guard()(), but move the {}
to the whole case:

	case IIO_CHAN_INFO_RAW: {
		if (iio_buffer_enabled(indio_dev))
			return -EBUSY;

		guard(mutex)(&data->mutex);
		...
	}

This will make the change much more cleaner.

...

>  		case IIO_MOD_Z:
> +			{
> +				guard(mutex)(&data->mutex);

Why do you change only one case?! If the comment is given, it applies
to all similar places. But see also above.

-- 
With Best Regards,
Andy Shevchenko