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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 20 Apr 2026 16:59:33 -0300
Raffael Raiel Trindade <raffaelraiel@usp.br> wrote:

> From: Raffael Raiel Trindade <raffaelraiel@usp.br>
> 
> Use 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>


There are some functions in here where guard() is useful but you haven't
updated. e.g. vcnl4000_measure(), vcnl4040_write_als_it()
vcnl4040_write_als_period() and maybe more.

I don't mind a mix of guard() and non guard() but only when that is
because one approach is better suited to a particular bit of code than the other.
Here it looks like at least some other functions would benefit similar to the
changes this patch makes.

So I'd like a more complete patch.

One other thing inline.

Thanks,

Jonathan




> @@ -1480,13 +1466,13 @@ static int vcnl4040_write_event_config(struct iio_dev *indio_dev,
>  	u16 val, mask;
>  	struct vcnl4000_data *data = iio_priv(indio_dev);
>  
> -	mutex_lock(&data->vcnl4000_lock);
> +	guard(mutex)(&data->vcnl4000_lock);
>  
>  	switch (chan->type) {
>  	case IIO_LIGHT:
>  		ret = i2c_smbus_read_word_data(data->client, VCNL4200_AL_CONF);
>  		if (ret < 0)
> -			goto out;
> +			return ret;
>  
>  		mask = VCNL4040_ALS_CONF_INT_EN;
>  		if (state)
> @@ -1495,13 +1481,11 @@ static int vcnl4040_write_event_config(struct iio_dev *indio_dev,
>  			val = (ret & ~mask);
>  
>  		data->als_int = FIELD_GET(VCNL4040_ALS_CONF_INT_EN, val);
> -		ret = i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF,
> -						val);
> -		break;
> +		return i2c_smbus_write_word_data(data->client, VCNL4200_AL_CONF, val);
>  	case IIO_PROXIMITY:
>  		ret = i2c_smbus_read_word_data(data->client, VCNL4200_PS_CONF1);
>  		if (ret < 0)
> -			goto out;
> +			return ret;
>  
>  		if (dir == IIO_EV_DIR_RISING)
>  			mask = VCNL4040_PS_IF_AWAY;
> @@ -1511,17 +1495,10 @@ static int vcnl4040_write_event_config(struct iio_dev *indio_dev,
>  		val = state ? (ret | mask) : (ret & ~mask);
>  
>  		data->ps_int = FIELD_GET(VCNL4040_PS_CONF2_PS_INT, val);
> -		ret = i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1,
> -						val);
> -		break;
> +		return i2c_smbus_write_word_data(data->client, VCNL4200_PS_CONF1, val);
>  	default:
> -		break;
> +		return ret;

		return -EVINAL;

and drop the intialization of ret.

>  	}
> -
> -out:
> -	mutex_unlock(&data->vcnl4000_lock);
> -
> -	return ret;
>  }
>  
>  static irqreturn_t vcnl4040_irq_thread(int irq, void *p)