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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 20 Apr 2026 23:10:01 -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>

I think a few of these can be done in a cleaner fashion - see below.

Jonathan

>  
>  static int ltr501_als_read_samp_period(const struct ltr501_data *data, int *val)
> @@ -505,9 +499,8 @@ 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)
>  			data->als_period = period;
>  
> @@ -525,9 +518,8 @@ 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;
This and the one above are more complex as we'd not want to imply protection of
ps_period by including it in the lock if that doesn't make sense.

>  
> @@ -669,18 +661,16 @@ static int ltr501_read_info_raw(struct ltr501_data *data,
>  
>  	switch (chan->type) {
>  	case IIO_INTENSITY:
> -		mutex_lock(&data->lock_als);
> -		ret = ltr501_read_als(data, buf);
> -		mutex_unlock(&data->lock_als);
> +		scoped_guard(mutex, &data->lock_als)
> +			ret = ltr501_read_als(data, buf);
>  		if (ret < 0)
>  			return ret;
>  		*val = le16_to_cpu(chan->address == LTR501_ALS_DATA1 ?
>  				   buf[0] : buf[1]);

This is cheap and local stuff only. For these two cases i'd use {} + guard() as below and
not worry about doing this under the lock.

>  		return IIO_VAL_INT;
>  	case IIO_PROXIMITY:
> -		mutex_lock(&data->lock_ps);
> -		ret = ltr501_read_ps(data);
> -		mutex_unlock(&data->lock_ps);
> +		scoped_guard(mutex, &data->lock_ps)
> +			ret = ltr501_read_ps(data);
>  		if (ret < 0)
>  			return ret;
>  		*val = ret & LTR501_PS_DATA_MASK;
> @@ -705,9 +695,8 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
>  			if (!iio_device_claim_direct(indio_dev))
>  				return -EBUSY;
>  
> -			mutex_lock(&data->lock_als);
> -			ret = ltr501_read_als(data, buf);
> -			mutex_unlock(&data->lock_als);
> +			scoped_guard(mutex, &data->lock_als)
> +				ret = ltr501_read_als(data, buf);
This use of scoped_guard() does make sense as there is non trivial work to
do afterwards.
>  			iio_device_release_direct(indio_dev);
>  			if (ret < 0)
>  				return ret;
> @@ -820,9 +809,8 @@ static int __ltr501_write_raw(struct iio_dev *indio_dev,
>  			if (val != 0)
>  				return -EINVAL;
I'd use a guard here (see below) for same reasons. It'll just be after this
initial sanity check.

>  
> -			mutex_lock(&data->lock_als);
> -			ret = ltr501_set_it_time(data, val2);
> -			mutex_unlock(&data->lock_als);
> +			scoped_guard(mutex, &data->lock_als)
> +				ret = ltr501_set_it_time(data, val2);

>  			return ret;
>  		default:
>  			return -EINVAL;
> @@ -971,18 +959,16 @@ static int ltr501_write_thresh(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		switch (dir) {
>  		case IIO_EV_DIR_RISING:
> -			mutex_lock(&data->lock_als);
> -			ret = regmap_bulk_write(data->regmap,
> -						LTR501_ALS_THRESH_UP,
> -						&val, 2);
> -			mutex_unlock(&data->lock_als);
> +			scoped_guard(mutex, &data->lock_als)
> +				ret = regmap_bulk_write(data->regmap,
> +							 LTR501_ALS_THRESH_UP,
> +							 &val, 2);
See below.

>  			return ret;
>  		case IIO_EV_DIR_FALLING:
> -			mutex_lock(&data->lock_als);
> -			ret = regmap_bulk_write(data->regmap,
> -						LTR501_ALS_THRESH_LOW,
> -						&val, 2);
> -			mutex_unlock(&data->lock_als);
> +			scoped_guard(mutex, &data->lock_als)
> +				ret = regmap_bulk_write(data->regmap,
> +							 LTR501_ALS_THRESH_LOW,
> +							 &val, 2);
See below.
>  			return ret;
>  		default:
>  			return -EINVAL;
> @@ -992,18 +978,16 @@ static int ltr501_write_thresh(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		switch (dir) {
>  		case IIO_EV_DIR_RISING:
> -			mutex_lock(&data->lock_ps);
> -			ret = regmap_bulk_write(data->regmap,
> -						LTR501_PS_THRESH_UP,
> -						&val, 2);
> -			mutex_unlock(&data->lock_ps);
> +			scoped_guard(mutex, &data->lock_ps)
> +				ret = regmap_bulk_write(data->regmap,
> +							 LTR501_PS_THRESH_UP,
> +							 &val, 2);
See below.
>  			return ret;
>  		case IIO_EV_DIR_FALLING:
> -			mutex_lock(&data->lock_ps);
> -			ret = regmap_bulk_write(data->regmap,
> -						LTR501_PS_THRESH_LOW,
> -						&val, 2);
> -			mutex_unlock(&data->lock_ps);
> +			scoped_guard(mutex, &data->lock_ps)
> +				ret = regmap_bulk_write(data->regmap,
> +							 LTR501_PS_THRESH_LOW,
> +							 &val, 2);
See below.
>  			return ret;
>  		default:
>  			return -EINVAL;
> @@ -1100,14 +1084,12 @@ static int ltr501_write_event_config(struct iio_dev *indio_dev,
>  
>  	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);
> +		scoped_guard(mutex, &data->lock_als)
> +			ret = regmap_field_write(data->reg_als_intr, state);
>  		return ret;
for these I'd rather see the pattern

	case IIO_INTENSITY: {
		guard(mutex)(&data->lock_als);
		return regmap_field_write();
	}

The scoped_guard() has some annoying issues wrt to compilers and their ability to
see that the loop (it's a for loop underneath) is always entered and so you can't
move the return in there.  The bare guard() form doesn't have that problem.

>  	case IIO_PROXIMITY:
> -		mutex_lock(&data->lock_ps);
> -		ret = regmap_field_write(data->reg_ps_intr, state);
> -		mutex_unlock(&data->lock_ps);
> +		scoped_guard(mutex, &data->lock_ps)
> +			ret = regmap_field_write(data->reg_ps_intr, state);
As above.

>  		return ret;
>  	default:
>  		return -EINVAL;