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

[Maxwell Doose]

Hi Gabriel, comments inline.

On Mon, May 25, 2026 at 6:01 AM Gabriel Rondon <grondon@xxxxxxxxx> wrote:
>
> Replace manual mutex_lock()/mutex_unlock() pairs with guard(mutex)
> and scoped_guard() from cleanup.h. This simplifies error paths by
> removing the need for explicit unlock calls before returning.
>
> Most converted functions hold the lock for their entire body, so
> guard(mutex) applies directly. bmc150_accel_trigger_handler() only
> holds the lock around a single register read, so scoped_guard() is
> used there to keep the lock scope unchanged.
>
> Signed-off-by: Gabriel Rondon <grondon@xxxxxxxxx>
> Reviewed-by: Stepan Ionichev <sozdayvek@xxxxxxxxx>
> ---
> Changes since v2:
>  - bmc150_accel_get_fifo_state(): normalize fifo_mode to 0/1 in the
>    sysfs output (data->fifo_mode ? 1 : 0). v2 dropped the bool
>    intermediate and printed the raw mode value, which would emit the
>    FIFO mode bit (0x40) as 64 instead of 0/1. The original 0/1 output
>    is preserved. Reported by Jonathan Cameron.
>  - Collected Reviewed-by from Stepan Ionichev.
>
> Changes since v1:
>  - Drop the verbose list of converted functions from the commit message.
>  - bmc150_accel_get_temp(): keep the original declaration order; no
>    unrelated line movement.
>  - bmc150_accel_get_axis(): convert to guard(mutex) as well, the only
>    code outside the old lock scope was a trivial error check.
>  - bmc150_accel_trigger_handler(): use scoped_guard() so that no manual
>    mutex_lock()/mutex_unlock() pairs are left behind.
>  - bmc150_accel_get_fifo_watermark() / bmc150_accel_get_fifo_state():
>    drop the intermediate variable and return directly.
>
>  drivers/iio/accel/bmc150-accel-core.c | 68 ++++++++-------------------
>  1 file changed, 20 insertions(+), 48 deletions(-)
>
[snip]
> @@ -624,25 +622,21 @@ static int bmc150_accel_get_axis(struct bmc150_accel_data *data,
>         int axis = chan->scan_index;
>         __le16 raw_val;
>
> -       mutex_lock(&data->mutex);
> +       guard(mutex)(&data->mutex);
>         ret = bmc150_accel_set_power_state(data, true);
>

Just a heads up, Jonathan or Andy may ask you to add a blank line in
between the guard()() and the function call below.

>
> -       if (ret < 0) {
> -               mutex_unlock(&data->mutex);
> +       if (ret < 0)
>                 return ret;
> -       }
>
>         ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_AXIS_TO_REG(axis),
>                                &raw_val, sizeof(raw_val));
>         if (ret < 0) {
>                 dev_err(dev, "Error reading axis %d\n", axis);
>                 bmc150_accel_set_power_state(data, false);
> -               mutex_unlock(&data->mutex);
>                 return ret;
>         }
>         *val = sign_extend32(le16_to_cpu(raw_val) >> chan->scan_type.shift,
>                              chan->scan_type.realbits - 1);
>         ret = bmc150_accel_set_power_state(data, false);
> -       mutex_unlock(&data->mutex);
>         if (ret < 0)
>                 return ret;
>
[snip]
> @@ -1187,10 +1168,9 @@ static irqreturn_t bmc150_accel_trigger_handler(int irq, void *p)
>         struct bmc150_accel_data *data = iio_priv(indio_dev);
>         int ret;
>
> -       mutex_lock(&data->mutex);
> -       ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
> -                              data->buffer, AXIS_MAX * 2);
> -       mutex_unlock(&data->mutex);
> +       scoped_guard(mutex, &data->mutex)
> +               ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
> +                                      data->buffer, AXIS_MAX * 2);
>         if (ret < 0)
>                 goto err_read;
>

Another heads up, you may be asked to refactor the critical section
into a helper or do guard()() with {}. So, something like (and not
compiled! Also, don't use this directly, the mail client I'm using to
send this converts tabs to spaces!):

{
        guard(mutex)(&data->mutex);
        ret = regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
                                               data->buffer, AXIS_MAX * 2);
}

or:

static int bmc150_accel_trigger_handler_helper(struct bmc150_accel_data *data)
{
        guard(mutex)(&data->mutex);
        return regmap_bulk_read(data->regmap, BMC150_ACCEL_REG_XOUT_L,
                                                 data->buffer, AXIS_MAX * 2);
}

Obviously, the patch as-is seems correct to me, so

Reviewed-by: Maxwell Doose <m32285159@xxxxxxxxx>

best regards,
max