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

[Maxwell Doose]

On Wed, May 27, 2026 at 5:29 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 26 May 2026 15:31:10 -0500
> Maxwell Doose <m32285159@xxxxxxxxx> wrote:
>
> > On Tue, May 26, 2026 at 9:47 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > >
> > > On Sat, 23 May 2026 13:25:32 -0500
> > > Maxwell Doose <m32285159@xxxxxxxxx> 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@xxxxxxxxx>
> > > 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!
>

I wonder, perhaps either size (not as good) or arr_size_bytes? Maybe
also scan_data_byte_size? Probably still something better out there,
those are just some names I can come up with off the top of my head.

best regards,
max



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