Re: [PATCH v8] iio: magnetometer: bmc150_magn: use automated cleanup for mutex

[Jonathan Cameron]

On Sat, 28 Feb 2026 12:23:20 -0500
Neel Bullywon <neelb2403@xxxxxxxxx> 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@xxxxxxxxx>
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;
>  }