Re: [PATCH v2] iio: light: iqs621-als: use lock guards

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 21 Apr 2026 01:00:11 -0300
Pedro Barletta Gennari <pedro.pbg@usp.br> wrote:

> Use guard(mutex)() for handling mutex lock instead of
> manually locking and unlocking the mutex. This prevents forgotten
> locks due to early exits and remove the need of gotos.
> 
> Signed-off-by: Pedro Barletta Gennari <pedro.pbg@usp.br>
> ---
> v2:
>   - Keep include list ordered
>   - Remove redundant 'else'
>   - Remove unnecessary variable 'ret'
Hi Pedro,

The changes here enable a few additional code improvements that
I'd like to see made in the same patch.

See below,

Thanks,

Jonathan

> ---
>  drivers/iio/light/iqs621-als.c | 85 +++++++++++-----------------------
>  1 file changed, 28 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/iio/light/iqs621-als.c b/drivers/iio/light/iqs621-als.c
> index b9f230210f07..8d08f28c642d 100644
> --- a/drivers/iio/light/iqs621-als.c
> +++ b/drivers/iio/light/iqs621-als.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
> @@ -107,25 +108,21 @@ static int iqs621_als_notifier(struct notifier_block *notifier,
>  	indio_dev = iqs621_als->indio_dev;
>  	timestamp = iio_get_time_ns(indio_dev);
>  
> -	mutex_lock(&iqs621_als->lock);
> +	guard(mutex)(&iqs621_als->lock);
>  
>  	if (event_flags & BIT(IQS62X_EVENT_SYS_RESET)) {
>  		ret = iqs621_als_init(iqs621_als);
>  		if (ret) {
>  			dev_err(indio_dev->dev.parent,
>  				"Failed to re-initialize device: %d\n", ret);
> -			ret = NOTIFY_BAD;
> -		} else {
> -			ret = NOTIFY_OK;
> +			return NOTIFY_BAD;
>  		}
> -
> -		goto err_mutex;
> +		return NOTIFY_OK;
>  	}
>  
>  	if (!iqs621_als->light_en && !iqs621_als->range_en &&
>  	    !iqs621_als->prox_en) {
 	if (!iqs621_als->light_en && !iqs621_als->range_en && !iqs621_als->prox_en)

After change noted below, I'd just make this a slightly long single line.
We have gotten more relaxed on going a little over 80 chars since this
code was written.

> -		ret = NOTIFY_DONE;
> -		goto err_mutex;
> +		return NOTIFY_DONE;
>  	}

Single statement so { } not needed

>  

>  
>  static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> @@ -278,11 +262,11 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
>  	unsigned int val;
>  	int ret;
>  
> -	mutex_lock(&iqs621_als->lock);
> +	guard(mutex)(&iqs621_als->lock);
>  
>  	ret = regmap_read(iqs62x->regmap, iqs62x->dev_desc->als_flags, &val);
>  	if (ret)
> -		goto err_mutex;
> +		return ret;
>  	iqs621_als->als_flags = val;
>  
>  	switch (chan->type) {
> @@ -293,7 +277,7 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
>  									 0xFF);
>  		if (!ret)
>  			iqs621_als->light_en = state;
> -		break;
> +		return ret;
Same as two cases below
>  
>  	case IIO_INTENSITY:
>  		ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
> @@ -302,12 +286,12 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
>  									 0xFF);
>  		if (!ret)
>  			iqs621_als->range_en = state;
> -		break;
> +		return ret;
As below, flip this to
		if (ret)
			return ret;

		iqs621_als->range_en = state;

		return 0;

>  
>  	case IIO_PROXIMITY:
>  		ret = regmap_read(iqs62x->regmap, IQS622_IR_FLAGS, &val);
>  		if (ret)
> -			goto err_mutex;
> +			return ret;
>  		iqs621_als->ir_flags = val;
>  
>  		ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
> @@ -315,16 +299,11 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
>  					 state ? 0 : 0xFF);
>  		if (!ret)
>  			iqs621_als->prox_en = state;

Please flip this to the more common form. Makes reading the code a little easier
if errors are handled out of line as early as possible.

		if (ret)
			return ret;

		iqs621_als->prox_en = state;

		return 0;

> -		break;
> +		return ret;
>  
>  	default:
> -		ret = -EINVAL;
> +		return -EINVAL;
>  	}
> -
> -err_mutex:
> -	mutex_unlock(&iqs621_als->lock);
> -
> -	return ret;
>  }

>  static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> @@ -377,7 +351,7 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
>  	u8 ir_flags_mask, *thresh_cache;
>  	int ret = -EINVAL;

Drop this initialization and instead return the specific value on error paths.

>  
> -	mutex_lock(&iqs621_als->lock);
> +	guard(mutex)(&iqs621_als->lock);
>  
>  	switch (dir) {
>  	case IIO_EV_DIR_RISING:
> @@ -426,29 +400,26 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
>  			break;
>  
>  		default:
> -			goto err_mutex;
> +			return ret;
>  		}
>  
>  		thresh_cache = &iqs621_als->thresh_prox;
>  		break;
>  
>  	default:
> -		goto err_mutex;
> +		return ret;

return -EINVAL;

>  	}
>  
>  	if (thresh_val > 0xFF)
> -		goto err_mutex;
> +		return ret;

return -EINVAL;

>  
>  	ret = regmap_write(iqs62x->regmap, thresh_reg, thresh_val);
>  	if (ret)
> -		goto err_mutex;
> +		return ret;
>  
>  	*thresh_cache = thresh_val;
>  	iqs621_als->ir_flags_mask = ir_flags_mask;
>  
> -err_mutex:
> -	mutex_unlock(&iqs621_als->lock);
> -
>  	return ret;
>  }
>