* [PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking
@ 2026-04-20 14:14 Luiz Mugnaini
2026-04-20 17:52 ` Andy Shevchenko
2026-04-21 14:38 ` Jonathan Cameron
0 siblings, 2 replies; 3+ messages in thread
From: Luiz Mugnaini @ 2026-04-20 14:14 UTC (permalink / raw)
To: cmo, jic23, dlechner, nuno.sa, andy; +Cc: Luiz Mugnaini, linux-iio
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking
2026-04-20 14:14 [PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking Luiz Mugnaini
@ 2026-04-20 17:52 ` Andy Shevchenko
2026-04-21 14:38 ` Jonathan Cameron
1 sibling, 0 replies; 3+ messages in thread
From: Andy Shevchenko @ 2026-04-20 17:52 UTC (permalink / raw)
To: Luiz Mugnaini; +Cc: cmo, jic23, dlechner, nuno.sa, andy, linux-iio
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking
2026-04-20 14:14 [PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking Luiz Mugnaini
2026-04-20 17:52 ` Andy Shevchenko
@ 2026-04-21 14:38 ` Jonathan Cameron
1 sibling, 0 replies; 3+ messages in thread
From: Jonathan Cameron @ 2026-04-21 14:38 UTC (permalink / raw)
To: Luiz Mugnaini; +Cc: cmo, dlechner, nuno.sa, andy, linux-iio
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)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-04-21 14:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-20 14:14 [PATCH v2] iio: temperature: mlx90614: use guard(mutex) for EEPROM access locking Luiz Mugnaini
2026-04-20 17:52 ` Andy Shevchenko
2026-04-21 14:38 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).