Re: [PATCH v3 5/5] iio: imu: inv_icm42600: use guard() to release mutexes

[David Lechner <dlechner@baylibre.com>]

On 9/1/25 2:49 AM, Sean Nyekjaer wrote:
> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
> for cleaner and safer mutex handling.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 25 +++------
>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 27 ++++-----
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   | 65 +++++++++-------------
>  drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c   | 20 +++----
>  4 files changed, 55 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 48014b61ced335eb2c8549cfc2e79ccde1934308..fbed6974ef04ac003c9b7bd38f87bd77f4b55509 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -561,11 +561,11 @@ static int inv_icm42600_accel_write_scale(struct iio_dev *indio_dev,
>  	conf.fs = idx / 2;
>  
>  	pm_runtime_get_sync(dev);
> -	mutex_lock(&st->lock);
>  
> -	ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
> +	scoped_guard(mutex, &st->lock) {
> +		ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
> +	}

Don't need braces here.

>  
> -	mutex_unlock(&st->lock);
>  	pm_runtime_put_autosuspend(dev);
>  
>  	return ret;


...

> @@ -299,7 +298,7 @@ static int inv_icm42600_buffer_postenable(struct iio_dev *indio_dev)
>  	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  
>  	/* exit if FIFO is already on */
>  	if (st->fifo.on) {
> @@ -311,30 +310,29 @@ static int inv_icm42600_buffer_postenable(struct iio_dev *indio_dev)
>  	ret = regmap_set_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,
>  			      INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  	/* flush FIFO data */
>  	ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
>  			   INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  	/* set FIFO in streaming mode */
>  	ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  			   INV_ICM42600_FIFO_CONFIG_STREAM);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  	/* workaround: first read of FIFO count after reset is always 0 */
>  	ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT, st->buffer, 2);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  out_on:
>  	/* increase FIFO on counter */
>  	st->fifo.on++;

I would be tempted to get rid of out_on as well even if we have to repeat
`st->fifo.on++;` in two places.

> -out_unlock:
> -	mutex_unlock(&st->lock);
> +
>  	return ret;

Can just return 0 here and simplify if (st->fifo.on).

>  }
>  
> @@ -343,7 +341,7 @@ static int inv_icm42600_buffer_predisable(struct iio_dev *indio_dev)
>  	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  
>  	/* exit if there are several sensors using the FIFO */
>  	if (st->fifo.on > 1) {
> @@ -355,25 +353,24 @@ static int inv_icm42600_buffer_predisable(struct iio_dev *indio_dev)
>  	ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  			   INV_ICM42600_FIFO_CONFIG_BYPASS);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  	/* flush FIFO data */
>  	ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
>  			   INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  	/* disable FIFO threshold interrupt */
>  	ret = regmap_clear_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,
>  				INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  out_off:
>  	/* decrease FIFO on counter */
>  	st->fifo.on--;
> -out_unlock:
> -	mutex_unlock(&st->lock);
> +
>  	return ret;

Same comments apply here.

>  }
>  
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 4bf436c46f1cfd7e7e1bb911d94a0a566d63e791..4db8bc68075a30c59e6e358bb0b73b1e6b9175ea 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -439,18 +439,13 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
>  			     unsigned int writeval, unsigned int *readval)
>  {
>  	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> -	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  
>  	if (readval)
> -		ret = regmap_read(st->map, reg, readval);
> +		return regmap_read(st->map, reg, readval);
>  	else

Don't need the `else` anymore.

> -		ret = regmap_write(st->map, reg, writeval);
> -
> -	mutex_unlock(&st->lock);
> -
> -	return ret;
> +		return regmap_write(st->map, reg, writeval);
>  }
>  
>  static int inv_icm42600_set_conf(struct inv_icm42600_state *st,
> @@ -820,22 +815,23 @@ static int inv_icm42600_suspend(struct device *dev)
>  	struct device *accel_dev;
>  	bool wakeup;
>  	int accel_conf;
> -	int ret = 0;
> +	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  
>  	st->suspended.gyro = st->conf.gyro.mode;
>  	st->suspended.accel = st->conf.accel.mode;
>  	st->suspended.temp = st->conf.temp_en;
> -	if (pm_runtime_suspended(dev))
> -		goto out_unlock;
> +	ret = pm_runtime_suspended(dev);
> +	if (ret)
> +		return ret;

pm_runtime_suspended() returns a bool, so this doesn't make sense.

Probably should be:

	if (pm_runtime_suspended(dev))
		return 0;

>  
>  	/* disable FIFO data streaming */
>  	if (st->fifo.on) {
>  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  				   INV_ICM42600_FIFO_CONFIG_BYPASS);
>  		if (ret)
> -			goto out_unlock;
> +			return ret;
>  	}
>  
>  	/* keep chip on and wake-up capable if APEX and wakeup on */
> @@ -851,7 +847,7 @@ static int inv_icm42600_suspend(struct device *dev)
>  		if (st->apex.wom.enable) {
>  			ret = inv_icm42600_disable_wom(st);
>  			if (ret)
> -				goto out_unlock;
> +				return ret;
>  		}
>  		accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
>  	}
> @@ -859,15 +855,13 @@ static int inv_icm42600_suspend(struct device *dev)
>  	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
>  					 accel_conf, false, NULL);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
>  	/* disable vddio regulator if chip is sleeping */
>  	if (!wakeup)
>  		regulator_disable(st->vddio_supply);
>  
> -out_unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /*
> @@ -881,12 +875,13 @@ static int inv_icm42600_resume(struct device *dev)
>  	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
>  	struct device *accel_dev;
>  	bool wakeup;
> -	int ret = 0;
> +	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  
> -	if (pm_runtime_suspended(dev))
> -		goto out_unlock;
> +	ret = pm_runtime_suspended(dev);
> +	if (ret)
> +		return ret;

same here.

>  
>  	/* check wakeup capability */
>  	accel_dev = &st->indio_accel->dev;

...

> @@ -690,13 +690,11 @@ static int inv_icm42600_gyro_hwfifo_set_watermark(struct iio_dev *indio_dev,
>  	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  
>  	st->fifo.watermark.gyro = val;
>  	ret = inv_icm42600_buffer_update_watermark(st);

Can return directly now.

>  
> -	mutex_unlock(&st->lock);
> -
>  	return ret;
>  }
>