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

[Matheus Giarola]

On Mon, Apr 13, 2026 at 4:50 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sun, 12 Apr 2026 12:47:48 -0300
> Matheus Giarola <matheustpgiarola@xxxxxxxxx> 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).
> >
> > Also, limit guard(mutex) scope in ad7280_show_balance_timer()
> > to keep it locked just when necessary.
> >
> > Suggested-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> > Signed-off-by: Matheus Giarola <matheusgiarola@xxxxxx>
> > ---
> > v4:
> > - limit guard(mutex) scope in ad7280_show_balance_timer()
>
> Some confusion has occured here.  The braces thing works for some
> places (where we bring only trivial extra operations under the lock)
> but don't use it where scoped_guard() is cleaner.

That makes sense. Sorry about the misunderstanding and thanks for the
correction.
>
> >
> > v3:
> > - correct v2 patch tags
> >
> > v2:
> > - fix missing scope in ad7280_read_raw() by wrapping IIO_CHAN_INFO_RAW
> >   case in braces so as to fix build errors.
> >
> >  drivers/iio/adc/ad7280a.c | 34 +++++++++++++---------------------
> >  1 file changed, 13 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7280a.c b/drivers/iio/adc/ad7280a.c
> > index ba12a3796e2b..f3cbf8810daa 100644
> > --- a/drivers/iio/adc/ad7280a.c
> > +++ b/drivers/iio/adc/ad7280a.c
> ...
>
> > @@ -519,10 +518,11 @@ static ssize_t ad7280_show_balance_timer(struct iio_dev *indio_dev,
> >       unsigned int msecs;
> >       int ret;
> >
> > -     mutex_lock(&st->lock);
> > -     ret = ad7280_read_reg(st, chan->address >> 8,
> > +     {
> > +             guard(mutex)(&st->lock);
> > +             ret = ad7280_read_reg(st, chan->address >> 8,
> >                             (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> > -     mutex_unlock(&st->lock);
>
> Not this.  For this case you scoped_guard() or just take the view
> that it doesn't matter if we hold the lock a bit longer.

Alright.  Since Andy pointed out that it would be better if we kept the lock
only where it was necessary in previous versions, I'll change it to
scoped_guard().
>
>
> > +     }
> >
> >       if (ret < 0)
> >               return ret;
> > @@ -551,11 +551,10 @@ static ssize_t ad7280_store_balance_timer(struct iio_dev *indio_dev,
> >       if (val > 31)
> >               return -EINVAL;
> >
> > -     mutex_lock(&st->lock);
> > +     guard(mutex)(&st->lock);
> >       ret = ad7280_write(st, chan->address >> 8,
> >                          (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG, 0,
> >                          FIELD_PREP(AD7280A_CB_TIMER_VAL_MSK, val));
> > -     mutex_unlock(&st->lock);
> >
> >       return ret ? ret : len;
> >  }
> > @@ -737,7 +736,7 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
> >       if (val2 != 0)
> >               return -EINVAL;
> >
> > -     mutex_lock(&st->lock);
> > +     guard(mutex)(&st->lock);
> >       switch (chan->type) {
> >       case IIO_VOLTAGE:
> >               value = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
> > @@ -760,8 +759,7 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
> Just above this is:
>                         if (ret)
>                                 break;
>
> Make that
>                         if (ret)
>                                 return ret;
>
> and same for other such breaks on error.
>
>
> >                       st->cell_threshlow = value;
> >                       break;
> >               default:
> > -                     ret = -EINVAL;
> > -                     goto err_unlock;
> > +                     return -EINVAL;
> >               }
> >               break;
> >       case IIO_TEMP:
> > @@ -785,18 +783,12 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
> >                       st->aux_threshlow = value;
> >                       break;
> >               default:
> > -                     ret = -EINVAL;
> > -                     goto err_unlock;
> > +                     return -EINVAL;
> >               }
> >               break;
> >       default:
> > -             ret = -EINVAL;
> > -             goto err_unlock;
> > +             return -EINVAL;
> >       }
> > -
> > -err_unlock:
> > -     mutex_unlock(&st->lock);
> > -
> >       return ret;
> maybe with the changes suggested above used throughout we might not
> be able to get here with out ret == 0;
> In which case make that obvious with
>         return 0;
>

Got it, I'll send a v5 with all these changes.

Thanks for all the support and suggestions,
Matheus.