[PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[Luiz Mugnaini]

From: Luiz Mugnaini <luizmugnaini@usp.br>

Replace mutex_lock()/mutex_unlock() pairs with guard() and
scoped_guard() from cleanup.h for cleaner and safer mutex handling. The
lock protects EEPROM access across five call sites in
mlx90614_read_raw(), mlx90614_write_raw(), and mlx90614_sleep().

In all cases, the code between mutex_unlock() and the end of scope is
either a return statement, or a call to mlx90614_power_put() followed by
trivial computation. In the later cases we prefer the use of
scoped_guard() to avoid holding the lock longer than needed.

Signed-off-by: Luiz Mugnaini <luizmugnaini@usp.br>
---
Changes in v2:
- Prefer using scoped_guard() instead of guard() for cases where we
  don't immediately return. This avoids adding more code to the critical
  section and preserves the same semantics as the previous mutex_lock()
  and mutex_unlock() pairs.
- Removed the intermediate variable ret from mlx_90614_sleep() as
  suggested.

 drivers/iio/temperature/mlx90614.c | 37 ++++++++++++++----------------
 1 file changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index 1ad21b73e..6c58375de 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -23,6 +23,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
@@ -139,6 +140,7 @@ static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
 	return ret;
 }
 
+
 /*
  * Find the IIR value inside iir_values array and return its position
  * which is equivalent to the bit value in sensor register
@@ -296,10 +298,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		mutex_lock(&data->lock);
-		ret = i2c_smbus_read_word_data(data->client,
-					       chip_info->op_eeprom_emissivity);
-		mutex_unlock(&data->lock);
+		scoped_guard(mutex, &data->lock)
+			ret = i2c_smbus_read_word_data(data->client,
+					chip_info->op_eeprom_emissivity);
+
 		mlx90614_power_put(data);
 
 		if (ret < 0)
@@ -319,10 +321,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		mutex_lock(&data->lock);
-		ret = i2c_smbus_read_word_data(data->client,
-					       chip_info->op_eeprom_config1);
-		mutex_unlock(&data->lock);
+		scoped_guard(mutex, &data->lock)
+			ret = i2c_smbus_read_word_data(data->client,
+					chip_info->op_eeprom_config1);
+
 		mlx90614_power_put(data);
 
 		if (ret < 0)
@@ -358,10 +360,10 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		mutex_lock(&data->lock);
-		ret = mlx90614_write_word(data->client,
+		scoped_guard(mutex, &data->lock)
+			ret = mlx90614_write_word(data->client,
 					  chip_info->op_eeprom_emissivity, val);
-		mutex_unlock(&data->lock);
+
 		mlx90614_power_put(data);
 
 		return ret;
@@ -373,10 +375,9 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		mutex_lock(&data->lock);
-		ret = mlx90614_iir_search(data->client,
+		scoped_guard(mutex, &data->lock)
+			ret = mlx90614_iir_search(data->client,
 					  val * 100 + val2 / 10000);
-		mutex_unlock(&data->lock);
 		mlx90614_power_put(data);
 
 		return ret;
@@ -467,7 +468,6 @@ static const struct iio_info mlx90614_info = {
 static int mlx90614_sleep(struct mlx90614_data *data)
 {
 	const struct mlx_chip_info *chip_info = data->chip_info;
-	s32 ret;
 
 	if (!data->wakeup_gpio) {
 		dev_dbg(&data->client->dev, "Sleep disabled");
@@ -476,14 +476,11 @@ static int mlx90614_sleep(struct mlx90614_data *data)
 
 	dev_dbg(&data->client->dev, "Requesting sleep");
 
-	mutex_lock(&data->lock);
-	ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
+	guard(mutex)(&data->lock);
+	return i2c_smbus_xfer(data->client->adapter, data->client->addr,
 			     data->client->flags | I2C_CLIENT_PEC,
 			     I2C_SMBUS_WRITE, chip_info->op_sleep,
 			     I2C_SMBUS_BYTE, NULL);
-	mutex_unlock(&data->lock);
-
-	return ret;
 }
 
 static int mlx90614_wakeup(struct mlx90614_data *data)
-- 
2.53.0

Re: [PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[Andy Shevchenko]

On Mon, Apr 20, 2026 at 11:14:29AM -0300, Luiz Mugnaini wrote:

> Replace mutex_lock()/mutex_unlock() pairs with guard() and
> scoped_guard() from cleanup.h for cleaner and safer mutex handling. The
> lock protects EEPROM access across five call sites in
> mlx90614_read_raw(), mlx90614_write_raw(), and mlx90614_sleep().
> 
> In all cases, the code between mutex_unlock() and the end of scope is
> either a return statement, or a call to mlx90614_power_put() followed by
> trivial computation. In the later cases we prefer the use of
> scoped_guard() to avoid holding the lock longer than needed.

...

> @@ -139,6 +140,7 @@ static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
>  	return ret;
>  }
>  
> +
>  /*
>   * Find the IIR value inside iir_values array and return its position
>   * which is equivalent to the bit value in sensor register

Stray change.

...

>  	dev_dbg(&data->client->dev, "Requesting sleep");

Side note: consider dropping this kind of debug messages. They can be easily
enabled by ftrace infrastructure without being explicitly coded.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking

[Jonathan Cameron]

On Mon, 20 Apr 2026 11:14:29 -0300
Luiz Mugnaini <luizmugnaini@usp.br> wrote:

> From: Luiz Mugnaini <luizmugnaini@usp.br>
> 
> Replace mutex_lock()/mutex_unlock() pairs with guard() and
> scoped_guard() from cleanup.h for cleaner and safer mutex handling. The
> lock protects EEPROM access across five call sites in
> mlx90614_read_raw(), mlx90614_write_raw(), and mlx90614_sleep().
> 
> In all cases, the code between mutex_unlock() and the end of scope is
> either a return statement, or a call to mlx90614_power_put() followed by
> trivial computation. In the later cases we prefer the use of
> scoped_guard() to avoid holding the lock longer than needed.
> 
> Signed-off-by: Luiz Mugnaini <luizmugnaini@usp.br>
Hi Luiz,

Thanks for the patch.

This one is very marginal benefit.

I'm all in favour of guard() usage when it simplifies things a fair bit
because we can have returns where previously goto magic was needed.
When it's just 
mutex_lock();
// oneline
mutex_unlock();

To me the churn is just not worthwhile - these really short lock
regions are not a cause of bugs.

The power management in this particular driver is what makes this
a bad idea.

So sorry, but I won't be picking this one up

Jonathan



> ---
> Changes in v2:
> - Prefer using scoped_guard() instead of guard() for cases where we
>   don't immediately return. This avoids adding more code to the critical
>   section and preserves the same semantics as the previous mutex_lock()
>   and mutex_unlock() pairs.
> - Removed the intermediate variable ret from mlx_90614_sleep() as
>   suggested.
> 
>  drivers/iio/temperature/mlx90614.c | 37 ++++++++++++++----------------
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index 1ad21b73e..6c58375de 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -23,6 +23,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h>
> @@ -139,6 +140,7 @@ static s32 mlx90614_write_word(const struct i2c_client *client, u8 command,
>  	return ret;
>  }
>  
> +
>  /*
>   * Find the IIR value inside iir_values array and return its position
>   * which is equivalent to the bit value in sensor register
> @@ -296,10 +298,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		mutex_lock(&data->lock);
> -		ret = i2c_smbus_read_word_data(data->client,
> -					       chip_info->op_eeprom_emissivity);
> -		mutex_unlock(&data->lock);
> +		scoped_guard(mutex, &data->lock)
> +			ret = i2c_smbus_read_word_data(data->client,
> +					chip_info->op_eeprom_emissivity);
> +
>  		mlx90614_power_put(data);
>  
>  		if (ret < 0)
> @@ -319,10 +321,10 @@ static int mlx90614_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		mutex_lock(&data->lock);
> -		ret = i2c_smbus_read_word_data(data->client,
> -					       chip_info->op_eeprom_config1);
> -		mutex_unlock(&data->lock);
> +		scoped_guard(mutex, &data->lock)
> +			ret = i2c_smbus_read_word_data(data->client,
> +					chip_info->op_eeprom_config1);
> +
>  		mlx90614_power_put(data);
>  
>  		if (ret < 0)
> @@ -358,10 +360,10 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		mutex_lock(&data->lock);
> -		ret = mlx90614_write_word(data->client,
> +		scoped_guard(mutex, &data->lock)
> +			ret = mlx90614_write_word(data->client,
>  					  chip_info->op_eeprom_emissivity, val);
> -		mutex_unlock(&data->lock);
> +
>  		mlx90614_power_put(data);
>  
>  		return ret;
> @@ -373,10 +375,9 @@ static int mlx90614_write_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		mutex_lock(&data->lock);
> -		ret = mlx90614_iir_search(data->client,
> +		scoped_guard(mutex, &data->lock)
> +			ret = mlx90614_iir_search(data->client,
>  					  val * 100 + val2 / 10000);
> -		mutex_unlock(&data->lock);
>  		mlx90614_power_put(data);
>  
>  		return ret;
> @@ -467,7 +468,6 @@ static const struct iio_info mlx90614_info = {
>  static int mlx90614_sleep(struct mlx90614_data *data)
>  {
>  	const struct mlx_chip_info *chip_info = data->chip_info;
> -	s32 ret;
>  
>  	if (!data->wakeup_gpio) {
>  		dev_dbg(&data->client->dev, "Sleep disabled");
> @@ -476,14 +476,11 @@ static int mlx90614_sleep(struct mlx90614_data *data)
>  
>  	dev_dbg(&data->client->dev, "Requesting sleep");
>  
> -	mutex_lock(&data->lock);
> -	ret = i2c_smbus_xfer(data->client->adapter, data->client->addr,
> +	guard(mutex)(&data->lock);
> +	return i2c_smbus_xfer(data->client->adapter, data->client->addr,
>  			     data->client->flags | I2C_CLIENT_PEC,
>  			     I2C_SMBUS_WRITE, chip_info->op_sleep,
>  			     I2C_SMBUS_BYTE, NULL);
> -	mutex_unlock(&data->lock);
> -
> -	return ret;
>  }
>  
>  static int mlx90614_wakeup(struct mlx90614_data *data)