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

[Jonathan Cameron <jic23@kernel.org>]

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)