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

[Jonathan Cameron <jic23@kernel.org>]

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;