public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] iio: imu: kmx61: Use guard(mutex)() over manual locking
@ 2026-05-05 22:07 Maxwell Doose
  2026-05-06  9:06 ` Andy Shevchenko
  2026-05-06 16:05 ` Jonathan Cameron
  0 siblings, 2 replies; 5+ messages in thread
From: Maxwell Doose @ 2026-05-05 22:07 UTC (permalink / raw)
  To: jic23
  Cc: David Lechner, Nuno Sá, Andy Shevchenko,
	open list:IIO SUBSYSTEM AND DRIVERS, open list

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@gmail.com>
---
 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()().

 drivers/iio/imu/kmx61.c | 98 ++++++++++++++++-------------------------
 1 file changed, 38 insertions(+), 60 deletions(-)

diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
index 3cd91d8a89ee..1f1a695ab161 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:
@@ -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: {
 		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;
 }
 
@@ -848,27 +845,24 @@ 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,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;
 }
 
@@ -1020,11 +1011,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 +1025,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 +1033,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,8 +1042,6 @@ 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;
 }
@@ -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;
+			}
+			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;
@@ -1419,22 +1407,16 @@ 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);
-
-	return ret;
+	guard(mutex)(&data->lock);
+	return kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, false);
 }
 
 static int kmx61_resume(struct device *dev)
@@ -1453,13 +1435,9 @@ 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);
-
-	return ret;
+	guard(mutex)(&data->lock);
+	return kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
 }
 
 static int kmx61_runtime_resume(struct device *dev)
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] iio: imu: kmx61: Use guard(mutex)() over manual locking
  2026-05-05 22:07 [PATCH v4] iio: imu: kmx61: Use guard(mutex)() over manual locking Maxwell Doose
@ 2026-05-06  9:06 ` Andy Shevchenko
  2026-05-06 14:58   ` Maxwell Doose
  2026-05-06 16:05 ` Jonathan Cameron
  1 sibling, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2026-05-06  9:06 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: jic23, David Lechner, Nuno Sá, Andy Shevchenko,
	open list:IIO SUBSYSTEM AND DRIVERS, open list

On Tue, May 05, 2026 at 05:07:19PM -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.

...

> -	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;
>  	}
> +	}
>  	return -EINVAL;

For making the above less ambiguous, add default case here instead of the
standalone return.

	}
	default:
		return -EINVAL;
	}

>  }

...

>  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);
> -
> -	return ret;
> +	guard(mutex)(&data->lock);

Leave a blank line here. Same applies to all guard()() cases like this.

> +	return kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>  }

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] iio: imu: kmx61: Use guard(mutex)() over manual locking
  2026-05-06  9:06 ` Andy Shevchenko
@ 2026-05-06 14:58   ` Maxwell Doose
  0 siblings, 0 replies; 5+ messages in thread
From: Maxwell Doose @ 2026-05-06 14:58 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: jic23, David Lechner, Nuno Sá, Andy Shevchenko,
	open list:IIO SUBSYSTEM AND DRIVERS, open list

On Wed, May 6, 2026 at 4:06 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Tue, May 05, 2026 at 05:07:19PM -0500, Maxwell Doose wrote:
[snip]
> > -     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;
> >       }
> > +     }
> >       return -EINVAL;
>
> For making the above less ambiguous, add default case here instead of the
> standalone return.
>

Sounds good, I'll get that into the next version.

>
>         }
>         default:
>                 return -EINVAL;
>         }
>
> >  }
>
> ...
>
> >  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);
> > -
> > -     return ret;
> > +     guard(mutex)(&data->lock);
>
> Leave a blank line here. Same applies to all guard()() cases like this.
>

Ah, got it. I'm used to grouping related stuff in the code so to me
it's a bit strange but I'll also add that in the next version.

best regards,
max



>
> > +     return kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> >  }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] iio: imu: kmx61: Use guard(mutex)() over manual locking
  2026-05-05 22:07 [PATCH v4] iio: imu: kmx61: Use guard(mutex)() over manual locking Maxwell Doose
  2026-05-06  9:06 ` Andy Shevchenko
@ 2026-05-06 16:05 ` Jonathan Cameron
  2026-05-06 16:56   ` Maxwell Doose
  1 sibling, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2026-05-06 16:05 UTC (permalink / raw)
  To: Maxwell Doose
  Cc: David Lechner, Nuno Sá, Andy Shevchenko,
	open list:IIO SUBSYSTEM AND DRIVERS, open list

On Tue,  5 May 2026 17:07:19 -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. 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@gmail.com>
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.

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.

>  		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.
>  }

> @@ -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!)

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;
}

> +			}
> +			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;


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4] iio: imu: kmx61: Use guard(mutex)() over manual locking
  2026-05-06 16:05 ` Jonathan Cameron
@ 2026-05-06 16:56   ` Maxwell Doose
  0 siblings, 0 replies; 5+ messages in thread
From: Maxwell Doose @ 2026-05-06 16:56 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: David Lechner, Nuno Sá, Andy Shevchenko,
	open list:IIO SUBSYSTEM AND DRIVERS, open list

Hi Jonathan,

On Wed, May 6, 2026 at 11:05 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue,  5 May 2026 17:07:19 -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. 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@gmail.com>
> 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;
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-05-06 16:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-05 22:07 [PATCH v4] iio: imu: kmx61: Use guard(mutex)() over manual locking Maxwell Doose
2026-05-06  9:06 ` Andy Shevchenko
2026-05-06 14:58   ` Maxwell Doose
2026-05-06 16:05 ` Jonathan Cameron
2026-05-06 16:56   ` Maxwell Doose

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox