Re: [PATCH v4] iio: light: vcnl4000: use lock guard()

[Jonathan Cameron <jic23@kernel.org>]

On Wed,  6 May 2026 18:05:33 -0300
Raffael Raiel Trindade <raffaelraiel@usp.br> wrote:

> From: Raffael Raiel Trindade <raffaelraiel@usp.br>
> 
> Use guard() and scoped_guard() for handling mutex lock instead of manually
> locking and unlocking. Remove gotos in error handling logic. This prevents
> forgotten locks on early exits.
> 
> Signed-off-by: Raffael Raiel Trindade <raffaelraiel@usp.br>
> Co-developed-by: Kim Carvalho <kim.ca@usp.br>
> Signed-off-by: Kim Carvalho <kim.ca@usp.br>

Hi All,

One remaining minor point and a suggestion for where there may be
some more useful cleanup to be done on this driver

Thanks,

Jonathan

> ---
> v4:
>  - use guard() and scoped_guard() instead of
>    lock() and unlock() in all functions
>  - drop ret = -EINVAL initialization
> 
> v3:
>  - fix indentation problems on line breaks
> 
> v2:
>  - remove unnecessary ret attributions on non-error logic.
>  - remove breaks on switch-case after return
> 
>  drivers/iio/light/vcnl4000.c | 148 +++++++++++------------------------
>  1 file changed, 45 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index c08927e34b4e..d87a95a05329 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>

>  
>  static int vcnl4200_set_power_state(struct vcnl4000_data *data, bool on)
> @@ -442,18 +433,18 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  	int tries = 20;
>  	int ret;
>  
> -	mutex_lock(&data->vcnl4000_lock);
> +	guard(mutex)(&data->vcnl4000_lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
>  					req_mask);
>  	if (ret < 0)
> -		goto fail;
> +		return ret;
>  
>  	/* wait for data to become ready */
>  	while (tries--) {
>  		ret = i2c_smbus_read_byte_data(data->client, VCNL4000_COMMAND);
>  		if (ret < 0)
> -			goto fail;
> +			return ret;
>  		if (ret & rdy_mask)
>  			break;
>  		msleep(20); /* measurement takes up to 100 ms */
> @@ -462,20 +453,13 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  	if (tries < 0) {
>  		dev_err(&data->client->dev,
>  			"vcnl4000_measure() failed, data not ready\n");
> -		ret = -EIO;
> -		goto fail;
> +		return -EIO;
>  	}
>  
>  	ret = vcnl4000_read_data(data, data_reg, val);
>  	if (ret < 0)
> -		goto fail;
> -
> -	mutex_unlock(&data->vcnl4000_lock);
> -
> -	return 0;
> +		return ret;
>  
> -fail:
> -	mutex_unlock(&data->vcnl4000_lock);
>  	return ret;
	
	return 0;

Check for any other cases that have been introduced where the final return
is always success (e.g. 0)

>  }

>  static ssize_t vcnl4040_read_ps_calibbias(struct vcnl4000_data *data, int *val, int *val2)
> @@ -878,20 +834,15 @@ static ssize_t vcnl4040_write_ps_calibbias(struct vcnl4000_data *data, int val)
>  	if (i >= ARRAY_SIZE(vcnl4040_ps_calibbias_ua))
>  		return -EINVAL;
>  
> -	mutex_lock(&data->vcnl4000_lock);
> +	guard(mutex)(&data->vcnl4000_lock);
>  
>  	ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF3);
>  	if (ret < 0)
> -		goto out_unlock;
> +		return ret;
>  
>  	regval = (ret & ~VCNL4040_PS_MS_LED_I);
>  	regval |= FIELD_PREP(VCNL4040_PS_MS_LED_I, i);

If you want more stuff to do on this driver, look at FIELD_MODIFY() and where
it can be usefully applied.

> -	ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF3,
> -					regval);
> -
> -out_unlock:
> -	mutex_unlock(&data->vcnl4000_lock);
> -	return ret;
> +	return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF3, regval);
>  }