Re: [PATCH] iio: imu: kmx61: Use guard(mutex)() family over manual locking

[Maxwell Doose]

Hi Andy,

On Mon, May 4, 2026 at 3:36 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> On Fri, May 01, 2026 at 10:24:54PM -0500, Maxwell Doose wrote:
> > Include linux/cleanup.h to take advantage of new macros.
> >
> > Replace manual mutex_lock() and mutex_unlock() calls across the file
> > with guard(mutex)() and scoped_guard() where appropriate. This will help
> > modernize the driver with up-to-date functions/macros.
> >
> > Remove now redundant gotos and ret variables, as the new RAII macros
> > make them unneeded.
>
> ...
>
> >               *val = sign_extend32(ret >> chan->scan_type.shift,
> >                                    chan->scan_type.realbits - 1);
> >               ret = kmx61_set_power_state(data, false, chan->address);
>
> >
>
> Now this blank line becomes a bit confusing as the following conditional is
> tightly coupled with the above code, remove it.
>

Will do, sounds good.

>
> > -             mutex_unlock(&data->lock);
> >               if (ret)
> >                       return ret;
> >               return IIO_VAL_INT;
>
> ...
>
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >       iio_for_each_active_channel(indio_dev, bit) {
> >               ret = kmx61_read_measurement(data, base, bit);
> >               if (ret < 0) {
> > -                     mutex_unlock(&data->lock);
> > -                     goto err;
> > +                     iio_trigger_notify_done(indio_dev->trig);
> > +                     return IRQ_HANDLED;
>
> Hmm... Is the HANDLED a right choice?
>

This is what was done at the former err goto label, so I thought it
was appropriate when we removed it to move it here.

>
>
> >               }
> >               buffer[i++] = ret;
> >       }
> > -     mutex_unlock(&data->lock);
> >
> >       iio_push_to_buffers(indio_dev, buffer);
> > -err:
> >       iio_trigger_notify_done(indio_dev->trig);
> >
> >       return IRQ_HANDLED;
>
> If the answer is yes, I'm wondering if we may deduplicate that...
>

Best way I can think of deduplication is:
A. __attribute__((cleanup())) function helper (overkill)
B. macro (bad idea)
C. refactor logic so that it will just fall through to the end

If we do go with option C then we should probably just wait until this
is merged and then send that patch...

>
> ...
>
> >  static int kmx61_suspend(struct device *dev)
> >  {
> > -     int ret;
> >       struct kmx61_data *data = i2c_get_clientdata(to_i2c_client(dev));
> >
> > -     mutex_lock(&data->lock);
> > -     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
> > +     guard(mutex)(&data->lock);
>
> > +     return kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
> >                            false);
>
> One line is okay in this case.
>

Also sounds good.

best regards,
max

>
>
> > -     mutex_unlock(&data->lock);
> > -
> > -     return ret;
> >  }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>