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@xxxxxxxxx> wrote:

> On Fri, May 15, 2026 at 9:18 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Thu, 14 May 2026 22:13:42 -0500
> > Maxwell Doose <m32285159@xxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxx/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@xxxxxxxxx>
> > >   */
> > >
> > > +#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;
> > >  
>