1. Patchew
  2. linux
  3. [PATCH v3 0/4] iio: mpl3115: add support for DRDY interrupt
  4. View patch

[PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h

Antoni Pokusinski posted 4 patches 5 days ago
[PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h
Posted by Antoni Pokusinski 5 days ago 
Include linux/cleanup.h and use the scoped_guard() to simplify the code.

Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
---
 drivers/iio/pressure/mpl3115.c | 42 +++++++++++++++-------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 579da60ef441..80af672f65c6 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -10,14 +10,16 @@
  * interrupts, user offset correction, raw mode
  */
 
-#include <linux/module.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/module.h>
+
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
-#include <linux/delay.h>
 
 #define MPL3115_STATUS 0x00
 #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
@@ -163,32 +165,26 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
 	u8 buffer[16] __aligned(8) = { };
 	int ret, pos = 0;
 
-	mutex_lock(&data->lock);
-	ret = mpl3115_request(data);
-	if (ret < 0) {
-		mutex_unlock(&data->lock);
-		goto done;
-	}
-
-	if (test_bit(0, indio_dev->active_scan_mask)) {
-		ret = i2c_smbus_read_i2c_block_data(data->client,
-			MPL3115_OUT_PRESS, 3, &buffer[pos]);
-		if (ret < 0) {
-			mutex_unlock(&data->lock);
+	scoped_guard(mutex, &data->lock) {
+		ret = mpl3115_request(data);
+		if (ret < 0)
 			goto done;
+
+		if (test_bit(0, indio_dev->active_scan_mask)) {
+			ret = i2c_smbus_read_i2c_block_data(data->client,
+				MPL3115_OUT_PRESS, 3, &buffer[pos]);
+			if (ret < 0)
+				goto done;
+			pos += 4;
 		}
-		pos += 4;
-	}
 
-	if (test_bit(1, indio_dev->active_scan_mask)) {
-		ret = i2c_smbus_read_i2c_block_data(data->client,
-			MPL3115_OUT_TEMP, 2, &buffer[pos]);
-		if (ret < 0) {
-			mutex_unlock(&data->lock);
-			goto done;
+		if (test_bit(1, indio_dev->active_scan_mask)) {
+			ret = i2c_smbus_read_i2c_block_data(data->client,
+				MPL3115_OUT_TEMP, 2, &buffer[pos]);
+			if (ret < 0)
+				goto done;
 		}
 	}
-	mutex_unlock(&data->lock);
 
 	iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer),
 				    iio_get_time_ns(indio_dev));
-- 
2.25.1
Re: [PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h
Posted by Jonathan Cameron 4 days, 6 hours ago 
On Sat, 27 Sep 2025 00:01:48 +0200
Antoni Pokusinski <apokusinski01@gmail.com> wrote:

> Include linux/cleanup.h and use the scoped_guard() to simplify the code.
See below. I'm not sure this is in general a good idea in this driver, but
see the comments below.  I think more traditional factoring out of the code
under the lock into a helper function should be the main change here.
That might or might not make sense combined with a scoped_guard().


> 
> Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> ---
>  drivers/iio/pressure/mpl3115.c | 42 +++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> index 579da60ef441..80af672f65c6 100644
> --- a/drivers/iio/pressure/mpl3115.c
> +++ b/drivers/iio/pressure/mpl3115.c
> @@ -10,14 +10,16 @@
>   * interrupts, user offset correction, raw mode
>   */
>  
> -#include <linux/module.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/module.h>
> +
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
> -#include <linux/delay.h>
>  
>  #define MPL3115_STATUS 0x00
>  #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
> @@ -163,32 +165,26 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
>  	u8 buffer[16] __aligned(8) = { };
>  	int ret, pos = 0;
>  
> -	mutex_lock(&data->lock);
> -	ret = mpl3115_request(data);
> -	if (ret < 0) {
> -		mutex_unlock(&data->lock);
> -		goto done;
> -	}
> -
> -	if (test_bit(0, indio_dev->active_scan_mask)) {
> -		ret = i2c_smbus_read_i2c_block_data(data->client,
> -			MPL3115_OUT_PRESS, 3, &buffer[pos]);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> +	scoped_guard(mutex, &data->lock) {
> +		ret = mpl3115_request(data);
> +		if (ret < 0)
>  			goto done;
Read the guidance in cleanup.h.  Whilst what you have here is actually not
a bug, it is considered fragile to combine gotos and scoped cleanup in a function.
Sometimes that means that if we are using guards() we need to also duplicate
some error handling.

So, the way to avoid that is to factor out the stuff under the goto to a helper
function.  That function than then return directly on errors like this.

Looks something like

	scoped_guard(mutex, &data->lock) {
		ret = mpl3115_fill_buffer(data, buffer);
		if (ret) {
			iio_trigger_notify_done(indio_dev->trig);
			return IRQ_HANDLED;
		}
	}

	iio_push_to_buffers_with_ts...
	iio_trigger_notify_done(indio_dev->trig);
	return IRQ_HANDLED;


However, it is also worth keeping in mind that sometimes scoped cleanup
of which guards are a special case is not the right solution for a whole
driver. I'm not sure if it is worth while in this case, but try the approach
mentioned above and see how it looks.

Alternative would still be to factor out the helper, but instead just have
	mutex_lock(&data->lock);
	ret = mpl3115_fill_buffer(data, buffer);
	mutex_unlock(&data->lock);
	if (ret)
		goto...


Jonathan

> +
> +		if (test_bit(0, indio_dev->active_scan_mask)) {
> +			ret = i2c_smbus_read_i2c_block_data(data->client,
> +				MPL3115_OUT_PRESS, 3, &buffer[pos]);
> +			if (ret < 0)
> +				goto done;
> +			pos += 4;
>  		}
> -		pos += 4;
> -	}
>  
> -	if (test_bit(1, indio_dev->active_scan_mask)) {
> -		ret = i2c_smbus_read_i2c_block_data(data->client,
> -			MPL3115_OUT_TEMP, 2, &buffer[pos]);
> -		if (ret < 0) {
> -			mutex_unlock(&data->lock);
> -			goto done;
> +		if (test_bit(1, indio_dev->active_scan_mask)) {
> +			ret = i2c_smbus_read_i2c_block_data(data->client,
> +				MPL3115_OUT_TEMP, 2, &buffer[pos]);
> +			if (ret < 0)
> +				goto done;
>  		}
>  	}
> -	mutex_unlock(&data->lock);
>  
>  	iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer),
>  				    iio_get_time_ns(indio_dev));
Re: [PATCH v3 2/4] iio: mpl3115: use guards from cleanup.h
Posted by Antoni Pokusinski 3 days, 12 hours ago 
On Sat, Sep 27, 2025 at 05:36:21PM +0100, Jonathan Cameron wrote:
> On Sat, 27 Sep 2025 00:01:48 +0200
> Antoni Pokusinski <apokusinski01@gmail.com> wrote:
> 
> > Include linux/cleanup.h and use the scoped_guard() to simplify the code.
> See below. I'm not sure this is in general a good idea in this driver, but
> see the comments below.  I think more traditional factoring out of the code
> under the lock into a helper function should be the main change here.
> That might or might not make sense combined with a scoped_guard().
> 
> 
> > 
> > Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> > ---
> >  drivers/iio/pressure/mpl3115.c | 42 +++++++++++++++-------------------
> >  1 file changed, 19 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index 579da60ef441..80af672f65c6 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> > @@ -10,14 +10,16 @@
> >   * interrupts, user offset correction, raw mode
> >   */
> >  
> > -#include <linux/module.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> >  #include <linux/i2c.h>
> > +#include <linux/module.h>
> > +
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/triggered_buffer.h>
> > -#include <linux/delay.h>
> >  
> >  #define MPL3115_STATUS 0x00
> >  #define MPL3115_OUT_PRESS 0x01 /* MSB first, 20 bit */
> > @@ -163,32 +165,26 @@ static irqreturn_t mpl3115_trigger_handler(int irq, void *p)
> >  	u8 buffer[16] __aligned(8) = { };
> >  	int ret, pos = 0;
> >  
> > -	mutex_lock(&data->lock);
> > -	ret = mpl3115_request(data);
> > -	if (ret < 0) {
> > -		mutex_unlock(&data->lock);
> > -		goto done;
> > -	}
> > -
> > -	if (test_bit(0, indio_dev->active_scan_mask)) {
> > -		ret = i2c_smbus_read_i2c_block_data(data->client,
> > -			MPL3115_OUT_PRESS, 3, &buffer[pos]);
> > -		if (ret < 0) {
> > -			mutex_unlock(&data->lock);
> > +	scoped_guard(mutex, &data->lock) {
> > +		ret = mpl3115_request(data);
> > +		if (ret < 0)
> >  			goto done;
> Read the guidance in cleanup.h.  Whilst what you have here is actually not
> a bug, it is considered fragile to combine gotos and scoped cleanup in a function.
> Sometimes that means that if we are using guards() we need to also duplicate
> some error handling.
> 
> So, the way to avoid that is to factor out the stuff under the goto to a helper
> function.  That function than then return directly on errors like this.
> 
> Looks something like
> 
> 	scoped_guard(mutex, &data->lock) {
> 		ret = mpl3115_fill_buffer(data, buffer);
> 		if (ret) {
> 			iio_trigger_notify_done(indio_dev->trig);
> 			return IRQ_HANDLED;
> 		}
> 	}
> 
> 	iio_push_to_buffers_with_ts...
> 	iio_trigger_notify_done(indio_dev->trig);
> 	return IRQ_HANDLED;
> 
> 
> However, it is also worth keeping in mind that sometimes scoped cleanup
> of which guards are a special case is not the right solution for a whole
> driver. I'm not sure if it is worth while in this case, but try the approach
> mentioned above and see how it looks.
> 
> Alternative would still be to factor out the helper, but instead just have
> 	mutex_lock(&data->lock);
> 	ret = mpl3115_fill_buffer(data, buffer);
> 	mutex_unlock(&data->lock);
> 	if (ret)
> 		goto...
> 
> 
> Jonathan
>
Thanks for the explanation, both approaches look quite neat to me.
However, if we use scoped_guard() then the iio_trigger_notify_done and
return IRQ_HANDLED are duplicated, so I'd lean slightly towards the
mutex_lock/mutex_unlock solution.

> > +
> > +		if (test_bit(0, indio_dev->active_scan_mask)) {
> > +			ret = i2c_smbus_read_i2c_block_data(data->client,
> > +				MPL3115_OUT_PRESS, 3, &buffer[pos]);
> > +			if (ret < 0)
> > +				goto done;
> > +			pos += 4;
> >  		}
> > -		pos += 4;
> > -	}
> >  
> > -	if (test_bit(1, indio_dev->active_scan_mask)) {
> > -		ret = i2c_smbus_read_i2c_block_data(data->client,
> > -			MPL3115_OUT_TEMP, 2, &buffer[pos]);
> > -		if (ret < 0) {
> > -			mutex_unlock(&data->lock);
> > -			goto done;
> > +		if (test_bit(1, indio_dev->active_scan_mask)) {
> > +			ret = i2c_smbus_read_i2c_block_data(data->client,
> > +				MPL3115_OUT_TEMP, 2, &buffer[pos]);
> > +			if (ret < 0)
> > +				goto done;
> >  		}
> >  	}
> > -	mutex_unlock(&data->lock);
> >  
> >  	iio_push_to_buffers_with_ts(indio_dev, buffer, sizeof(buffer),
> >  				    iio_get_time_ns(indio_dev));
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.