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

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

[Matheus Giarola]

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).

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Matheus Giarola <matheusgiarola@usp.br>
---
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 | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/ad7280a.c b/drivers/iio/adc/ad7280a.c
index ba12a3796e2b..f34514be1648 100644
--- a/drivers/iio/adc/ad7280a.c
+++ b/drivers/iio/adc/ad7280a.c
@@ -496,7 +496,7 @@ static ssize_t ad7280_store_balance_sw(struct iio_dev *indio_dev,
 	devaddr = chan->address >> 8;
 	ch = chan->address & 0xFF;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	if (readin)
 		st->cb_mask[devaddr] |= BIT(ch);
 	else
@@ -505,7 +505,6 @@ static ssize_t ad7280_store_balance_sw(struct iio_dev *indio_dev,
 	ret = ad7280_write(st, devaddr, AD7280A_CELL_BALANCE_REG, 0,
 			   FIELD_PREP(AD7280A_CELL_BALANCE_CHAN_BITMAP_MSK,
 				      st->cb_mask[devaddr]));
-	mutex_unlock(&st->lock);
 
 	return ret ? ret : len;
 }
@@ -519,10 +518,9 @@ static ssize_t ad7280_show_balance_timer(struct iio_dev *indio_dev,
 	unsigned int msecs;
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 	ret = ad7280_read_reg(st, chan->address >> 8,
 			      (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
-	mutex_unlock(&st->lock);
 
 	if (ret < 0)
 		return ret;
@@ -551,11 +549,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 +734,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 +757,7 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
 			st->cell_threshlow = value;
 			break;
 		default:
-			ret = -EINVAL;
-			goto err_unlock;
+			return -EINVAL;
 		}
 		break;
 	case IIO_TEMP:
@@ -785,18 +781,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;
 }
 
@@ -884,14 +874,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 +888,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;
-- 
2.47.3

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

[Andy Shevchenko]

On Thu, Apr 02, 2026 at 12:15:06AM -0300, Matheus Giarola 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).

...

> static ssize_t ad7280_show_balance_timer(struct iio_dev *indio_dev,

>  	unsigned int msecs;
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
>  	ret = ad7280_read_reg(st, chan->address >> 8,
>  			      (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> -	mutex_unlock(&st->lock);
>  
>  	if (ret < 0)
>  		return ret;

This will include the sysfs_emit() and other things in the critical section. Why?

...

> static int ad7280_read_raw(struct iio_dev *indio_dev,

>  	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;

>  		*val = ret;

This also will be under the lock, but it's fine.

>  		return IIO_VAL_INT;
> +	}

-- 
With Best Regards,
Andy Shevchenko

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

[Matheus Giarola]

On Thu, Apr 2, 2026 at 5:36 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Thu, Apr 02, 2026 at 12:15:06AM -0300, Matheus Giarola 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).
>
> ...
>
> > static ssize_t ad7280_show_balance_timer(struct iio_dev *indio_dev,
>
> >       unsigned int msecs;
> >       int ret;
> >
> > -     mutex_lock(&st->lock);
> > +     guard(mutex)(&st->lock);
> >       ret = ad7280_read_reg(st, chan->address >> 8,
> >                             (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> > -     mutex_unlock(&st->lock);
> >
> >       if (ret < 0)
> >               return ret;
>
> This will include the sysfs_emit() and other things in the critical section. Why?

Hi Andy,
Thanks for pointing that out. The way I wrote it, guard() will
unnecessarily keep the
mutex locked in sysfs_emit() and other parts.
In a previous review, Jonathan suggested using braces instead of scoped_guard()
to limit the scope in the IIO_CHAN_INFO_RAW case. So I'll wrap the
critical section in
braces to fix it.
>
> ...
>
> > static int ad7280_read_raw(struct iio_dev *indio_dev,
>
> >       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;
>
> >               *val = ret;
>
> This also will be under the lock, but it's fine.
Ok, that makes sense. I'll keep it that way then.
>
> >               return IIO_VAL_INT;
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks,
Matheus.