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

[Maxwell Doose]

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.

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