[PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[Maxwell Doose]

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

Re: [PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[Joshua Crofts]

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

Re: [PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[David Lechner]

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;
>  

Re: [PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[Maxwell Doose]

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;
> >

Re: [PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[Maxwell Doose]

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

Re: [PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[Andy Shevchenko]

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).

>  	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

Re: [PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[Maxwell Doose]

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
>
>

Re: [PATCH] iio: chemical: sps30: Replace manual locking with RAII locking

[Joshua Crofts]

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