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

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

[Gabriel Rondon]

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()

Functions where the mutex is released before subsequent non-trivial
work (e.g. bmc150_accel_get_axis, bmc150_accel_trigger_handler) are
left unchanged to preserve the existing lock scope.

Signed-off-by: Gabriel Rondon <grondon@gmail.com>
---
 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;
 
-	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;
 }
 
@@ -810,17 +808,14 @@ static int bmc150_accel_write_event_config(struct iio_dev *indio_dev,
 	if (state == data->ev_enable_state)
 		return 0;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
 	ret = bmc150_accel_set_interrupt(data, BMC150_ACCEL_INT_ANY_MOTION,
 					 state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
+	if (ret < 0)
 		return ret;
-	}
 
 	data->ev_enable_state = state;
-	mutex_unlock(&data->mutex);
 
 	return 0;
 }
@@ -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);
 }
@@ -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);
 }
@@ -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;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 	data->watermark = val;
-	mutex_unlock(&data->mutex);
 
 	return 0;
 }
@@ -1021,13 +1013,10 @@ static int __bmc150_accel_fifo_flush(struct iio_dev *indio_dev,
 static int bmc150_accel_fifo_flush(struct iio_dev *indio_dev, unsigned samples)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
-	int ret;
 
-	mutex_lock(&data->mutex);
-	ret = __bmc150_accel_fifo_flush(indio_dev, samples, false);
-	mutex_unlock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	return ret;
+	return __bmc150_accel_fifo_flush(indio_dev, samples, false);
 }
 
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
@@ -1230,32 +1219,24 @@ static int bmc150_accel_trigger_set_state(struct iio_trigger *trig,
 	struct bmc150_accel_data *data = t->data;
 	int ret;
 
-	mutex_lock(&data->mutex);
+	guard(mutex)(&data->mutex);
 
-	if (t->enabled == state) {
-		mutex_unlock(&data->mutex);
+	if (t->enabled == state)
 		return 0;
-	}
 
 	if (t->setup) {
 		ret = t->setup(t, state);
-		if (ret < 0) {
-			mutex_unlock(&data->mutex);
+		if (ret < 0)
 			return ret;
-		}
 	}
 
 	ret = bmc150_accel_set_interrupt(data, t->intr, state);
-	if (ret < 0) {
-		mutex_unlock(&data->mutex);
+	if (ret < 0)
 		return ret;
-	}
 
 	t->enabled = state;
 
-	mutex_unlock(&data->mutex);
-
-	return ret;
+	return 0;
 }
 
 static const struct iio_trigger_ops bmc150_accel_trigger_ops = {
-- 
2.33.0

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

[Jonathan Cameron]

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;
>  }