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

Maxwell Doose
2026-05-04T18:28:31Z

Hi Andy,

On Mon, May 4, 2026 at 3:36 AM Andy Shevchenko
<andriy.shevchenko@intel.com> 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
>
>