Re: [PATCH] iio: accel: bmc150: use guard(mutex) for mutex handling

[Jonathan Cameron <jic23@kernel.org>]

On Wed,  8 Apr 2026 09:28:42 +0100
Gabriel Rondon <grondon@gmail.com> wrote:

> Replace manual mutex_lock()/mutex_unlock() pairs with guard(mutex)
> from cleanup.h in functions where the mutex protects the entire
> function body. This simplifies error paths by removing the need for
> explicit unlock calls before returning.
> 
> Converted functions:
>   - bmc150_accel_get_temp()
>   - bmc150_accel_write_event_config()
>   - bmc150_accel_get_fifo_watermark()
>   - bmc150_accel_get_fifo_state()
>   - bmc150_accel_set_watermark()
>   - bmc150_accel_fifo_flush()
>   - bmc150_accel_trigger_set_state()
Not sure this list is useful in the commit log. Rather verbose.
> 
> Functions where the mutex is released before subsequent non-trivial
> work (e.g. bmc150_accel_get_axis, bmc150_accel_trigger_handler) are
Add () after those function names.

I took a quick look and can't see anything beyond an if (ret) check in
bmc150_accel_get_axis()

If it's the only one left I'd use scoped_guard() for bmc150_accel_trigger_handler()

I'm not against a mix of guard and not as appropriate in a given place, but
when there is only one left it seems silly to make a reader have to consider
both styles!

> left unchanged to preserve the existing lock scope.
> 
> Signed-off-by: Gabriel Rondon <grondon@gmail.com>
Good patch in general but there is room for further simplifications
in the code being touched.

> ---
>  drivers/iio/accel/bmc150-accel-core.c | 49 ++++++++-------------------
>  1 file changed, 15 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
> index 42ccf0316ce5..6a2d7a133d2e 100644
> --- a/drivers/iio/accel/bmc150-accel-core.c
> +++ b/drivers/iio/accel/bmc150-accel-core.c
> @@ -7,6 +7,7 @@
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
> @@ -597,21 +598,18 @@ static int bmc150_accel_set_scale(struct bmc150_accel_data *data, int val)
>  static int bmc150_accel_get_temp(struct bmc150_accel_data *data, int *val)
>  {
>  	struct device *dev = regmap_get_device(data->regmap);
> -	int ret;
>  	unsigned int value;
> +	int ret;

Unrelated change - here it just acts as noise for the real changes so don't
do this sort of code movement of lines we aren't otherwise touching.

>  
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  
>  	ret = regmap_read(data->regmap, BMC150_ACCEL_REG_TEMP, &value);
>  	if (ret < 0) {
>  		dev_err(dev, "Error reading reg_temp\n");
> -		mutex_unlock(&data->mutex);
>  		return ret;
>  	}
>  	*val = sign_extend32(value, 7);
>  
> -	mutex_unlock(&data->mutex);
> -
>  	return IIO_VAL_INT;
>  }
>  

> @@ -847,9 +842,8 @@ static ssize_t bmc150_accel_get_fifo_watermark(struct device *dev,
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
>  	int wm;
>  
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  	wm = data->watermark;
> -	mutex_unlock(&data->mutex);
>  
>  	return sprintf(buf, "%d\n", wm);

	return sprintf(buf, "%d\n", data->watermark);

>  }
> @@ -862,9 +856,8 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev,
>  	struct bmc150_accel_data *data = iio_priv(indio_dev);
>  	bool state;
>  
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  	state = data->fifo_mode;
> -	mutex_unlock(&data->mutex);
>  
>  	return sprintf(buf, "%d\n", state);

	return sprintf(buf, "%d\n", data->fifo_mode);

Though, given we are touching them anyway maybe also
	return sysfs_emit(buf, %d\n", data->fifo_mode);
is appropriate.

>  }
> @@ -906,9 +899,8 @@ static int bmc150_accel_set_watermark(struct iio_dev *indio_dev, unsigned val)
>  	if (val > BMC150_ACCEL_FIFO_LENGTH)
>  		val = BMC150_ACCEL_FIFO_LENGTH;

Unrelated but if I were reviewing this code today I'd have suggested
	val = min(val, BMC150_ACCEL_FIFO_LENGTH);

>  
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  	data->watermark = val;
> -	mutex_unlock(&data->mutex);
>  
>  	return 0;
>  }