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

[Maxwell Doose]

On Wed, May 20, 2026 at 7:28 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 19 May 2026 08:19:43 -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().
> >
> > In addition, small refactor to replace "?:" operator with regular
> > if/else returns.
> >
> > Signed-off-by: Maxwell Doose <m32285159@xxxxxxxxx>
> Hi Maxwell
>
> One of those cases where it made us look at code for first time in a while.
> The original handling merrily copied garbage data.  Let's clean that up whilst
> we are here.
>
> Thanks,
>
> Jonathan
> >  static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > @@ -579,24 +587,34 @@ static irqreturn_t scd30_irq_thread_handler(int irq, void *priv)
> >       return IRQ_HANDLED;
> >  }
> >
> > +/* Meant ONLY for scd30_trigger_handler() */
> > +static int scd30_trigger_handler_helper_locked(struct iio_dev *indio_dev,
> > +                                            int *scan_data, int arr_size)
> > +{
> > +     struct scd30_state *state = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     guard(mutex)(&state->lock);
> > +
> > +     if (!iio_trigger_using_own(indio_dev))
> > +             ret = scd30_read_poll(state);
> > +     else
> > +             ret = scd30_read_meas(state);
> > +     memcpy(scan_data, state->meas, arr_size);
>
> Why is that a good thing to do if we are in an error condition?
> I've have thought the data wasn't valid.
>
>         if (ret)
>                 return ret;
>
>         memcpy()..
>
>         return 0;
>
> Or is there something useful in that data?
> Looks like some less than ideal code in the original driver but
> I think it makes sense to clean that up now. Just make sure to put
> a note in the patch description.
>

That's likely a result of me being lazy and copy-pasting some of the
code and touching it up so we can make the stack frame leaner. TBH I
would say moving it may be out of scope for this patch? I don't know,
I think it may be worth leaving since the original code did this. I'll
still check the docs later to see if there is actually something
useful in the data.

best regards,
max

>
> > +     return ret;
> > +}
> > +
> >  static irqreturn_t scd30_trigger_handler(int irq, void *p)
> >  {
> >       struct iio_poll_func *pf = p;
> >       struct iio_dev *indio_dev = pf->indio_dev;
> > -     struct scd30_state *state = iio_priv(indio_dev);
> >       struct {
> >               int data[SCD30_MEAS_COUNT];
> >               aligned_s64 ts;
> >       } scan = { };
> >       int ret;
> >
> > -     mutex_lock(&state->lock);
> > -     if (!iio_trigger_using_own(indio_dev))
> > -             ret = scd30_read_poll(state);
> > -     else
> > -             ret = scd30_read_meas(state);
> > -     memcpy(scan.data, state->meas, sizeof(state->meas));
> > -     mutex_unlock(&state->lock);
> > +     ret = scd30_trigger_handler_helper_locked(indio_dev, scan.data, sizeof(scan.data));
> >       if (ret)
> >               goto out;
> >
>