Re: [PATCH v4] iio: adc: ad7280a: replace mutex_lock() with guard(mutex)

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 12 Apr 2026 12:47:48 -0300
Matheus Giarola <matheustpgiarola@gmail.com> wrote:

> Use guard(mutex) instead of mutex_lock()/mutex_unlock(),
> ensuring the mutex is released automatically when leaving
> the function scope. The change improves error handling and
> avoids issues such as missing unlocks. It also simplifies the
> code by removing 'err_unlock' label and several mutex_unlock()
> calls.
> 
> As suggested, in ad7280_read_raw(), wrap the IIO_CHAN_INFO_RAW
> case in braces, providing a scope for the implicit variable
> declared by guard(mutex).
> 
> Also, limit guard(mutex) scope in ad7280_show_balance_timer()
> to keep it locked just when necessary.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Matheus Giarola <matheusgiarola@usp.br>
> ---
> v4:
> - limit guard(mutex) scope in ad7280_show_balance_timer()

Some confusion has occured here.  The braces thing works for some
places (where we bring only trivial extra operations under the lock)
but don't use it where scoped_guard() is cleaner.

> 
> v3:
> - correct v2 patch tags
> 
> v2:
> - fix missing scope in ad7280_read_raw() by wrapping IIO_CHAN_INFO_RAW
>   case in braces so as to fix build errors.
> 
>  drivers/iio/adc/ad7280a.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7280a.c b/drivers/iio/adc/ad7280a.c
> index ba12a3796e2b..f3cbf8810daa 100644
> --- a/drivers/iio/adc/ad7280a.c
> +++ b/drivers/iio/adc/ad7280a.c
...

> @@ -519,10 +518,11 @@ static ssize_t ad7280_show_balance_timer(struct iio_dev *indio_dev,
>  	unsigned int msecs;
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> -	ret = ad7280_read_reg(st, chan->address >> 8,
> +	{
> +		guard(mutex)(&st->lock);
> +		ret = ad7280_read_reg(st, chan->address >> 8,
>  			      (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> -	mutex_unlock(&st->lock);

Not this.  For this case you scoped_guard() or just take the view
that it doesn't matter if we hold the lock a bit longer.


> +	}
>  
>  	if (ret < 0)
>  		return ret;
> @@ -551,11 +551,10 @@ static ssize_t ad7280_store_balance_timer(struct iio_dev *indio_dev,
>  	if (val > 31)
>  		return -EINVAL;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  	ret = ad7280_write(st, chan->address >> 8,
>  			   (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG, 0,
>  			   FIELD_PREP(AD7280A_CB_TIMER_VAL_MSK, val));
> -	mutex_unlock(&st->lock);
>  
>  	return ret ? ret : len;
>  }
> @@ -737,7 +736,7 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
>  	if (val2 != 0)
>  		return -EINVAL;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  	switch (chan->type) {
>  	case IIO_VOLTAGE:
>  		value = ((val - 1000) * 100) / 1568; /* LSB 15.68mV */
> @@ -760,8 +759,7 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
Just above this is:
			if (ret)
				break;

Make that
			if (ret)
				return ret;

and same for other such breaks on error.


>  			st->cell_threshlow = value;
>  			break;
>  		default:
> -			ret = -EINVAL;
> -			goto err_unlock;
> +			return -EINVAL;
>  		}
>  		break;
>  	case IIO_TEMP:
> @@ -785,18 +783,12 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
>  			st->aux_threshlow = value;
>  			break;
>  		default:
> -			ret = -EINVAL;
> -			goto err_unlock;
> +			return -EINVAL;
>  		}
>  		break;
>  	default:
> -		ret = -EINVAL;
> -		goto err_unlock;
> +		return -EINVAL;
>  	}
> -
> -err_unlock:
> -	mutex_unlock(&st->lock);
> -
>  	return ret;
maybe with the changes suggested above used throughout we might not
be able to get here with out ret == 0;
In which case make that obvious with
	return 0;

>  }
>  
> @@ -884,14 +876,13 @@ static int ad7280_read_raw(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	switch (m) {
> -	case IIO_CHAN_INFO_RAW:
> -		mutex_lock(&st->lock);
> +	case IIO_CHAN_INFO_RAW: {
> +		guard(mutex)(&st->lock);
>  		if (chan->address == AD7280A_ALL_CELLS)
>  			ret = ad7280_read_all_channels(st, st->scan_cnt, NULL);
>  		else
>  			ret = ad7280_read_channel(st, chan->address >> 8,
>  						  chan->address & 0xFF);
> -		mutex_unlock(&st->lock);
>  
>  		if (ret < 0)
>  			return ret;
> @@ -899,6 +890,7 @@ static int ad7280_read_raw(struct iio_dev *indio_dev,
>  		*val = ret;
>  
>  		return IIO_VAL_INT;
> +	}
>  	case IIO_CHAN_INFO_SCALE:
>  		if ((chan->address & 0xFF) <= AD7280A_CELL_VOLTAGE_6_REG)
>  			*val = 4000;