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

[PATCH v3] 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>
---
v2:
 - Switch over some scoped_guard()s to guard(mutex)() per David's
   suggestion.
 - Remove redundant whitespace per Andy's suggestion.
 - Add new wrapper sps30_do_meas() per Andy's suggestion (see commit
   message).
 - Add Joshua's RB
   (link: https://lore.kernel.org/linux-iio/CAKqfh0FWig8mRR-xhvnfcFeSinR6RySyPaf9Gbpb6WU+diiiUQ@mail.gmail.com/T/#t)

v3:
 - Remove sps30_do_meas() wrapper per Andy's suggestion.
 - Remove Joshua's RB (major changes).

 drivers/iio/chemical/sps30.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
index a934bf0298dd..9cb201cf0f15 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,8 @@ 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;
 
@@ -135,8 +135,8 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_PROCESSED:
 		switch (chan->type) {
-		case IIO_MASSCONCENTRATION:
-			mutex_lock(&state->lock);
+		case IIO_MASSCONCENTRATION: {
+			guard(mutex)(&state->lock);
 			/* read up to the number of bytes actually needed */
 			switch (chan->channel2) {
 			case IIO_MOD_PM1:
@@ -152,7 +152,6 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
 				ret = sps30_do_meas(state, data, 4);
 				break;
 			}
-			mutex_unlock(&state->lock);
 			if (ret)
 				return ret;
 
@@ -160,6 +159,7 @@ static int sps30_read_raw(struct iio_dev *indio_dev,
 			*val2 = (data[chan->address] % 100) * 10000;
 
 			return IIO_VAL_INT_PLUS_MICRO;
+		}
 		default:
 			return -EINVAL;
 		}
@@ -197,9 +197,9 @@ static ssize_t start_cleaning_store(struct device *dev,
 	if (kstrtoint(buf, 0, &val) || val != 1)
 		return -EINVAL;
 
-	mutex_lock(&state->lock);
+	guard(mutex)(&state->lock);
+
 	ret = state->ops->clean_fan(state);
-	mutex_unlock(&state->lock);
 	if (ret)
 		return ret;
 
@@ -215,9 +215,9 @@ static ssize_t cleaning_period_show(struct device *dev,
 	__be32 val;
 	int ret;
 
-	mutex_lock(&state->lock);
+	guard(mutex)(&state->lock);
+
 	ret = state->ops->read_cleaning_period(state, &val);
-	mutex_unlock(&state->lock);
 	if (ret)
 		return ret;
 
@@ -238,12 +238,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 +255,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 v3] iio: chemical: sps30: Replace manual locking with RAII locking

[Jonathan Cameron]

On Thu, 14 May 2026 22:13:42 -0500
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>
> ---
> v2:
>  - Switch over some scoped_guard()s to guard(mutex)() per David's
>    suggestion.
>  - Remove redundant whitespace per Andy's suggestion.
>  - Add new wrapper sps30_do_meas() per Andy's suggestion (see commit
>    message).
>  - Add Joshua's RB
>    (link: https://lore.kernel.org/linux-iio/CAKqfh0FWig8mRR-xhvnfcFeSinR6RySyPaf9Gbpb6WU+diiiUQ@mail.gmail.com/T/#t)
> 
> v3:
>  - Remove sps30_do_meas() wrapper per Andy's suggestion.
I think this wasn't what Andy meant. I think he meant just
push the guard into the the function you were wrapping.

>  - Remove Joshua's RB (major changes).
> 
>  drivers/iio/chemical/sps30.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> index a934bf0298dd..9cb201cf0f15 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,8 @@ 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));
Every call to sps30_do_meas() is done with the lock held just over that call.

So just move the lock (as a guard) into sps30_do_meas().

I think that's what Andy was suggesting in v2 review, but maybe not!


>  	if (ret)
>  		goto err;
>  

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

[Maxwell Doose]

On Fri, May 15, 2026 at 9:18 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 14 May 2026 22:13:42 -0500
> 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>
> > ---
> > v2:
> >  - Switch over some scoped_guard()s to guard(mutex)() per David's
> >    suggestion.
> >  - Remove redundant whitespace per Andy's suggestion.
> >  - Add new wrapper sps30_do_meas() per Andy's suggestion (see commit
> >    message).
> >  - Add Joshua's RB
> >    (link: https://lore.kernel.org/linux-iio/CAKqfh0FWig8mRR-xhvnfcFeSinR6RySyPaf9Gbpb6WU+diiiUQ@mail.gmail.com/T/#t)
> >
> > v3:
> >  - Remove sps30_do_meas() wrapper per Andy's suggestion.
> I think this wasn't what Andy meant. I think he meant just
> push the guard into the the function you were wrapping.
>

TBH I'm fairly confused on what Andy wanted in terms of "what do I
wrap". If he wants me to put the guard()() into the function call
itself I can do that but see the (ps) in my other email.

> >  - Remove Joshua's RB (major changes).
> >
> >  drivers/iio/chemical/sps30.c | 29 +++++++++++++----------------
> >  1 file changed, 13 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > index a934bf0298dd..9cb201cf0f15 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,8 @@ 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));
> Every call to sps30_do_meas() is done with the lock held just over that call.
>
> So just move the lock (as a guard) into sps30_do_meas().
>
> I think that's what Andy was suggesting in v2 review, but maybe not!
>

Perhaps, we'll see I suppose.

best regards,
max


>
> >       if (ret)
> >               goto err;
> >

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

[Jonathan Cameron]

On Fri, 15 May 2026 09:35:03 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Fri, May 15, 2026 at 9:18 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 14 May 2026 22:13:42 -0500
> > 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>
> > > ---
> > > v2:
> > >  - Switch over some scoped_guard()s to guard(mutex)() per David's
> > >    suggestion.
> > >  - Remove redundant whitespace per Andy's suggestion.
> > >  - Add new wrapper sps30_do_meas() per Andy's suggestion (see commit
> > >    message).
> > >  - Add Joshua's RB
> > >    (link: https://lore.kernel.org/linux-iio/CAKqfh0FWig8mRR-xhvnfcFeSinR6RySyPaf9Gbpb6WU+diiiUQ@mail.gmail.com/T/#t)
> > >
> > > v3:
> > >  - Remove sps30_do_meas() wrapper per Andy's suggestion.  
> > I think this wasn't what Andy meant. I think he meant just
> > push the guard into the the function you were wrapping.
> >  
> 
> TBH I'm fairly confused on what Andy wanted in terms of "what do I
> wrap". If he wants me to put the guard()() into the function call
> itself I can do that but see the (ps) in my other email.

I guess you mean:
Andy:
"Ah, I was under impression that this is (unlocked version) is used somewhere
else. Since it's not the case, the wrapper is not needed, just use guard()()
in the original code."

Could be read either way but I think he'll be fine with it in sps30_do_meas()
even if the other direction was what he was thinking of.

J


> 
> > >  - Remove Joshua's RB (major changes).
> > >
> > >  drivers/iio/chemical/sps30.c | 29 +++++++++++++----------------
> > >  1 file changed, 13 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/iio/chemical/sps30.c b/drivers/iio/chemical/sps30.c
> > > index a934bf0298dd..9cb201cf0f15 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,8 @@ 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));  
> > Every call to sps30_do_meas() is done with the lock held just over that call.
> >
> > So just move the lock (as a guard) into sps30_do_meas().
> >
> > I think that's what Andy was suggesting in v2 review, but maybe not!
> >  
> 
> Perhaps, we'll see I suppose.
> 
> best regards,
> max
> 
> 
> >  
> > >       if (ret)
> > >               goto err;
> > >  
>