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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 26 May 2026 15:31:10 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Tue, May 26, 2026 at 9:47 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 23 May 2026 13:25:32 -0500
> > Maxwell Doose <m32285159@gmail.com> wrote:
> >  
> > > scd30_core.c currently uses manual mutex_lock() and mutex_unlock()
> > > calls. Replace them with the newer guard(mutex)() for cleaner RAII
> > > patterns and to improve maintainability.
> > >
> > > Add new helper function scd30_trigger_handler_helper_locked() containing
> > > the critical section for scd30_trigger_handler(). After moving
> > > scd30_trigger_handler()'s critical section into the new helper, tune up
> > > control logic to return ret early and not memcpy() if it's an error
> > > condition.
> > >
> > > In addition, small refactor to replace "?:" operator with regular
> > > if/else returns.
> > >
> > > Signed-off-by: Maxwell Doose <m32285159@gmail.com>  
> > Just some naming things inline.
> >
> > I thought about just changing them and applying but decided
> > I'd rather you took another look to make sure you agree
> > with the suggested changes (and if you do send me a v5)
> >
> > Thanks,
> >
> > Jonathan
> >  
> > >  static ssize_t calibration_forced_value_store(struct device *dev, struct device_attribute *attr,
> > > @@ -424,11 +430,13 @@ static ssize_t calibration_forced_value_store(struct device *dev, struct device_
> > >       if (val < SCD30_FRC_MIN_PPM || val > SCD30_FRC_MAX_PPM)
> > >               return -EINVAL;
> > >
> > > -     mutex_lock(&state->lock);
> > > -     ret = scd30_command_write(state, CMD_FRC, val);
> > > -     mutex_unlock(&state->lock);
> > > +     guard(mutex)(&state->lock);
> > >
> > > -     return ret ?: len;
> > > +     ret = scd30_command_write(state, CMD_FRC, val);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     return len;
> > >  }
> > >
> > >  static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > > @@ -579,24 +587,36 @@ static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> > >       return IRQ_HANDLED;
> > >  }
> > >
> > > +/* Meant ONLY for scd30_trigger_handler() */  
> >
> > No need to say this in a comment. The code naming is clear enough. However...
> >  
> 
> Agree here with the removal of said comment.
> 
> > > +static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
> > > +                                            int *scan_data, int arr_size)  
> >
> > Avoid using _locked() in naming.  It isn't clear to readers if that
> > means it is locked already, or will lock..  Also somewhat unnecessary
> > here. We don't need the function name to say why there is a helper.
> >  
> 
> Ah...I'm honestly not sure. I know we've decided that _locked was
> appropriate in certain cases, I wonder if that may also be the case
> here. I guess though since I'd have to send a v5 anyways I'll change
> that.
> 
> >
> > arr_size would normally mean array size - i.e. how many ints
> > there are in scan_data.  Here it is the size of scan_data - probably
> > also make it a size_t
> >  
> 
> Hm, perhaps? I definitely agree with size_t, I'm not so sure on
> changing the name. However like I said since I'd likely have to send a
> v5 anyways I'll switch up the naming.

Given people will associate arr_size with ARRAY_SIZE() which on this array
would return the number of ints (so /4), that name definitely needs to change.
Exactly what it changes to is a different question!

> 
> >
> > So this whole thing becomes
> >
> > static int scd30_trigger_handler_helper(struct iio_dev *indio_dev,
> >                                         int *scan_data, size_t scan_data_size)
> >
> >  
> 
> Sounds good.
> 
> best regards,
> max