Re: [PATCH 5/7] iio: amplifiers: ad8366: use cleanup.h mutex guard

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 19 Jan 2026 15:43:07 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 19/01/2026 15:36, Rodrigo Alencar via B4 Relay wrote:
> > From: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > 
> > Changes related to mutex handling:
> > - use guard() from cleanup for mutex locking
> > - replace mutex_init() for devm_mutex_init()  
> 
> Why? We see all this from the diff but I do not see benefits.

Also, 2 patches as two different things.
That should make it easier to say what benefits of each is.

A little more comment on comments inline.


> 
> > 
> > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com>
> > ---
> >  drivers/iio/amplifiers/ad8366.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/amplifiers/ad8366.c b/drivers/iio/amplifiers/ad8366.c
> > index 160a8ab0c2ee..26856cb4216e 100644
> > --- a/drivers/iio/amplifiers/ad8366.c
> > +++ b/drivers/iio/amplifiers/ad8366.c
> > @@ -17,6 +17,7 @@
> >   * Copyright 2012-2026 Analog Devices Inc.
> >   */
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/slab.h>
> > @@ -164,7 +165,8 @@ static int ad8366_read_raw(struct iio_dev *indio_dev,
> >  	int ret;
> >  	int code, gain = 0;
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);
> > +
> >  	switch (m) {
> >  	case IIO_CHAN_INFO_HARDWAREGAIN:
> >  		code = st->ch[chan->channel];
> > @@ -210,7 +212,6 @@ static int ad8366_read_raw(struct iio_dev *indio_dev,
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > -	mutex_unlock(&st->lock);  
> 
> This does not simplify code.
Agreed. Without moving to early returns there is little point in this change.
With early returns the argument of easier code flow can be made
though it isn't particularly strong in this case as only two exit
paths and they are right next to each other.

> 
> >  
> >  	return ret;
> >  };
> > @@ -267,7 +268,8 @@ static int ad8366_write_raw(struct iio_dev *indio_dev,
> >  		break;
> >  	}
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);  
> 
> Neither this.
With early returns this one is more obviously useful. Then can argue
above is for consistency.
> 
> > +
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_HARDWAREGAIN:
> >  		st->ch[chan->channel] = code;
> > @@ -276,7 +278,6 @@ static int ad8366_write_raw(struct iio_dev *indio_dev,
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > -	mutex_unlock(&st->lock);
> >  
> >  	return ret;
> >  }
> > @@ -336,10 +337,13 @@ static int ad8366_probe(struct spi_device *spi)
> >  	}
> >  
> >  	spi_set_drvdata(spi, indio_dev);
> > -	mutex_init(&st->lock);
> >  	st->spi = spi;
> >  	st->type = spi_get_device_id(spi)->driver_data;
> >  
> > +	ret = devm_mutex_init(&spi->dev, &st->lock);  
> 
> And this one actually has impact - missing mutex_destroy...
> 
> > +	if (ret)
> > +		return ret;
> > +
> >  	switch (st->type) {
> >  	case ID_AD8366:
> >  		indio_dev->channels = ad8366_channels;
> >   
> 
> 
> Best regards,
> Krzysztof