1. Patchew
  2. linux
  3. View series

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

Matheus Giarola posted 1 patch 1 month, 3 weeks ago 
drivers/iio/adc/ad7280a.c | 43 ++++++++++++++++-----------------------
1 file changed, 17 insertions(+), 26 deletions(-)
[PATCH v5] iio: adc: ad7280a: replace mutex_lock() with guard(mutex)
Posted by Matheus Giarola 1 month, 3 weeks ago 
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.

In ad7280a_write_thresh(), replace break with return on error.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Matheus Giarola <matheusgiarola@usp.br>
---
v5:
- replace braces with scoped_guard() in ad7280_show_balance_timer()
- replace breaks with return on error in ad7280a_write_thresh()

v4:
- limit guard(mutex) scope in ad7280_show_balance_timer()

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 | 43 ++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/adc/ad7280a.c b/drivers/iio/adc/ad7280a.c
index f50e2b3121bf..156309d2449c 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,10 @@ 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,
+	scoped_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 +550,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 +735,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 */
@@ -748,7 +746,7 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
 			ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
 					   1, value);
 			if (ret)
-				break;
+				return ret;
 			st->cell_threshhigh = value;
 			break;
 		case IIO_EV_DIR_FALLING:
@@ -756,12 +754,11 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
 			ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
 					   1, value);
 			if (ret)
-				break;
+				return ret;
 			st->cell_threshlow = value;
 			break;
 		default:
-			ret = -EINVAL;
-			goto err_unlock;
+			return -EINVAL;
 		}
 		break;
 	case IIO_TEMP:
@@ -773,7 +770,7 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
 			ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
 					   1, value);
 			if (ret)
-				break;
+				return ret;
 			st->aux_threshhigh = value;
 			break;
 		case IIO_EV_DIR_FALLING:
@@ -781,23 +778,17 @@ static int ad7280a_write_thresh(struct iio_dev *indio_dev,
 			ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, addr,
 					   1, value);
 			if (ret)
-				break;
+				return ret;
 			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;
+	return 0;
 }
 
 static irqreturn_t ad7280_event_handler(int irq, void *private)
@@ -884,14 +875,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 +889,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 v5] iio: adc: ad7280a: replace mutex_lock() with guard(mutex)
Posted by Andy Shevchenko 1 month, 2 weeks ago 
On Tue, Apr 21, 2026 at 04:08:55PM -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).
> 
> Also, limit guard(mutex) scope in ad7280_show_balance_timer()
> to keep it locked just when necessary.
> 
> In ad7280a_write_thresh(), replace break with return on error.

...

> 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,
> +	scoped_guard(mutex, &st->lock) {
> +		ret = ad7280_read_reg(st, chan->address >> 8,
>  			      (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> -	mutex_unlock(&st->lock);
> +	}

Not sure if we need {} here (as it's one statement body), but the indentation
now got broken of the second line there.

What about

	u8 devaddr = chan->address >> 8;
	u8 addr = chan->address;
	// ...or use existing names in the driver for the same things

	scoped_guard(mutex, &st->lock)
		ret = ad7280_read_reg(st, devaddr, addr + AD7280A_CB1_TIMER_REG);

?

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v5] iio: adc: ad7280a: replace mutex_lock() with guard(mutex)
Posted by Andy Shevchenko 1 month, 2 weeks ago 
On Wed, Apr 22, 2026 at 10:44:36AM +0300, Andy Shevchenko wrote:
> On Tue, Apr 21, 2026 at 04:08:55PM -0300, Matheus Giarola wrote:

...

> > 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,
> > +	scoped_guard(mutex, &st->lock) {
> > +		ret = ad7280_read_reg(st, chan->address >> 8,
> >  			      (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> > -	mutex_unlock(&st->lock);
> > +	}
> 
> Not sure if we need {} here (as it's one statement body), but the indentation
> now got broken of the second line there.
> 
> What about
> 
> 	u8 devaddr = chan->address >> 8;
> 	u8 addr = chan->address;

Just checked the second one s/addr/ch/ as per ad7280_store_balance_sw()
implementation.

> 	// ...or use existing names in the driver for the same things
> 
> 	scoped_guard(mutex, &st->lock)
> 		ret = ad7280_read_reg(st, devaddr, addr + AD7280A_CB1_TIMER_REG);
> 
> ?

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v5] iio: adc: ad7280a: replace mutex_lock() with guard(mutex)
Posted by Matheus Giarola 1 month, 2 weeks ago 
Hi Andy,
Thanks for the review. I noticed that there's another student working on the
same change, so we decided to join efforts and continue on his patch,
dropping this one.

The other patch can be found at:
https://lore.kernel.org/linux-iio/20260417131029.12568-1-lucas@ciziks.com/

Best regards,
Matheus.

On Wed, Apr 22, 2026 at 4:46 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 22, 2026 at 10:44:36AM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 21, 2026 at 04:08:55PM -0300, Matheus Giarola wrote:
>
> ...
>
> > > 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,
> > > +   scoped_guard(mutex, &st->lock) {
> > > +           ret = ad7280_read_reg(st, chan->address >> 8,
> > >                           (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> > > -   mutex_unlock(&st->lock);
> > > +   }
> >
> > Not sure if we need {} here (as it's one statement body), but the indentation
> > now got broken of the second line there.
> >
> > What about
> >
> >       u8 devaddr = chan->address >> 8;
> >       u8 addr = chan->address;
>
> Just checked the second one s/addr/ch/ as per ad7280_store_balance_sw()
> implementation.
>
> >       // ...or use existing names in the driver for the same things
> >
> >       scoped_guard(mutex, &st->lock)
> >               ret = ad7280_read_reg(st, devaddr, addr + AD7280A_CB1_TIMER_REG);
> >
> > ?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Re: [PATCH v5] iio: adc: ad7280a: replace mutex_lock() with guard(mutex)
Posted by Andy Shevchenko 1 month, 2 weeks ago 
On Wed, Apr 22, 2026 at 08:03:43PM -0300, Matheus Giarola wrote:

First of all, do not top-post!

> Thanks for the review. I noticed that there's another student working on the
> same change, so we decided to join efforts and continue on his patch,
> dropping this one.
> 
> The other patch can be found at:
> https://lore.kernel.org/linux-iio/20260417131029.12568-1-lucas@ciziks.com/

Make sure that student sees my review that I gave against your contribution.
It will save my time when reviewing that one.

> On Wed, Apr 22, 2026 at 4:46 AM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Wed, Apr 22, 2026 at 10:44:36AM +0300, Andy Shevchenko wrote:
> > > On Tue, Apr 21, 2026 at 04:08:55PM -0300, Matheus Giarola wrote:
> >
> > ...
> >
> > > > 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,
> > > > +   scoped_guard(mutex, &st->lock) {
> > > > +           ret = ad7280_read_reg(st, chan->address >> 8,
> > > >                           (chan->address & 0xFF) + AD7280A_CB1_TIMER_REG);
> > > > -   mutex_unlock(&st->lock);
> > > > +   }
> > >
> > > Not sure if we need {} here (as it's one statement body), but the indentation
> > > now got broken of the second line there.
> > >
> > > What about
> > >
> > >       u8 devaddr = chan->address >> 8;
> > >       u8 addr = chan->address;
> >
> > Just checked the second one s/addr/ch/ as per ad7280_store_balance_sw()
> > implementation.
> >
> > >       // ...or use existing names in the driver for the same things
> > >
> > >       scoped_guard(mutex, &st->lock)
> > >               ret = ad7280_read_reg(st, devaddr, addr + AD7280A_CB1_TIMER_REG);
> > >
> > > ?

-- 
With Best Regards,
Andy Shevchenko

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.