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

Sean Nyekjaer posted 5 patches 1 month ago
There is a newer version of this series
[PATCH v3 5/5] iio: imu: inv_icm42600: use guard() to release mutexes
Posted by Sean Nyekjaer 1 month ago
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);
+	}
 
-	mutex_unlock(&st->lock);
 	pm_runtime_put_autosuspend(dev);
 
 	return ret;
@@ -986,16 +986,11 @@ static int inv_icm42600_accel_hwfifo_set_watermark(struct iio_dev *indio_dev,
 						   unsigned int val)
 {
 	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
-	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	st->fifo.watermark.accel = val;
-	ret = inv_icm42600_buffer_update_watermark(st);
-
-	mutex_unlock(&st->lock);
-
-	return ret;
+	return inv_icm42600_buffer_update_watermark(st);
 }
 
 static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
@@ -1007,15 +1002,13 @@ static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
 	if (count == 0)
 		return 0;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = inv_icm42600_buffer_hwfifo_flush(st, count);
-	if (!ret)
-		ret = st->fifo.nb.accel;
-
-	mutex_unlock(&st->lock);
+	if (ret)
+		return ret;
 
-	return ret;
+	return st->fifo.nb.accel;
 }
 
 static int inv_icm42600_accel_read_event_config(struct iio_dev *indio_dev,
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
index 36d69a0face655bf2dc9229c52d07448e9b2ca02..64534ab9e96fd3798bee7eace5a5968eec63570b 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
@@ -283,9 +283,8 @@ static int inv_icm42600_buffer_preenable(struct iio_dev *indio_dev)
 
 	pm_runtime_get_sync(dev);
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	inv_sensors_timestamp_reset(ts);
-	mutex_unlock(&st->lock);
 
 	return 0;
 }
@@ -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++;
-out_unlock:
-	mutex_unlock(&st->lock);
+
 	return ret;
 }
 
@@ -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;
 }
 
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
-		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;
 
 	/* 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;
 
 	/* check wakeup capability */
 	accel_dev = &st->indio_accel->dev;
@@ -898,7 +893,7 @@ static int inv_icm42600_resume(struct device *dev)
 	} else {
 		ret = inv_icm42600_enable_regulator_vddio(st);
 		if (ret)
-			goto out_unlock;
+			return ret;
 	}
 
 	/* restore sensors state */
@@ -906,13 +901,13 @@ static int inv_icm42600_resume(struct device *dev)
 					 st->suspended.accel,
 					 st->suspended.temp, NULL);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	/* restore APEX features if disabled */
 	if (!wakeup && st->apex.wom.enable) {
 		ret = inv_icm42600_enable_wom(st);
 		if (ret)
-			goto out_unlock;
+			return ret;
 	}
 
 	/* restore FIFO data streaming */
@@ -923,9 +918,7 @@ static int inv_icm42600_resume(struct device *dev)
 				   INV_ICM42600_FIFO_CONFIG_STREAM);
 	}
 
-out_unlock:
-	mutex_unlock(&st->lock);
-	return ret;
+	return 0;
 }
 
 /* Runtime suspend will turn off sensors that are enabled by iio devices. */
@@ -934,34 +927,28 @@ static int inv_icm42600_runtime_suspend(struct device *dev)
 	struct inv_icm42600_state *st = dev_get_drvdata(dev);
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	/* disable all sensors */
 	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
 					 INV_ICM42600_SENSOR_MODE_OFF, false,
 					 NULL);
 	if (ret)
-		goto error_unlock;
+		return ret;
 
 	regulator_disable(st->vddio_supply);
 
-error_unlock:
-	mutex_unlock(&st->lock);
-	return ret;
+	return 0;
 }
 
 /* Sensors are enabled by iio devices, no need to turn them back on here. */
 static int inv_icm42600_runtime_resume(struct device *dev)
 {
 	struct inv_icm42600_state *st = dev_get_drvdata(dev);
-	int ret;
 
-	mutex_lock(&st->lock);
-
-	ret = inv_icm42600_enable_regulator_vddio(st);
+	guard(mutex)(&st->lock);
 
-	mutex_unlock(&st->lock);
-	return ret;
+	return inv_icm42600_enable_regulator_vddio(st);
 }
 
 EXPORT_NS_GPL_DEV_PM_OPS(inv_icm42600_pm_ops, IIO_ICM42600) = {
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
index 8a7cc91276319f0b1714ad11d46e409688b258c4..865f6052a4b6278b59390fc7fcefd1bc7ecef076 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
@@ -277,11 +277,11 @@ static int inv_icm42600_gyro_write_scale(struct iio_dev *indio_dev,
 	conf.fs = idx / 2;
 
 	pm_runtime_get_sync(dev);
-	mutex_lock(&st->lock);
 
-	ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
+	scoped_guard(mutex, &st->lock) {
+		ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
+	}
 
-	mutex_unlock(&st->lock);
 	pm_runtime_put_autosuspend(dev);
 
 	return ret;
@@ -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);
 
-	mutex_unlock(&st->lock);
-
 	return ret;
 }
 
@@ -709,15 +707,13 @@ static int inv_icm42600_gyro_hwfifo_flush(struct iio_dev *indio_dev,
 	if (count == 0)
 		return 0;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = inv_icm42600_buffer_hwfifo_flush(st, count);
-	if (!ret)
-		ret = st->fifo.nb.gyro;
-
-	mutex_unlock(&st->lock);
+	if (ret)
+		return ret;
 
-	return ret;
+	return st->fifo.nb.gyro;
 }
 
 static const struct iio_info inv_icm42600_gyro_info = {

-- 
2.50.1
Re: [PATCH v3 5/5] iio: imu: inv_icm42600: use guard() to release mutexes
Posted by David Lechner 3 weeks, 6 days ago
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;
>  }
>
Re: [PATCH v3 5/5] iio: imu: inv_icm42600: use guard() to release mutexes
Posted by Jonathan Cameron 3 weeks, 4 days ago
> ...
> 
> > @@ -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.

There is strong guidance in cleanup.h on basically never mixing gotos
and cleanup (including guard).  It is probably fine here but some compilers
(gcc) are very bad at detecting uninitialized conditions that can happen with
gotos.  More generally Linus has expressed that if you need to mix the two
cleanup.h magic is not appropriate. Following David's suggestion the problem
is solved through duplication of that increment.

> 
> > -out_unlock:
> > -	mutex_unlock(&st->lock);
> > +
> >  	return ret;  
> 
> Can just return 0 here and simplify if (st->fifo.on).