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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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