Re: [PATCH v3] iio: imu: kmx61: Use guard(mutex)() family over manual locking
From: Andy Shevchenko
Date: Tue May 05 2026 - 09:15:35 EST
On Tue, May 05, 2026 at 07:47:06AM -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.
Did you compile this version?
...
> - case IIO_CHAN_INFO_SAMP_FREQ:
> + case IIO_CHAN_INFO_SAMP_FREQ: {
> 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;
> }
> + }
This looks suspicious.
...
Please, slow down and check the patches you sent.
Also, use, if not yet, --histogram when preparing patches, it might make them
more readable.
...
> switch (mask) {
> - case IIO_CHAN_INFO_SAMP_FREQ:
> + case IIO_CHAN_INFO_SAMP_FREQ: {
> if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> return -EINVAL;
>
> - mutex_lock(&data->lock);
> - ret = kmx61_set_odr(data, val, val2, chan->address);
> - mutex_unlock(&data->lock);
> - return ret;
> - case IIO_CHAN_INFO_SCALE:
> + guard(mutex)(&data->lock);
+ blank line.
> + return kmx61_set_odr(data, val, val2, chan->address);
> + }
> + case IIO_CHAN_INFO_SCALE: {
> switch (chan->type) {
> case IIO_ACCEL:
> if (val != 0)
> return -EINVAL;
> - mutex_lock(&data->lock);
> - ret = kmx61_set_scale(data, val2);
> - mutex_unlock(&data->lock);
> - return ret;
> + guard(mutex)(&data->lock);
> + return kmx61_set_scale(data, val2);
Missing scope.
> default:
> return -EINVAL;
> }
> + }
> default:
> return -EINVAL;
> }
...
> - mutex_lock(&data->lock);
> + guard(mutex)(&data->lock);
+ blank line.
> 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;
> }
> 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;
...
> - mutex_lock(&data->lock);
> + guard(mutex)(&data->lock);
+ blank line.
> kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> - mutex_unlock(&data->lock);
Since guard()() is not like lock/unlock, the blank line is better for
readability. lock/unlock scenarios look&feel as special scope, that's why
the blank lines there are optional.
--
With Best Regards,
Andy Shevchenko