Re: [PATCH v3 2/3] iio: light: ltr501: use automatic cleanup of locks

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 22 Apr 2026 20:12:56 -0300
rafasales@usp.br wrote:

> From: "Rafael B. Sales" <rafasales@usp.br>
> 
> Replace `mutex_lock()` and `mutex_unlock()` calls with guards
> to reduce boilerplate and allow for simpler code blocks.
> 
> Signed-off-by: Rafael B. Sales <rafasales@usp.br>
> Co-developed-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br>
> Signed-off-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br>
> 
> ---
> Changes in v3:
> - Replaces `scoped_guard()` with {} + `guard()` in some cases
> - Simplifies some return statements
> - Includes cleanup.h header
> 
> Changes in v2:
> - Maintains original locking boundaries
> - Simplifies some return statements

>  static int ltr501_als_read_samp_period(const struct ltr501_data *data, int *val)
> @@ -504,9 +499,10 @@ static int ltr501_write_intr_prst(struct ltr501_data *data,
>  		if (new_val < 0 || new_val > 0x0f)
>  			return -EINVAL;
>  
> -		mutex_lock(&data->lock_als);
> -		ret = regmap_field_write(data->reg_als_prst, new_val);
> -		mutex_unlock(&data->lock_als);
> +		scoped_guard(mutex, &data->lock_als) {
> +			ret = regmap_field_write(data->reg_als_prst, new_val);
> +		}
> +
>  		if (ret >= 0)
Whilst here I'd like these to take more standard form.  

		if (ret)
			return ret;

		data->als_period = period;

		return 0; 

It would be a separate patch though so I don't mind if we leave it for another day.

Also removes suggestion that the current code makes that ret > 0 is a possibility
when it isn't.
>  			data->als_period = period;
>  
> @@ -524,9 +520,10 @@ static int ltr501_write_intr_prst(struct ltr501_data *data,
>  		if (new_val < 0 || new_val > 0x0f)
>  			return -EINVAL;
>  
> -		mutex_lock(&data->lock_ps);
> -		ret = regmap_field_write(data->reg_ps_prst, new_val);
> -		mutex_unlock(&data->lock_ps);
> +		scoped_guard(mutex, &data->lock_ps) {
> +			ret = regmap_field_write(data->reg_ps_prst, new_val);
> +		}
> +
>  		if (ret >= 0)
>  			data->ps_period = period;
Same on this one and any other similar cases.


> @@ -1095,19 +1085,16 @@ static int ltr501_write_event_config(struct iio_dev *indio_dev,
>  				     enum iio_event_direction dir, bool state)
>  {
>  	struct ltr501_data *data = iio_priv(indio_dev);
> -	int ret;
>  
>  	switch (chan->type) {
> -	case IIO_INTENSITY:
> -		mutex_lock(&data->lock_als);
> -		ret = regmap_field_write(data->reg_als_intr, state);
> -		mutex_unlock(&data->lock_als);
> -		return ret;
> -	case IIO_PROXIMITY:
> -		mutex_lock(&data->lock_ps);
> -		ret = regmap_field_write(data->reg_ps_intr, state);
> -		mutex_unlock(&data->lock_ps);
> -		return ret;
> +	case IIO_INTENSITY: {
> +		guard(mutex)(&data->lock_als);
> +		return regmap_field_write(data->reg_als_intr, state);
> +		}
If you end up keeping this stuff, I think conventional style would have
the bracket here:
	}
> +	case IIO_PROXIMITY: {
> +		guard(mutex)(&data->lock_ps);
> +		return regmap_field_write(data->reg_ps_intr, state);
> +		}
>  	default:
>  		return -EINVAL;
>  	}