diff mbox series

[v2,10/10] iio: adc: ad7606: switch mutexes to scoped_guard

Message ID 20240628-cleanup-ad7606-v2-10-96e02f90256d@baylibre.com (mailing list archive)
State Changes Requested
Headers show
Series iio: adc: ad7606: Improvements | expand

Commit Message

Guillaume Stols June 28, 2024, 2:48 p.m. UTC 
Switching to scoped_guard simplifies the code and avoids to take care to
unlock the mutex in case of premature return.

Signed-off-by: Guillaume Stols <gstols@baylibre.com>
---
 drivers/iio/adc/ad7606.c | 60 ++++++++++++++++++++++--------------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

Comments

Jonathan Cameron June 29, 2024, 3:36 p.m. UTC | #1 
On Fri, 28 Jun 2024 14:48:28 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> Switching to scoped_guard simplifies the code and avoids to take care to
> unlock the mutex in case of premature return.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
> ---
>  drivers/iio/adc/ad7606.c | 60 ++++++++++++++++++++++--------------------------
>  1 file changed, 27 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
> index 50ccc245e314..3c439787d130 100644
> --- a/drivers/iio/adc/ad7606.c
> +++ b/drivers/iio/adc/ad7606.c
> @@ -69,19 +69,17 @@ static int ad7606_reg_access(struct iio_dev *indio_dev,
>  	struct ad7606_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);
> +
>  	if (readval) {
>  		ret = st->bops->reg_read(st, reg);
>  		if (ret < 0)
> -			goto err_unlock;
> +			return ret;
>  		*readval = ret;
> -		ret = 0;
> +		return 0;
>  	} else {
> -		ret = st->bops->reg_write(st, reg, writeval);
> +		return st->bops->reg_write(st, reg, writeval);
>  	}
> -err_unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
>  }
>  
>  static int ad7606_read_samples(struct ad7606_state *st)
> @@ -124,18 +122,18 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
>  	struct ad7606_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> -	mutex_lock(&st->lock);

Why not simply a guard(mutex)(&st->lock) ?

Then we avoid the somewhat nasty label in an nested block of code.

> +	scoped_guard(mutex, &st->lock) {
> +		ret = ad7606_read_samples(st);
> +		if (ret)
> +			goto error_ret;
>  
> -	ret = ad7606_read_samples(st);
> -	if (ret == 0)
>  		iio_push_to_buffers_with_timestamp(indio_dev, st->data,
>  						   iio_get_time_ns(indio_dev));
> -
> -	iio_trigger_notify_done(indio_dev->trig);
> -	/* The rising edge of the CONVST signal starts a new conversion. */
> -	gpiod_set_value(st->gpio_convst, 1);
> -
> -	mutex_unlock(&st->lock);
> +error_ret:
> +		iio_trigger_notify_done(indio_dev->trig);
> +		/* The rising edge of the CONVST signal starts a new conversion. */
> +		gpiod_set_value(st->gpio_convst, 1);
> +	}
>  
>  	return IRQ_HANDLED;
>  }
> @@ -259,17 +257,15 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		mutex_lock(&st->lock);
> -		i = find_closest(val2, st->scale_avail, st->num_scales);
> -		if (st->sw_mode_en)
> -			ch = chan->address;
> -		ret = st->write_scale(indio_dev, ch, i);
> -		if (ret < 0) {
> -			mutex_unlock(&st->lock);
> -			return ret;
> +		scoped_guard(mutex, &st->lock) {

The mutex is grabbed in all paths that actually do anything.
Pull it out of the switch and use
guard(mutex)(&st->lock);

That will reduce the changes needed and give the same cleanups.
I doubt we care about potentially slowing down the path that returns an error
as we are writing something unwriteable.

> +			i = find_closest(val2, st->scale_avail, st->num_scales);
> +			if (st->sw_mode_en)
> +				ch = chan->address;
> +			ret = st->write_scale(indio_dev, ch, i);
> +			if (ret < 0)
> +				return ret;
> +			st->range[ch] = i;
>  		}
> -		st->range[ch] = i;
> -		mutex_unlock(&st->lock);
>  
>  		return 0;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> @@ -277,14 +273,12 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		i = find_closest(val, st->oversampling_avail,
>  				 st->num_os_ratios);
> -		mutex_lock(&st->lock);
> -		ret = st->write_os(indio_dev, i);
> -		if (ret < 0) {
> -			mutex_unlock(&st->lock);
> -			return ret;
> +		scoped_guard(mutex, &st->lock) {
> +			ret = st->write_os(indio_dev, i);
> +			if (ret < 0)
> +				return ret;
> +			st->oversampling = st->oversampling_avail[i];
>  		}
> -		st->oversampling = st->oversampling_avail[i];
> -		mutex_unlock(&st->lock);
>  
>  		return 0;
>  	default:
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 50ccc245e314..3c439787d130 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -69,19 +69,17 @@  static int ad7606_reg_access(struct iio_dev *indio_dev,
 	struct ad7606_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
+
 	if (readval) {
 		ret = st->bops->reg_read(st, reg);
 		if (ret < 0)
-			goto err_unlock;
+			return ret;
 		*readval = ret;
-		ret = 0;
+		return 0;
 	} else {
-		ret = st->bops->reg_write(st, reg, writeval);
+		return st->bops->reg_write(st, reg, writeval);
 	}
-err_unlock:
-	mutex_unlock(&st->lock);
-	return ret;
 }
 
 static int ad7606_read_samples(struct ad7606_state *st)
@@ -124,18 +122,18 @@  static irqreturn_t ad7606_trigger_handler(int irq, void *p)
 	struct ad7606_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->lock);
+	scoped_guard(mutex, &st->lock) {
+		ret = ad7606_read_samples(st);
+		if (ret)
+			goto error_ret;
 
-	ret = ad7606_read_samples(st);
-	if (ret == 0)
 		iio_push_to_buffers_with_timestamp(indio_dev, st->data,
 						   iio_get_time_ns(indio_dev));
-
-	iio_trigger_notify_done(indio_dev->trig);
-	/* The rising edge of the CONVST signal starts a new conversion. */
-	gpiod_set_value(st->gpio_convst, 1);
-
-	mutex_unlock(&st->lock);
+error_ret:
+		iio_trigger_notify_done(indio_dev->trig);
+		/* The rising edge of the CONVST signal starts a new conversion. */
+		gpiod_set_value(st->gpio_convst, 1);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -259,17 +257,15 @@  static int ad7606_write_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
-		mutex_lock(&st->lock);
-		i = find_closest(val2, st->scale_avail, st->num_scales);
-		if (st->sw_mode_en)
-			ch = chan->address;
-		ret = st->write_scale(indio_dev, ch, i);
-		if (ret < 0) {
-			mutex_unlock(&st->lock);
-			return ret;
+		scoped_guard(mutex, &st->lock) {
+			i = find_closest(val2, st->scale_avail, st->num_scales);
+			if (st->sw_mode_en)
+				ch = chan->address;
+			ret = st->write_scale(indio_dev, ch, i);
+			if (ret < 0)
+				return ret;
+			st->range[ch] = i;
 		}
-		st->range[ch] = i;
-		mutex_unlock(&st->lock);
 
 		return 0;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -277,14 +273,12 @@  static int ad7606_write_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		i = find_closest(val, st->oversampling_avail,
 				 st->num_os_ratios);
-		mutex_lock(&st->lock);
-		ret = st->write_os(indio_dev, i);
-		if (ret < 0) {
-			mutex_unlock(&st->lock);
-			return ret;
+		scoped_guard(mutex, &st->lock) {
+			ret = st->write_os(indio_dev, i);
+			if (ret < 0)
+				return ret;
+			st->oversampling = st->oversampling_avail[i];
 		}
-		st->oversampling = st->oversampling_avail[i];
-		mutex_unlock(&st->lock);
 
 		return 0;
 	default: