drivers/iio/imu/kmx61.c | 129 +++++++++++++++++++++------------------- 1 file changed, 67 insertions(+), 62 deletions(-)
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 to simplify
error paths and eliminate manual locking calls.
Add new helper function kmx61_read_for_each_active_channel() to mitigate
certain style issues and to prevent notifying that the IRQ is finished
whilst holding the lock.
Update certain returns, and add default case to return -EINVAL in
kmx61_read_raw().
Remove now-redundant gotos and ret variables, as the new RAII macros
make them unneeded.
Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
drivers/iio/imu/kmx61.c | 129 +++++++++++++++++++++-------------------
1 file changed, 67 insertions(+), 62 deletions(-)
diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
index 3cd91d8a89ee..6e9eafc06d78 100644
--- a/drivers/iio/imu/kmx61.c
+++ b/drivers/iio/imu/kmx61.c
@@ -7,6 +7,7 @@
* IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
*/
+#include <linux/cleanup.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
#include <linux/mod_devicetable.h>
@@ -783,7 +784,7 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
struct kmx61_data *data = kmx61_get_data(indio_dev);
switch (mask) {
- case IIO_CHAN_INFO_RAW:
+ case IIO_CHAN_INFO_RAW: {
switch (chan->type) {
case IIO_ACCEL:
base_reg = KMX61_ACC_XOUT_L;
@@ -794,28 +795,24 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
- mutex_lock(&data->lock);
+ guard(mutex)(&data->lock);
ret = kmx61_set_power_state(data, true, chan->address);
- if (ret) {
- mutex_unlock(&data->lock);
+ if (ret)
return ret;
- }
ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
if (ret < 0) {
kmx61_set_power_state(data, false, chan->address);
- mutex_unlock(&data->lock);
return ret;
}
*val = sign_extend32(ret >> chan->scan_type.shift,
chan->scan_type.realbits - 1);
ret = kmx61_set_power_state(data, false, chan->address);
-
- mutex_unlock(&data->lock);
if (ret)
return ret;
return IIO_VAL_INT;
+ }
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_ACCEL:
@@ -834,41 +831,40 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
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;
+ default:
+ return -EINVAL;
}
- return -EINVAL;
}
static int kmx61_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int val,
int val2, long mask)
{
- int ret;
struct kmx61_data *data = kmx61_get_data(indio_dev);
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;
+ guard(mutex)(&data->lock);
+
+ return kmx61_set_odr(data, val, val2, chan->address);
+ }
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
- case IIO_ACCEL:
+ 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);
+ }
default:
return -EINVAL;
}
@@ -945,29 +941,26 @@ 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;
+ return 0;
}
static int kmx61_acc_validate_trigger(struct iio_dev *indio_dev,
@@ -1020,11 +1013,11 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
struct kmx61_data *data = kmx61_get_data(indio_dev);
- mutex_lock(&data->lock);
+ guard(mutex)(&data->lock);
if (!state && data->ev_enable_state && data->motion_trig_on) {
data->motion_trig_on = false;
- goto err_unlock;
+ return ret;
}
if (data->acc_dready_trig == trig || data->motion_trig == trig)
@@ -1034,7 +1027,7 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
ret = kmx61_set_power_state(data, state, device);
if (ret < 0)
- goto err_unlock;
+ return ret;
if (data->acc_dready_trig == trig || data->mag_dready_trig == trig)
ret = kmx61_setup_new_data_interrupt(data, state, device);
@@ -1042,7 +1035,7 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
ret = kmx61_setup_any_motion_interrupt(data, state);
if (ret < 0) {
kmx61_set_power_state(data, false, device);
- goto err_unlock;
+ return ret;
}
if (data->acc_dready_trig == trig)
@@ -1051,10 +1044,8 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
data->mag_dready_trig_on = state;
else
data->motion_trig_on = state;
-err_unlock:
- mutex_unlock(&data->lock);
- return ret;
+ return 0;
}
static void kmx61_trig_reenable(struct iio_trigger *trig)
@@ -1181,30 +1172,51 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
return IRQ_HANDLED;
}
-static irqreturn_t kmx61_trigger_handler(int irq, void *p)
+/**
+ * kmx61_read_for_each_active_channel() - Read each active channel into a buffer
+ *
+ * @indio_dev: IIO Device struct to read from
+ * @buffer: Destination buffer to write to, the array must be of at least size 8
+ *
+ * Intended only for use in kmx61_trigger_handler().
+ *
+ * Return:
+ * 0 on success, negative errno on failure.
+ */
+static int kmx61_read_for_each_active_channel(struct iio_dev *indio_dev, s16 *buffer)
{
- struct iio_poll_func *pf = p;
- struct iio_dev *indio_dev = pf->indio_dev;
struct kmx61_data *data = kmx61_get_data(indio_dev);
- int bit, ret, i = 0;
u8 base;
- s16 buffer[8] = { };
+ int ret, bit;
+ int i = 0;
if (indio_dev == data->acc_indio_dev)
base = KMX61_ACC_XOUT_L;
else
base = KMX61_MAG_XOUT_L;
- 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;
- }
+ if (ret < 0)
+ return ret;
buffer[i++] = ret;
}
- mutex_unlock(&data->lock);
+
+ 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;
+ int ret;
+ s16 buffer[8] = { };
+
+ ret = kmx61_read_for_each_active_channel(indio_dev, buffer);
+ if (ret < 0)
+ goto err;
iio_push_to_buffers(indio_dev, buffer);
err:
@@ -1419,22 +1431,18 @@ static void kmx61_remove(struct i2c_client *client)
iio_trigger_unregister(data->motion_trig);
}
- mutex_lock(&data->lock);
+ guard(mutex)(&data->lock);
+
kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
- mutex_unlock(&data->lock);
}
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,
- false);
- mutex_unlock(&data->lock);
+ guard(mutex)(&data->lock);
- return ret;
+ return kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, false);
}
static int kmx61_resume(struct device *dev)
@@ -1453,13 +1461,10 @@ static int kmx61_resume(struct device *dev)
static int kmx61_runtime_suspend(struct device *dev)
{
struct kmx61_data *data = i2c_get_clientdata(to_i2c_client(dev));
- int ret;
- mutex_lock(&data->lock);
- ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
- mutex_unlock(&data->lock);
+ guard(mutex)(&data->lock);
- return ret;
+ return kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
}
static int kmx61_runtime_resume(struct device *dev)
--
2.54.0On Thu, 21 May 2026 17:30:43 -0500
Maxwell Doose <m32285159@gmail.com> 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 to simplify
> error paths and eliminate manual locking calls.
>
> Add new helper function kmx61_read_for_each_active_channel() to mitigate
> certain style issues and to prevent notifying that the IRQ is finished
> whilst holding the lock.
>
> Update certain returns, and add default case to return -EINVAL in
> kmx61_read_raw().
>
> Remove now-redundant gotos and ret variables, as the new RAII macros
> make them unneeded.
A few remaining things inline. I'll tweak whilst applying.
Tweaked as:
diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
index 52a6e044e1e5..b8a8297b39af 100644
--- a/drivers/iio/imu/kmx61.c
+++ b/drivers/iio/imu/kmx61.c
@@ -827,15 +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: {
if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
return -EINVAL;
- scoped_guard(mutex, &data->lock)
- ret = kmx61_get_odr(data, val, val2, chan->address);
+ guard(mutex)(&data->lock);
+
+ ret = kmx61_get_odr(data, val, val2, chan->address);
if (ret)
return -EINVAL;
return IIO_VAL_INT_PLUS_MICRO;
+ }
default:
return -EINVAL;
}
@@ -1178,8 +1180,6 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
* @indio_dev: IIO Device struct to read from
* @buffer: Destination buffer to write to, the array must be of at least size 8
*
- * Intended only for use in kmx61_trigger_handler().
- *
* Return:
* 0 on success,
*/
See below for why.
>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
> drivers/iio/imu/kmx61.c | 129 +++++++++++++++++++++-------------------
> 1 file changed, 67 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> index 3cd91d8a89ee..6e9eafc06d78 100644
> --- a/drivers/iio/imu/kmx61.c
> +++ b/drivers/iio/imu/kmx61.c
> @@ -7,6 +7,7 @@
> * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
> */
>
> +#include <linux/cleanup.h>
> #include <linux/i2c.h>
> #include <linux/interrupt.h>
> #include <linux/mod_devicetable.h>
> @@ -783,7 +784,7 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> struct kmx61_data *data = kmx61_get_data(indio_dev);
>
> switch (mask) {
> - case IIO_CHAN_INFO_RAW:
> + case IIO_CHAN_INFO_RAW: {
> switch (chan->type) {
> case IIO_ACCEL:
> base_reg = KMX61_ACC_XOUT_L;
> @@ -794,28 +795,24 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> - mutex_lock(&data->lock);
> + guard(mutex)(&data->lock);
>
> ret = kmx61_set_power_state(data, true, chan->address);
> - if (ret) {
> - mutex_unlock(&data->lock);
> + if (ret)
> return ret;
> - }
>
> ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
> if (ret < 0) {
> kmx61_set_power_state(data, false, chan->address);
> - mutex_unlock(&data->lock);
> return ret;
> }
> *val = sign_extend32(ret >> chan->scan_type.shift,
> chan->scan_type.realbits - 1);
> ret = kmx61_set_power_state(data, false, chan->address);
> -
> - mutex_unlock(&data->lock);
> if (ret)
> return ret;
> return IIO_VAL_INT;
> + }
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> case IIO_ACCEL:
> @@ -834,41 +831,40 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> 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);
Why is this one a scoped_guard()? Seems like it would be more consistent
if it was done like the one above with {} and guard()
> if (ret)
> return -EINVAL;
Unconnected to this patch but why are we eating the error code from kmx61_get_odr()?
In practice makes no difference as -EINVAL is the only error returned.
Still nice to do
if (ret)
return ret;
so we don't need to go check that.
I'd be fine with you sneaking this in this patch with a brief not in the commit
message if you want to. Or can leave for another day.
> return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> }
> - return -EINVAL;
> }
>
...
>
> static void kmx61_trig_reenable(struct iio_trigger *trig)
> @@ -1181,30 +1172,51 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
> return IRQ_HANDLED;
> }
>
> -static irqreturn_t kmx61_trigger_handler(int irq, void *p)
> +/**
> + * kmx61_read_for_each_active_channel() - Read each active channel into a buffer
> + *
> + * @indio_dev: IIO Device struct to read from
> + * @buffer: Destination buffer to write to, the array must be of at least size 8
> + *
> + * Intended only for use in kmx61_trigger_handler().
Not seeing a reason to have this comment. If there is another useful
place to use it the function is fairly generic. Right now there
isn't obvious, but we don't need to justify that!
> + *
On Fri, May 22, 2026 at 7:46 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 21 May 2026 17:30:43 -0500
> Maxwell Doose <m32285159@gmail.com> 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 to simplify
> > error paths and eliminate manual locking calls.
> >
> > Add new helper function kmx61_read_for_each_active_channel() to mitigate
> > certain style issues and to prevent notifying that the IRQ is finished
> > whilst holding the lock.
> >
> > Update certain returns, and add default case to return -EINVAL in
> > kmx61_read_raw().
> >
> > Remove now-redundant gotos and ret variables, as the new RAII macros
> > make them unneeded.
>
> A few remaining things inline. I'll tweak whilst applying.
>
> Tweaked as:
> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> index 52a6e044e1e5..b8a8297b39af 100644
> --- a/drivers/iio/imu/kmx61.c
> +++ b/drivers/iio/imu/kmx61.c
> @@ -827,15 +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: {
> if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> return -EINVAL;
>
> - scoped_guard(mutex, &data->lock)
> - ret = kmx61_get_odr(data, val, val2, chan->address);
> + guard(mutex)(&data->lock);
> +
> + ret = kmx61_get_odr(data, val, val2, chan->address);
> if (ret)
> return -EINVAL;
> return IIO_VAL_INT_PLUS_MICRO;
> + }
> default:
> return -EINVAL;
> }
> @@ -1178,8 +1180,6 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
> * @indio_dev: IIO Device struct to read from
> * @buffer: Destination buffer to write to, the array must be of at least size 8
> *
> - * Intended only for use in kmx61_trigger_handler().
> - *
> * Return:
> * 0 on success,
> */
>
> See below for why.
>
>
> >
> > Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> > ---
> > drivers/iio/imu/kmx61.c | 129 +++++++++++++++++++++-------------------
> > 1 file changed, 67 insertions(+), 62 deletions(-)
> >
> > diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> > index 3cd91d8a89ee..6e9eafc06d78 100644
> > --- a/drivers/iio/imu/kmx61.c
> > +++ b/drivers/iio/imu/kmx61.c
> > @@ -7,6 +7,7 @@
> > * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
> > */
> >
> > +#include <linux/cleanup.h>
> > #include <linux/i2c.h>
> > #include <linux/interrupt.h>
> > #include <linux/mod_devicetable.h>
> > @@ -783,7 +784,7 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> > struct kmx61_data *data = kmx61_get_data(indio_dev);
> >
> > switch (mask) {
> > - case IIO_CHAN_INFO_RAW:
> > + case IIO_CHAN_INFO_RAW: {
> > switch (chan->type) {
> > case IIO_ACCEL:
> > base_reg = KMX61_ACC_XOUT_L;
> > @@ -794,28 +795,24 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> > default:
> > return -EINVAL;
> > }
> > - mutex_lock(&data->lock);
> > + guard(mutex)(&data->lock);
> >
> > ret = kmx61_set_power_state(data, true, chan->address);
> > - if (ret) {
> > - mutex_unlock(&data->lock);
> > + if (ret)
> > return ret;
> > - }
> >
> > ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
> > if (ret < 0) {
> > kmx61_set_power_state(data, false, chan->address);
> > - mutex_unlock(&data->lock);
> > return ret;
> > }
> > *val = sign_extend32(ret >> chan->scan_type.shift,
> > chan->scan_type.realbits - 1);
> > ret = kmx61_set_power_state(data, false, chan->address);
> > -
> > - mutex_unlock(&data->lock);
> > if (ret)
> > return ret;
> > return IIO_VAL_INT;
> > + }
> > case IIO_CHAN_INFO_SCALE:
> > switch (chan->type) {
> > case IIO_ACCEL:
> > @@ -834,41 +831,40 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> > 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);
>
> Why is this one a scoped_guard()? Seems like it would be more consistent
> if it was done like the one above with {} and guard()
>
I'm guessing you're talking about the {} in the switch-case. I guess
it's likely because the guard()() in that specific case goes for the
whole scope of the case, whereas we only need to hold the lock for
this call (I tend to keep section length as close as possible when
doing these things, and typically I'd use scoped_guard() for a single
call).
>
> > if (ret)
> > return -EINVAL;
> Unconnected to this patch but why are we eating the error code from kmx61_get_odr()?
> In practice makes no difference as -EINVAL is the only error returned.
>
> Still nice to do
> if (ret)
> return ret;
> so we don't need to go check that.
> I'd be fine with you sneaking this in this patch with a brief not in the commit
> message if you want to. Or can leave for another day.
>
Seems like a better idea to do this in a separate patch, perhaps where
we refresh some of the returns/control paths in the file. But at the
same time not opposed to sneaking this in if I end up touching that
function in a different patch.
>
> > return IIO_VAL_INT_PLUS_MICRO;
> > + default:
> > + return -EINVAL;
> > }
> > - return -EINVAL;
> > }
> >
> ...
>
>
> >
> > static void kmx61_trig_reenable(struct iio_trigger *trig)
> > @@ -1181,30 +1172,51 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
> > return IRQ_HANDLED;
> > }
> >
> > -static irqreturn_t kmx61_trigger_handler(int irq, void *p)
> > +/**
> > + * kmx61_read_for_each_active_channel() - Read each active channel into a buffer
> > + *
> > + * @indio_dev: IIO Device struct to read from
> > + * @buffer: Destination buffer to write to, the array must be of at least size 8
> > + *
> > + * Intended only for use in kmx61_trigger_handler().
> Not seeing a reason to have this comment. If there is another useful
> place to use it the function is fairly generic. Right now there
> isn't obvious, but we don't need to justify that!
>
That's probably just me erring on the side of caution just to make
sure nobody uses this erroneously. Although, I guess I could see this
being used somewhere else (assuming it was tweaked a little bit).
best regards,
max
> > + *
>
On Fri, 22 May 2026 10:38:09 -0500
Maxwell Doose <m32285159@gmail.com> wrote:
> On Fri, May 22, 2026 at 7:46 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 21 May 2026 17:30:43 -0500
> > Maxwell Doose <m32285159@gmail.com> 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 to simplify
> > > error paths and eliminate manual locking calls.
> > >
> > > Add new helper function kmx61_read_for_each_active_channel() to mitigate
> > > certain style issues and to prevent notifying that the IRQ is finished
> > > whilst holding the lock.
> > >
> > > Update certain returns, and add default case to return -EINVAL in
> > > kmx61_read_raw().
> > >
> > > Remove now-redundant gotos and ret variables, as the new RAII macros
> > > make them unneeded.
> >
> > A few remaining things inline. I'll tweak whilst applying.
> >
> > Tweaked as:
> > diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> > index 52a6e044e1e5..b8a8297b39af 100644
> > --- a/drivers/iio/imu/kmx61.c
> > +++ b/drivers/iio/imu/kmx61.c
> > @@ -827,15 +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: {
> > if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> > return -EINVAL;
> >
> > - scoped_guard(mutex, &data->lock)
> > - ret = kmx61_get_odr(data, val, val2, chan->address);
> > + guard(mutex)(&data->lock);
> > +
> > + ret = kmx61_get_odr(data, val, val2, chan->address);
> > if (ret)
> > return -EINVAL;
> > return IIO_VAL_INT_PLUS_MICRO;
> > + }
> > default:
> > return -EINVAL;
> > }
> > @@ -1178,8 +1180,6 @@ static irqreturn_t kmx61_data_rdy_trig_poll(int irq, void *private)
> > * @indio_dev: IIO Device struct to read from
> > * @buffer: Destination buffer to write to, the array must be of at least size 8
> > *
> > - * Intended only for use in kmx61_trigger_handler().
> > - *
> > * Return:
> > * 0 on success,
> > */
> >
> > See below for why.
I failed to say I've applied it with those tweaks.
> > > @@ -834,41 +831,40 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> > > 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);
> >
> > Why is this one a scoped_guard()? Seems like it would be more consistent
> > if it was done like the one above with {} and guard()
> >
>
> I'm guessing you're talking about the {} in the switch-case. I guess
> it's likely because the guard()() in that specific case goes for the
> whole scope of the case, whereas we only need to hold the lock for
> this call (I tend to keep section length as close as possible when
> doing these things, and typically I'd use scoped_guard() for a single
> call).
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);
if (ret)
return -EINVAL;
return IIO_VAL_INT_PLUS_MICRO;
Is the block in question. Given nothing else happens after this point,
other than returning. I'd always favour guard as it keeps the error
check at same level as the source of the error.
>
> >
> > > if (ret)
> > > return -EINVAL;
> > Unconnected to this patch but why are we eating the error code from kmx61_get_odr()?
> > In practice makes no difference as -EINVAL is the only error returned.
> >
> > Still nice to do
> > if (ret)
> > return ret;
> > so we don't need to go check that.
> > I'd be fine with you sneaking this in this patch with a brief not in the commit
> > message if you want to. Or can leave for another day.
> >
>
> Seems like a better idea to do this in a separate patch, perhaps where
> we refresh some of the returns/control paths in the file. But at the
> same time not opposed to sneaking this in if I end up touching that
> function in a different patch.
Sure.
>
> >
> > > return IIO_VAL_INT_PLUS_MICRO;
> > > + default:
> > > + return -EINVAL;
> > > }
> > > - return -EINVAL;
> > > }
> > >
> >
On Fri, May 22, 2026 at 12:29 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 22 May 2026 10:38:09 -0500
> Maxwell Doose <m32285159@gmail.com> wrote:
>
> > On Fri, May 22, 2026 at 7:46 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Thu, 21 May 2026 17:30:43 -0500
> > > Maxwell Doose <m32285159@gmail.com> wrote:
[snip]
> > > > @@ -834,41 +831,40 @@ static int kmx61_read_raw(struct iio_dev *indio_dev,
> > > > 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);
> > >
> > > Why is this one a scoped_guard()? Seems like it would be more consistent
> > > if it was done like the one above with {} and guard()
> > >
> >
> > I'm guessing you're talking about the {} in the switch-case. I guess
> > it's likely because the guard()() in that specific case goes for the
> > whole scope of the case, whereas we only need to hold the lock for
> > this call (I tend to keep section length as close as possible when
> > doing these things, and typically I'd use scoped_guard() for a single
> > call).
>
> 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);
> if (ret)
> return -EINVAL;
> return IIO_VAL_INT_PLUS_MICRO;
>
> Is the block in question. Given nothing else happens after this point,
> other than returning. I'd always favour guard as it keeps the error
> check at same level as the source of the error.
>
Sorry if I wasn't clear, I meant that. I see now that it just returns
right after, haha, that's my bad :/
best regards,
max
On Thu, May 21, 2026 at 5:30 PM Maxwell Doose <m32285159@gmail.com> 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 to simplify
> error paths and eliminate manual locking calls.
>
> Add new helper function kmx61_read_for_each_active_channel() to mitigate
> certain style issues and to prevent notifying that the IRQ is finished
> whilst holding the lock.
>
> Update certain returns, and add default case to return -EINVAL in
> kmx61_read_raw().
>
> Remove now-redundant gotos and ret variables, as the new RAII macros
> make them unneeded.
>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
> ---
> drivers/iio/imu/kmx61.c | 129 +++++++++++++++++++++-------------------
> 1 file changed, 67 insertions(+), 62 deletions(-)
>
[snip]
Crap, forgot the whole changelog.
v2:
- Remove redundant blank line per Andy.
- Put kmx61_set_mode() function call in kmx61_runtime_suspend() on one
line per Andy.
v3:
- Add dedicated scope for guards.
v4:
- Fix certain scoping in switch-case blocks.
- Remove stray ret variable.
- Used scoped_guard() in kmx61_trigger_handler() instead of guard()().
v5:
- Added new helper function kmx61_read_for_each_active_channel() per
Jonathan's suggestion.
- Add default case to return -EINVAL in kmx61_read_raw() per Andy's
suggestion.
- Add blank lines between guard(mutex)() calls and regular code per
Andy's suggestion.
- Change last return statement of kmx61_write_event_config() to be 0
per Jonathan's suggestion.
v6:
- Rebased onto TOCTOU fix per Jonathan's suggestion.
- (note: now dependent on said commit, link below)
- link: https://lore.kernel.org/linux-iio/CAKqfh0GHVtAS6Uw3Bjo+B0PCpcfT8FogqEPCmt2x59zhmmh1Kg@mail.gmail.com/T/#t
v7:
- Remove some unneeded parentheses.
- Remove stray whitespace.
- Fix up return value (from return ret; to return 0;) since we won't
ever return anything other than 0 from that return statement now.
Note:
Will save the dev_get_drvdata() patch for later.
best regards,
max
© 2016 - 2026 Red Hat, Inc.