diff mbox series

[v3,8/8] iio: adc: ad7606: switch mutexes to scoped_guard

Message ID 20240702-cleanup-ad7606-v3-8-57fd02a4e2aa@baylibre.com (mailing list archive)
State Not Applicable
Headers show
Series iio: adc: ad7606: Improvements | expand

Commit Message

Guillaume Stols July 2, 2024, 5:34 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 | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

Comments

Nuno Sá July 3, 2024, 11:12 a.m. UTC | #1 
On Tue, 2024-07-02 at 17:34 +0000, Guillaume Stols 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>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Markus Elfring July 6, 2024, 12:56 p.m. UTC | #2 
> Switching to scoped_guard simplifies the code and avoids to take care to
> unlock the mutex in case of premature return.

Can such a change description become more imperative?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc6#n94


…
> +++ b/drivers/iio/adc/ad7606.c
…
> @@ -124,19 +122,19 @@ static irqreturn_t ad7606_trigger_handler(int irq, void *p)
…
>  	ret = ad7606_read_samples(st);
> -	if (ret == 0)
> -		iio_push_to_buffers_with_timestamp(indio_dev, st->data,
> -						   iio_get_time_ns(indio_dev));
> +	if (ret)
> +		goto error_ret;
>
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->data,
> +					   iio_get_time_ns(indio_dev));
> +error_ret:
>  	iio_trigger_notify_done(indio_dev->trig);
…

I find that these control flow adjustments do not fit to the changelog.
They can be offered in another update step (on demand), can't they?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc6#n81


There are no scoped_guard() calls performed in your patch.
How do you think about to use a summary phrase like “Switch mutex calls to mutex guard usage”?

Regards,
Markus
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index 50ccc245e314..539e4a8621fe 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,19 +122,19 @@  static irqreturn_t ad7606_trigger_handler(int irq, void *p)
 	struct ad7606_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	ret = ad7606_read_samples(st);
-	if (ret == 0)
-		iio_push_to_buffers_with_timestamp(indio_dev, st->data,
-						   iio_get_time_ns(indio_dev));
+	if (ret)
+		goto error_ret;
 
+	iio_push_to_buffers_with_timestamp(indio_dev, st->data,
+					   iio_get_time_ns(indio_dev));
+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);
 
-	mutex_unlock(&st->lock);
-
 	return IRQ_HANDLED;
 }
 
@@ -257,19 +255,17 @@  static int ad7606_write_raw(struct iio_dev *indio_dev,
 	struct ad7606_state *st = iio_priv(indio_dev);
 	int i, ret, ch = 0;
 
+	guard(mutex)(&st->lock);
+
 	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);
+		if (ret < 0)
 			return ret;
-		}
 		st->range[ch] = i;
-		mutex_unlock(&st->lock);
 
 		return 0;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
@@ -277,14 +273,9 @@  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);
+		if (ret < 0)
 			return ret;
-		}
-		st->oversampling = st->oversampling_avail[i];
-		mutex_unlock(&st->lock);
 
 		return 0;
 	default: