drivers/iio/magnetometer/bmc150_magn.c | 112 ++++++++++--------------- 1 file changed, 44 insertions(+), 68 deletions(-)
Use guard() and scoped_guard() to replace manual mutex lock/unlock
calls. This simplifies error handling and ensures RAII-style cleanup.
guard() is used in read_raw, write_raw, trig_reen, and
trigger_set_state. Case blocks using guard() in read_raw and write_raw
are wrapped in braces at the case label level to ensure clear scope for
the cleanup guards.
A bmc150_magn_set_power_mode_locked() helper is added to deduplicate
the lock-call-unlock pattern used by remove, runtime_suspend, suspend,
and resume.
The trigger_handler function is left unchanged as mixing guard() with
goto error paths can be fragile.
Signed-off-by: Neel Bullywon <neelb2403@gmail.com>
---
I don't have BMC150 hardware, so I went with Andy's _locked() helper
approach rather than moving the guard into bmc150_magn_set_power_mode()
itself, which would touch the runtime PM locking Jonathan flagged as
suspicious. The deeper runtime PM cleanup can be done separately by
someone who can test it.
v8:
- Add bmc150_magn_set_power_mode_locked() helper to deduplicate the
lock-call-unlock pattern in remove, runtime_suspend, suspend, and
resume (Andy Shevchenko)
v7:
- Split into separate patches (Jonathan Cameron)
v6:
- Remove scoped_guard() from runtime_suspend and use guard() in
suspend/resume (Jonathan Cameron)
v5:
- Use scoped_guard() instead of guard() in remove and
runtime_suspend (Andy Shevchenko)
drivers/iio/magnetometer/bmc150_magn.c | 112 ++++++++++---------------
1 file changed, 44 insertions(+), 68 deletions(-)
diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index 6a73f6e2f1f0..a4e2209f6ed4 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -12,6 +12,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/pm.h>
@@ -255,6 +256,13 @@ static int bmc150_magn_set_power_mode(struct bmc150_magn_data *data,
return -EINVAL;
}
+static int bmc150_magn_set_power_mode_locked(struct bmc150_magn_data *data,
+ enum bmc150_magn_power_modes mode)
+{
+ guard(mutex)(&data->mutex);
+ return bmc150_magn_set_power_mode(data, mode, true);
+}
+
static int bmc150_magn_set_power_state(struct bmc150_magn_data *data, bool on)
{
int ret = 0;
@@ -453,33 +461,29 @@ static int bmc150_magn_read_raw(struct iio_dev *indio_dev,
s32 values[AXIS_XYZ_MAX];
switch (mask) {
- case IIO_CHAN_INFO_RAW:
+ case IIO_CHAN_INFO_RAW: {
if (iio_buffer_enabled(indio_dev))
return -EBUSY;
- mutex_lock(&data->mutex);
+
+ guard(mutex)(&data->mutex);
ret = bmc150_magn_set_power_state(data, true);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
+ 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_set_power_state(data, false);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
+ if (ret < 0)
return ret;
- }
- mutex_unlock(&data->mutex);
return IIO_VAL_INT;
+ }
case IIO_CHAN_INFO_SCALE:
/*
* The API/driver performs an off-chip temperature
@@ -527,48 +531,39 @@ static int bmc150_magn_write_raw(struct iio_dev *indio_dev,
int ret;
switch (mask) {
- case IIO_CHAN_INFO_SAMP_FREQ:
+ case IIO_CHAN_INFO_SAMP_FREQ: {
if (val > data->max_odr)
return -EINVAL;
- mutex_lock(&data->mutex);
- ret = bmc150_magn_set_odr(data, val);
- mutex_unlock(&data->mutex);
- return ret;
+ guard(mutex)(&data->mutex);
+ return bmc150_magn_set_odr(data, val);
+ }
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
switch (chan->channel2) {
case IIO_MOD_X:
- case IIO_MOD_Y:
+ case IIO_MOD_Y: {
if (val < 1 || val > 511)
return -EINVAL;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = bmc150_magn_set_max_odr(data, val, 0, 0);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
+ if (ret < 0)
return ret;
- }
- ret = regmap_update_bits(data->regmap,
+ return regmap_update_bits(data->regmap,
BMC150_MAGN_REG_REP_XY,
BMC150_MAGN_REG_REP_DATAMASK,
- BMC150_MAGN_REPXY_TO_REGVAL
- (val));
- mutex_unlock(&data->mutex);
- return ret;
- case IIO_MOD_Z:
+ BMC150_MAGN_REPXY_TO_REGVAL(val));
+ }
+ case IIO_MOD_Z: {
if (val < 1 || val > 256)
return -EINVAL;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = bmc150_magn_set_max_odr(data, 0, val, 0);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
+ if (ret < 0)
return ret;
- }
- ret = regmap_update_bits(data->regmap,
+ return regmap_update_bits(data->regmap,
BMC150_MAGN_REG_REP_Z,
BMC150_MAGN_REG_REP_DATAMASK,
- BMC150_MAGN_REPZ_TO_REGVAL
- (val));
- mutex_unlock(&data->mutex);
- return ret;
+ BMC150_MAGN_REPZ_TO_REGVAL(val));
+ }
default:
return -EINVAL;
}
@@ -782,9 +777,8 @@ static void bmc150_magn_trig_reen(struct iio_trigger *trig)
if (!data->dready_trigger_on)
return;
- mutex_lock(&data->mutex);
+ guard(mutex)(&data->mutex);
ret = bmc150_magn_reset_intr(data);
- mutex_unlock(&data->mutex);
if (ret)
dev_err(data->dev, "Failed to reset interrupt\n");
}
@@ -794,32 +788,28 @@ static int bmc150_magn_data_rdy_trigger_set_state(struct iio_trigger *trig,
{
struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
struct bmc150_magn_data *data = iio_priv(indio_dev);
- int ret = 0;
+ int ret;
+
+ guard(mutex)(&data->mutex);
- mutex_lock(&data->mutex);
if (state == data->dready_trigger_on)
- goto err_unlock;
+ return 0;
ret = regmap_update_bits(data->regmap, BMC150_MAGN_REG_INT_DRDY,
BMC150_MAGN_MASK_DRDY_EN,
state << BMC150_MAGN_SHIFT_DRDY_EN);
if (ret < 0)
- goto err_unlock;
+ return ret;
data->dready_trigger_on = state;
if (state) {
ret = bmc150_magn_reset_intr(data);
if (ret < 0)
- goto err_unlock;
+ return ret;
}
- mutex_unlock(&data->mutex);
return 0;
-
-err_unlock:
- mutex_unlock(&data->mutex);
- return ret;
}
static const struct iio_trigger_ops bmc150_magn_trigger_ops = {
@@ -980,9 +970,7 @@ void bmc150_magn_remove(struct device *dev)
if (data->dready_trig)
iio_trigger_unregister(data->dready_trig);
- mutex_lock(&data->mutex);
- bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SUSPEND, true);
- mutex_unlock(&data->mutex);
+ bmc150_magn_set_power_mode_locked(data, BMC150_MAGN_POWER_MODE_SUSPEND);
regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
}
@@ -995,10 +983,8 @@ static int bmc150_magn_runtime_suspend(struct device *dev)
struct bmc150_magn_data *data = iio_priv(indio_dev);
int ret;
- mutex_lock(&data->mutex);
- ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SLEEP,
- true);
- mutex_unlock(&data->mutex);
+ ret = bmc150_magn_set_power_mode_locked(data,
+ BMC150_MAGN_POWER_MODE_SLEEP);
if (ret < 0) {
dev_err(dev, "powering off device failed\n");
return ret;
@@ -1024,28 +1010,18 @@ static int bmc150_magn_suspend(struct device *dev)
{
struct iio_dev *indio_dev = dev_get_drvdata(dev);
struct bmc150_magn_data *data = iio_priv(indio_dev);
- int ret;
- mutex_lock(&data->mutex);
- ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_SLEEP,
- true);
- mutex_unlock(&data->mutex);
-
- return ret;
+ return bmc150_magn_set_power_mode_locked(data,
+ BMC150_MAGN_POWER_MODE_SLEEP);
}
static int bmc150_magn_resume(struct device *dev)
{
struct iio_dev *indio_dev = dev_get_drvdata(dev);
struct bmc150_magn_data *data = iio_priv(indio_dev);
- int ret;
- mutex_lock(&data->mutex);
- ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
- true);
- mutex_unlock(&data->mutex);
-
- return ret;
+ return bmc150_magn_set_power_mode_locked(data,
+ BMC150_MAGN_POWER_MODE_NORMAL);
}
#endif
--
2.44.0On Sat, 28 Feb 2026 12:23:20 -0500
Neel Bullywon <neelb2403@gmail.com> wrote:
> Use guard() and scoped_guard() to replace manual mutex lock/unlock
> calls. This simplifies error handling and ensures RAII-style cleanup.
>
> guard() is used in read_raw, write_raw, trig_reen, and
> trigger_set_state. Case blocks using guard() in read_raw and write_raw
> are wrapped in braces at the case label level to ensure clear scope for
> the cleanup guards.
>
> A bmc150_magn_set_power_mode_locked() helper is added to deduplicate
> the lock-call-unlock pattern used by remove, runtime_suspend, suspend,
> and resume.
>
> The trigger_handler function is left unchanged as mixing guard() with
> goto error paths can be fragile.
>
> Signed-off-by: Neel Bullywon <neelb2403@gmail.com>
Hi Neel
LGTM, but I'll leave some time for Andy to take another look if he
wants to.
One comment inline on a possible follow up bit of driver cleanup.
Jonathan
> ---
> I don't have BMC150 hardware, so I went with Andy's _locked() helper
> approach rather than moving the guard into bmc150_magn_set_power_mode()
> itself, which would touch the runtime PM locking Jonathan flagged as
> suspicious. The deeper runtime PM cleanup can be done separately by
> someone who can test it.
>
> v8:
> - Add bmc150_magn_set_power_mode_locked() helper to deduplicate the
> lock-call-unlock pattern in remove, runtime_suspend, suspend, and
> resume (Andy Shevchenko)
> v7:
> - Split into separate patches (Jonathan Cameron)
> v6:
> - Remove scoped_guard() from runtime_suspend and use guard() in
> suspend/resume (Jonathan Cameron)
> v5:
> - Use scoped_guard() instead of guard() in remove and
> runtime_suspend (Andy Shevchenko)
>
> drivers/iio/magnetometer/bmc150_magn.c | 112 ++++++++++---------------
> 1 file changed, 44 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index 6a73f6e2f1f0..a4e2209f6ed4 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -794,32 +788,28 @@ static int bmc150_magn_data_rdy_trigger_set_state(struct iio_trigger *trig,
> {
> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> struct bmc150_magn_data *data = iio_priv(indio_dev);
> - int ret = 0;
> + int ret;
> +
> + guard(mutex)(&data->mutex);
>
> - mutex_lock(&data->mutex);
> if (state == data->dready_trigger_on)
> - goto err_unlock;
> + return 0;
>
> ret = regmap_update_bits(data->regmap, BMC150_MAGN_REG_INT_DRDY,
> BMC150_MAGN_MASK_DRDY_EN,
> state << BMC150_MAGN_SHIFT_DRDY_EN);
A nice follow up might be to look for all cases like this in this driver
and make them
ret = regmap_assign_bits(data->regmap, BMC150_MAGN_REG_INT_DRDY,
BMC150_MAGN_MASK_DRDY_EN, state);
And drop that shift define.
> if (ret < 0)
> - goto err_unlock;
> + return ret;
>
> data->dready_trigger_on = state;
>
> if (state) {
> ret = bmc150_magn_reset_intr(data);
> if (ret < 0)
> - goto err_unlock;
> + return ret;
> }
> - mutex_unlock(&data->mutex);
>
> return 0;
> -
> -err_unlock:
> - mutex_unlock(&data->mutex);
> - return ret;
> }
On Sun, Mar 01, 2026 at 11:56:33AM +0000, Jonathan Cameron wrote: > On Sat, 28 Feb 2026 12:23:20 -0500 > Neel Bullywon <neelb2403@gmail.com> wrote: > > > Use guard() and scoped_guard() to replace manual mutex lock/unlock > > calls. This simplifies error handling and ensures RAII-style cleanup. > > > > guard() is used in read_raw, write_raw, trig_reen, and > > trigger_set_state. Case blocks using guard() in read_raw and write_raw > > are wrapped in braces at the case label level to ensure clear scope for > > the cleanup guards. > > > > A bmc150_magn_set_power_mode_locked() helper is added to deduplicate > > the lock-call-unlock pattern used by remove, runtime_suspend, suspend, > > and resume. > > > > The trigger_handler function is left unchanged as mixing guard() with > > goto error paths can be fragile. > > > > Signed-off-by: Neel Bullywon <neelb2403@gmail.com> > Hi Neel > > LGTM, but I'll leave some time for Andy to take another look if he > wants to. I briefly looked and I'm fine with the code, but I haven't reviewed it fully, hence Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com> -- With Best Regards, Andy Shevchenko
On Mon, 2 Mar 2026 10:21:11 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Sun, Mar 01, 2026 at 11:56:33AM +0000, Jonathan Cameron wrote: > > On Sat, 28 Feb 2026 12:23:20 -0500 > > Neel Bullywon <neelb2403@gmail.com> wrote: > > > > > Use guard() and scoped_guard() to replace manual mutex lock/unlock > > > calls. This simplifies error handling and ensures RAII-style cleanup. > > > > > > guard() is used in read_raw, write_raw, trig_reen, and > > > trigger_set_state. Case blocks using guard() in read_raw and write_raw > > > are wrapped in braces at the case label level to ensure clear scope for > > > the cleanup guards. > > > > > > A bmc150_magn_set_power_mode_locked() helper is added to deduplicate > > > the lock-call-unlock pattern used by remove, runtime_suspend, suspend, > > > and resume. > > > > > > The trigger_handler function is left unchanged as mixing guard() with > > > goto error paths can be fragile. > > > > > > Signed-off-by: Neel Bullywon <neelb2403@gmail.com> > > Hi Neel > > > > LGTM, but I'll leave some time for Andy to take another look if he > > wants to. > > I briefly looked and I'm fine with the code, but I haven't reviewed it fully, > hence > > Acked-by: Andy Shevchenko <andriy.shevchenko@intel.com> > Applied. Thanks J
© 2016 - 2026 Red Hat, Inc.