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

[Jonathan Cameron]

On Fri, 28 Jun 2024 14:48:28 +0000
Guillaume Stols <gstols@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>  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:
>