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