Re: [PATCH v3] iio: adc: ad7280a: replace mutex_lock() with guard(mutex)

[Matheus Giarola]

On Thu, Apr 2, 2026 at 5:36 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> On Thu, Apr 02, 2026 at 12:15:06AM -0300, Matheus Giarola wrote:
> > Use guard(mutex) instead of mutex_lock()/mutex_unlock(),
> > ensuring the mutex is released automatically when leaving
> > the function scope. The change improves error handling and
> > avoids issues such as missing unlocks. It also simplifies the
> > code by removing 'err_unlock' label and several mutex_unlock()
> > calls.
> >
> > As suggested, in ad7280_read_raw(), wrap the IIO_CHAN_INFO_RAW
> > case in braces, providing a scope for the implicit variable
> > declared by guard(mutex).
>
> ...
>
> > static ssize_t ad7280_show_balance_timer(struct iio_dev *indio_dev,
>
> >       unsigned int msecs;
> >       int ret;
> >
> > -     mutex_lock(&st->lock);
> > +     guard(mutex)(&st->lock);
> >       ret = ad7280_read_reg(st, chan->address >> 8,
> >                             (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> > -     mutex_unlock(&st->lock);
> >
> >       if (ret < 0)
> >               return ret;
>
> This will include the sysfs_emit() and other things in the critical section. Why?

Hi Andy,
Thanks for pointing that out. The way I wrote it, guard() will
unnecessarily keep the
mutex locked in sysfs_emit() and other parts.
In a previous review, Jonathan suggested using braces instead of scoped_guard()
to limit the scope in the IIO_CHAN_INFO_RAW case. So I'll wrap the
critical section in
braces to fix it.
>
> ...
>
> > static int ad7280_read_raw(struct iio_dev *indio_dev,
>
> >       switch (m) {
> > -     case IIO_CHAN_INFO_RAW:
> > -             mutex_lock(&st->lock);
> > +     case IIO_CHAN_INFO_RAW: {
> > +             guard(mutex)(&st->lock);
> >               if (chan->address == AD7280A_ALL_CELLS)
> >                       ret = ad7280_read_all_channels(st, st->scan_cnt, NULL);
> >               else
> >                       ret = ad7280_read_channel(st, chan->address >> 8,
> >                                                 chan->address & 0xFF);
> > -             mutex_unlock(&st->lock);
> >
> >               if (ret < 0)
> >                       return ret;
>
> >               *val = ret;
>
> This also will be under the lock, but it's fine.
Ok, that makes sense. I'll keep it that way then.
>
> >               return IIO_VAL_INT;
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks,
Matheus.