drivers/iio/chemical/sps30.c | 60 +++++++++++++++++------------------- 1 file changed, 29 insertions(+), 31 deletions(-)
Replace manual mutex_lock() and mutex_unlock() calls with the much newer
guard(mutex)() and scoped_guard() macros to enable RAII patterns,
modernize the driver, and to increase readability.
Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
drivers/iio/chemical/sps30.c | 60 +++++++++++++++++-------------------
1 file changed, 29 insertions(+), 31 deletions(-)
diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
index a934bf0298dd..186dec4cfd78 100644
--- a/drivers/iio/chemical/sps30.c
+++ b/drivers/iio/chemical/sps30.c
@@ -5,6 +5,7 @@
* Copyright (c) Tomasz Duszynski <tduszyns@gmail.com>
*/
+#include <linux/cleanup.h>
#include <linux/crc8.h>
#include <linux/delay.h>
#include <linux/i2c.h>
@@ -111,9 +112,9 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)
aligned_s64 ts;
} scan;
- mutex_lock(&state->lock);
- ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
- mutex_unlock(&state->lock);
+ scoped_guard(mutex, &state->lock)
+ ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
+
if (ret)
goto err;
@@ -136,23 +137,23 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_PROCESSED:
switch (chan->type) {
case IIO_MASSCONCENTRATION:
- mutex_lock(&state->lock);
- /* read up to the number of bytes actually needed */
- switch (chan->channel2) {
- case IIO_MOD_PM1:
- ret = sps30_do_meas(state, data, 1);
- break;
- case IIO_MOD_PM2P5:
- ret = sps30_do_meas(state, data, 2);
- break;
- case IIO_MOD_PM4:
- ret = sps30_do_meas(state, data, 3);
- break;
- case IIO_MOD_PM10:
- ret = sps30_do_meas(state, data, 4);
- break;
+ scoped_guard(mutex, &state->lock) {
+ /* read up to the number of bytes actually needed */
+ switch (chan->channel2) {
+ case IIO_MOD_PM1:
+ ret = sps30_do_meas(state, data, 1);
+ break;
+ case IIO_MOD_PM2P5:
+ ret = sps30_do_meas(state, data, 2);
+ break;
+ case IIO_MOD_PM4:
+ ret = sps30_do_meas(state, data, 3);
+ break;
+ case IIO_MOD_PM10:
+ ret = sps30_do_meas(state, data, 4);
+ break;
+ }
}
- mutex_unlock(&state->lock);
if (ret)
return ret;
@@ -197,9 +198,9 @@ static ssize_t start_cleaning_store(struct device *dev,
if (kstrtoint(buf, 0, &val) || val != 1)
return -EINVAL;
- mutex_lock(&state->lock);
- ret = state->ops->clean_fan(state);
- mutex_unlock(&state->lock);
+ scoped_guard(mutex, &state->lock)
+ ret = state->ops->clean_fan(state);
+
if (ret)
return ret;
@@ -215,9 +216,9 @@ static ssize_t cleaning_period_show(struct device *dev,
__be32 val;
int ret;
- mutex_lock(&state->lock);
- ret = state->ops->read_cleaning_period(state, &val);
- mutex_unlock(&state->lock);
+ scoped_guard(mutex, &state->lock)
+ ret = state->ops->read_cleaning_period(state, &val);
+
if (ret)
return ret;
@@ -238,12 +239,11 @@ static ssize_t cleaning_period_store(struct device *dev, struct device_attribute
(val > SPS30_AUTO_CLEANING_PERIOD_MAX))
return -EINVAL;
- mutex_lock(&state->lock);
+ guard(mutex)(&state->lock);
+
ret = state->ops->write_cleaning_period(state, cpu_to_be32(val));
- if (ret) {
- mutex_unlock(&state->lock);
+ if (ret)
return ret;
- }
msleep(20);
@@ -256,8 +256,6 @@ static ssize_t cleaning_period_store(struct device *dev, struct device_attribute
dev_warn(dev,
"period changed but reads will return the old value\n");
- mutex_unlock(&state->lock);
-
return len;
}
--
2.54.0On Sat, May 09, 2026 at 07:52:00AM -0500, Maxwell Doose wrote: > Replace manual mutex_lock() and mutex_unlock() calls with the much newer > guard(mutex)() and scoped_guard() macros to enable RAII patterns, > modernize the driver, and to increase readability. ... > - mutex_lock(&state->lock); > - ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data)); > - mutex_unlock(&state->lock); > + scoped_guard(mutex, &state->lock) > + ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data)); > + It's the same mistake already had been told many times, do not add blank line to the coupled statements (usually assignment followed by a check). > if (ret) > goto err; And looking at the code, it's rather better to have this lock inside sps30_do_meas() (it might require to create a locked wrapper as sps30_do_meas() while renaming the current one to __sps30_do_meas() to clarify locking rules). -- With Best Regards, Andy Shevchenko
On Sun, May 10, 2026 at 1:54 AM Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Sat, May 09, 2026 at 07:52:00AM -0500, Maxwell Doose wrote: > > Replace manual mutex_lock() and mutex_unlock() calls with the much newer > > guard(mutex)() and scoped_guard() macros to enable RAII patterns, > > modernize the driver, and to increase readability. > > ... > > > - mutex_lock(&state->lock); > > - ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data)); > > - mutex_unlock(&state->lock); > > + scoped_guard(mutex, &state->lock) > > + ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data)); > > > + > > It's the same mistake already had been told many times, do not add blank line > to the coupled statements (usually assignment followed by a check). > Sorry! I tend to use that style for some of my other things. Will fix for v2 (I have to send that anyways). > > > if (ret) > > goto err; > > And looking at the code, it's rather better to have this lock inside > sps30_do_meas() (it might require to create a locked wrapper as > sps30_do_meas() while renaming the current one to __sps30_do_meas() > to clarify locking rules). > Sounds good, can also add for v2. best regards, max > > -- > With Best Regards, > Andy Shevchenko > >
On 5/9/26 7:52 AM, Maxwell Doose wrote:
> Replace manual mutex_lock() and mutex_unlock() calls with the much newer
> guard(mutex)() and scoped_guard() macros to enable RAII patterns,
> modernize the driver, and to increase readability.
>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
> drivers/iio/chemical/sps30.c | 60 +++++++++++++++++-------------------
> 1 file changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> index a934bf0298dd..186dec4cfd78 100644
> --- a/drivers/iio/chemical/sps30.c
> +++ b/drivers/iio/chemical/sps30.c
> @@ -5,6 +5,7 @@
> * Copyright (c) Tomasz Duszynski <tduszyns@gmail.com>
> */
>
> +#include <linux/cleanup.h>
> #include <linux/crc8.h>
> #include <linux/delay.h>
> #include <linux/i2c.h>
> @@ -111,9 +112,9 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)
> aligned_s64 ts;
> } scan;
>
> - mutex_lock(&state->lock);
> - ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
> - mutex_unlock(&state->lock);
> + scoped_guard(mutex, &state->lock)
> + ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
> +
> if (ret)
> goto err;
>
> @@ -136,23 +137,23 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_MASSCONCENTRATION:
> - mutex_lock(&state->lock);
> - /* read up to the number of bytes actually needed */
> - switch (chan->channel2) {
> - case IIO_MOD_PM1:
> - ret = sps30_do_meas(state, data, 1);
> - break;
> - case IIO_MOD_PM2P5:
> - ret = sps30_do_meas(state, data, 2);
> - break;
> - case IIO_MOD_PM4:
> - ret = sps30_do_meas(state, data, 3);
> - break;
> - case IIO_MOD_PM10:
> - ret = sps30_do_meas(state, data, 4);
> - break;
We can do it like this:
case IIO_MASSCONCENTRATION: {
guard(mutex)(&state->lock);
/* read up to the number of bytes actually needed */
switch (chan->channel2) {
case IIO_MOD_PM1:
ret = sps30_do_meas(state, data, 1);
break;
case IIO_MOD_PM2P5:
ret = sps30_do_meas(state, data, 2);
break;
case IIO_MOD_PM4:
ret = sps30_do_meas(state, data, 3);
break;
case IIO_MOD_PM10:
ret = sps30_do_meas(state, data, 4);
break;
default:
return -EINVAL;
}
if (ret)
return ret;
*val = data[chan->address] / 100;
*val2 = (data[chan->address] % 100) * 10000;
return IIO_VAL_INT_PLUS_MICRO;
}
default:
return -EINVAL;
Make less indent and we don't have to initialize ret at the start of
the function anymore.
> + scoped_guard(mutex, &state->lock) {
> + /* read up to the number of bytes actually needed */
> + switch (chan->channel2) {
> + case IIO_MOD_PM1:
> + ret = sps30_do_meas(state, data, 1);
> + break;
> + case IIO_MOD_PM2P5:
> + ret = sps30_do_meas(state, data, 2);
> + break;
> + case IIO_MOD_PM4:
> + ret = sps30_do_meas(state, data, 3);
> + break;
> + case IIO_MOD_PM10:
> + ret = sps30_do_meas(state, data, 4);
> + break;
> + }
> }
> - mutex_unlock(&state->lock);
> if (ret)
> return ret;
>
> @@ -197,9 +198,9 @@ static ssize_t start_cleaning_store(struct device *dev,
> if (kstrtoint(buf, 0, &val) || val != 1)
> return -EINVAL;
>
> - mutex_lock(&state->lock);
> - ret = state->ops->clean_fan(state);
> - mutex_unlock(&state->lock);
> + scoped_guard(mutex, &state->lock)
Doesn't need to be scoped. there is just return after this.
> + ret = state->ops->clean_fan(state);
> +
> if (ret)
> return ret;
>
> @@ -215,9 +216,9 @@ static ssize_t cleaning_period_show(struct device *dev,
> __be32 val;
> int ret;
>
> - mutex_lock(&state->lock);
> - ret = state->ops->read_cleaning_period(state, &val);
> - mutex_unlock(&state->lock);
> + scoped_guard(mutex, &state->lock)
> + ret = state->ops->read_cleaning_period(state, &val);
> +
IMHO, this one just make the code uglier, not really an improvement.
> if (ret)
> return ret;
>
On Sat, May 9, 2026 at 4:22 PM David Lechner <dlechner@baylibre.com> wrote: [snip] > Doesn't need to be scoped. there is just return after this. > > > + ret = state->ops->clean_fan(state); > > + > > if (ret) > > return ret; > > Ah. Idea was to keep lengths of critical sections as close as possible to what they were before just to be safe. Will change all of those over for v2. best regards, max > > @@ -215,9 +216,9 @@ static ssize_t cleaning_period_show(struct device *dev, > > __be32 val; > > int ret; > > > > - mutex_lock(&state->lock); > > - ret = state->ops->read_cleaning_period(state, &val); > > - mutex_unlock(&state->lock); > > + scoped_guard(mutex, &state->lock) > > + ret = state->ops->read_cleaning_period(state, &val); > > + > > IMHO, this one just make the code uglier, not really an improvement. > > > if (ret) > > return ret; > >
On Sat, 9 May 2026 at 14:53, Maxwell Doose <m32285159@gmail.com> wrote:
>
> Replace manual mutex_lock() and mutex_unlock() calls with the much newer
> guard(mutex)() and scoped_guard() macros to enable RAII patterns,
> modernize the driver, and to increase readability.
>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
> drivers/iio/chemical/sps30.c | 60 +++++++++++++++++-------------------
> 1 file changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> index a934bf0298dd..186dec4cfd78 100644
> --- a/drivers/iio/chemical/sps30.c
> +++ b/drivers/iio/chemical/sps30.c
> @@ -5,6 +5,7 @@
> * Copyright (c) Tomasz Duszynski <tduszyns@gmail.com>
> */
>
> +#include <linux/cleanup.h>
> #include <linux/crc8.h>
> #include <linux/delay.h>
> #include <linux/i2c.h>
> @@ -111,9 +112,9 @@ static irqreturn_t sps30_trigger_handler(int irq, void *p)
> aligned_s64 ts;
> } scan;
>
> - mutex_lock(&state->lock);
> - ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
> - mutex_unlock(&state->lock);
> + scoped_guard(mutex, &state->lock)
> + ret = sps30_do_meas(state, scan.data, ARRAY_SIZE(scan.data));
> +
> if (ret)
> goto err;
>
> @@ -136,23 +137,23 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_MASSCONCENTRATION:
> - mutex_lock(&state->lock);
> - /* read up to the number of bytes actually needed */
> - switch (chan->channel2) {
> - case IIO_MOD_PM1:
> - ret = sps30_do_meas(state, data, 1);
> - break;
> - case IIO_MOD_PM2P5:
> - ret = sps30_do_meas(state, data, 2);
> - break;
> - case IIO_MOD_PM4:
> - ret = sps30_do_meas(state, data, 3);
> - break;
> - case IIO_MOD_PM10:
> - ret = sps30_do_meas(state, data, 4);
> - break;
> + scoped_guard(mutex, &state->lock) {
> + /* read up to the number of bytes actually needed */
> + switch (chan->channel2) {
> + case IIO_MOD_PM1:
> + ret = sps30_do_meas(state, data, 1);
> + break;
> + case IIO_MOD_PM2P5:
> + ret = sps30_do_meas(state, data, 2);
> + break;
> + case IIO_MOD_PM4:
> + ret = sps30_do_meas(state, data, 3);
> + break;
> + case IIO_MOD_PM10:
> + ret = sps30_do_meas(state, data, 4);
> + break;
> + }
> }
> - mutex_unlock(&state->lock);
> if (ret)
> return ret;
>
> @@ -197,9 +198,9 @@ static ssize_t start_cleaning_store(struct device *dev,
> if (kstrtoint(buf, 0, &val) || val != 1)
> return -EINVAL;
>
> - mutex_lock(&state->lock);
> - ret = state->ops->clean_fan(state);
> - mutex_unlock(&state->lock);
> + scoped_guard(mutex, &state->lock)
> + ret = state->ops->clean_fan(state);
> +
> if (ret)
> return ret;
>
> @@ -215,9 +216,9 @@ static ssize_t cleaning_period_show(struct device *dev,
> __be32 val;
> int ret;
>
> - mutex_lock(&state->lock);
> - ret = state->ops->read_cleaning_period(state, &val);
> - mutex_unlock(&state->lock);
> + scoped_guard(mutex, &state->lock)
> + ret = state->ops->read_cleaning_period(state, &val);
> +
> if (ret)
> return ret;
>
> @@ -238,12 +239,11 @@ static ssize_t cleaning_period_store(struct device *dev, struct device_attribute
> (val > SPS30_AUTO_CLEANING_PERIOD_MAX))
> return -EINVAL;
>
> - mutex_lock(&state->lock);
> + guard(mutex)(&state->lock);
> +
> ret = state->ops->write_cleaning_period(state, cpu_to_be32(val));
> - if (ret) {
> - mutex_unlock(&state->lock);
> + if (ret)
> return ret;
> - }
>
> msleep(20);
>
> @@ -256,8 +256,6 @@ static ssize_t cleaning_period_store(struct device *dev, struct device_attribute
> dev_warn(dev,
> "period changed but reads will return the old value\n");
>
> - mutex_unlock(&state->lock);
> -
> return len;
> }
>
> --
> 2.54.0
>
>
LGTM.
Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
--
Kind regards
CJD
Hi Joshua, On Sat, May 9, 2026 at 8:04 AM Joshua Crofts <joshua.crofts1@gmail.com> wrote: [snip] > > LGTM. > > Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com> > David asked me to make some changes. Will I still be good to add your rb or do you want to wait and see? best regards, max > -- > Kind regards > > CJD
On Sun, 10 May 2026 at 01:55, Maxwell Doose <m32285159@gmail.com> wrote: > > Hi Joshua, > > On Sat, May 9, 2026 at 8:04 AM Joshua Crofts <joshua.crofts1@gmail.com> wrote: > [snip] > > > > LGTM. > > > > Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com> > > > > David asked me to make some changes. Will I still be good to add your > rb or do you want to wait and see? Hi Max, Yes, you can keep my rb, I agree with David's changes. -- Kind regards CJD
© 2016 - 2026 Red Hat, Inc.