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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 12 May 2026 21:06:54 -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.
> 
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
A couple of trivial things.

Thanks,

Jonathan

>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_ACCEL:
> @@ -834,41 +831,41 @@ 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;
> +
No blank line needed.
>  }

> @@ -1051,8 +1045,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;
Can we get here with non 0?  If not
	return 0; to make that visually obvious.

>  }
> @@ -1181,30 +1173,52 @@ 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) {

No brackets needed any more.

> -			mutex_unlock(&data->lock);
> -			goto err;
> +			return ret;
>  		}
>  		buffer[i++] = ret;
>  	}
> -	mutex_unlock(&data->lock);
> +
> +	return 0;
> +}

> @@ -1419,22 +1433,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_resume(struct device *dev)
> @@ -1453,13 +1463,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));

I'm a bit surprised there are any of these left. I thought we'd switched
them all for
	struct kmx61_data *data = dev_get_drvdata(dev);

To avoid going in circles. 

Ah well, job for another day!



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