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

[Maxwell Doose]

Hi Jonathan,

On Wed, May 6, 2026 at 11:05 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue,  5 May 2026 17:07:19 -0500
> Maxwell Doose <m32285159@xxxxxxxxx> 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.
> >
> > Signed-off-by: Maxwell Doose <m32285159@xxxxxxxxx>
> Hi Maxwell
>
> I'd slow down a bit to give more time for reviews.
> I'd imagine some of the below was true in earlier versions.
>

Yeah...sorry about that. I tend to want to get my latest fixes out
fast so patches can be merged faster and we all waste less time. I can
wait until tomorrow evening to send this out if you'd like.

>
> Jonathan
>
> > @@ -830,17 +827,17 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> >               default:
> >                       return -EINVAL;
> >               }
> > -     case IIO_CHAN_INFO_SAMP_FREQ:
> > +     case IIO_CHAN_INFO_SAMP_FREQ: {
>
> Don't need {} in this case as no local variables added in this scope.
> scoped_guard() is a trick with a for loop so has it's own scope definition
> as part of the macro implementation.
>

Ah. See, I'm a bit skeptical of scoped_guard() nowadays because of the
hidden for. But I'll replace that {} with the scoped_guard().

>
> >               if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> >                       return -EINVAL;
> >
> > -             mutex_lock(&data->lock);
> > -             ret = kmx61_get_odr(data, val, val2, chan->address);
> > -             mutex_unlock(&data->lock);
> > +             scoped_guard(mutex, &data->lock)
> > +                     ret = kmx61_get_odr(data, val, val2, chan->address);
> >               if (ret)
> >                       return -EINVAL;
> >               return IIO_VAL_INT_PLUS_MICRO;
> >       }
> > +     }
> >       return -EINVAL;
> >  }
>
> > @@ -945,28 +939,25 @@ static int kmx61_write_event_config(struct iio_dev *indio_dev,
> >       if (state && data->ev_enable_state)
> >               return 0;
> >
> > -     mutex_lock(&data->lock);
> > +     guard(mutex)(&data->lock);
> >
> >       if (!state && data->motion_trig_on) {
> >               data->ev_enable_state = false;
> > -             goto err_unlock;
> > +             return ret;
> >       }
> >
> >       ret = kmx61_set_power_state(data, state, KMX61_ACC);
> >       if (ret < 0)
> > -             goto err_unlock;
> > +             return ret;
> >
> >       ret = kmx61_setup_any_motion_interrupt(data, state);
> >       if (ret < 0) {
> >               kmx61_set_power_state(data, false, KMX61_ACC);
> > -             goto err_unlock;
> > +             return ret;
> >       }
> >
> >       data->ev_enable_state = state;
> >
> > -err_unlock:
> > -     mutex_unlock(&data->lock);
> > -
> >       return ret;
> If we know this is 0 (probably is) then return 0.
>

Will fix for v5.

>
> >  }
>
> > @@ -1195,19 +1184,18 @@ static irqreturn_t kmx61_trigger_handler(int irq, void *p)
> >       else
> >               base = KMX61_MAG_XOUT_L;
> >
> > -     mutex_lock(&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;
> > +     scoped_guard(mutex, &data->lock) {
> > +             iio_for_each_active_channel(indio_dev, bit) {
> > +                     ret = kmx61_read_measurement(data, base, bit);
> > +                     if (ret < 0) {
> > +                             iio_trigger_notify_done(indio_dev->trig);
> > +                             return IRQ_HANDLED;
>
> The duplication is a bit ugly. I'd be tempted to use a helper to clean this up
> Would be something like (not even compiled!)
>

Likely a good idea, I can do that, would probably be better to keep
the guard()() out of that scope.

>
> static int kmx61_read_all(struct iio_dev *indio_dev, s16 buffer[at_least 8])
> {
>         struct kmx61_data *data = kmx61_get_data(indio_dev);
>         u8 base;
>         int ret, i, bit;
>
>         if (indio_dev == data->acc_indio_dev)
>                 base = KMX61_ACC_XOUT_L;
>         else
>                 base = KMX61_MAG_XOUT_L;
>
>         guard(mutex)(&data->lock);
>
>         iio_for_each_active_channel(indio_dev, bit) {
>                 ret = kmx61_read_measurement(data, base, bit);
>                 if (ret < 0)
>                         return ret;
>                 buffer[i++] = ret;
>         }
>         return 0;
> }
>
> static irqreturn_t kmx61_trigger_handler(int irq, void *p)
> {
>         struct iio_poll_func *pf = p;
>         struct iio_dev *indio_dev = pf->indio_dev;
>         s16 buffer[8] = { };
>         u8 base;
>
>         if (kmx61_read_all(indio_dev, buffer))
>                 goto err;
>
>         iio_push_to_buffers(indio_dev, buffer);
> err:
>         iio_trigger_notify_done(indio_dev->trig);
>
>         return IRQ_HANDLED;
> }
>

I'll likely take the above and tune it up so that it will work as intended.

best regards,
max





> > +                     }
> > +                     buffer[i++] = ret;
> >               }
> > -             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;
>