[PATCH v3 0/2] iio: light: gp2ap020a00f: fix return type and simplify locking

[PATCH v3 0/2] iio: light: gp2ap020a00f: fix return type and simplify locking

[Ethan Tidmore]

Patch 1 is a cleanup that simplifies the locking mechanisms using
guard(), as suggested by Jonathan Cameron. This removes the need
for goto error paths, making the subsequent fix cleaner.

Patch 2 changes the return type of the helper function to int and
adds the necessary error checks in the callers.

Ethan Tidmore (2):
  iio: light: gp2ap020a00f: simplify locking with guard()
  iio: light: gp2ap020a00f: correct return type to int

 drivers/iio/light/gp2ap020a00f.c | 77 ++++++++++++--------------------
 1 file changed, 29 insertions(+), 48 deletions(-)

-- 
2.53.0

[PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Ethan Tidmore]

Use the guard() cleanup handler to manage the device lock.
This simplifies the code by removing the need for manual unlocking
and goto error handling paths.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
---
v3:
- Use guard() instead of manual locking and unlocking.

 drivers/iio/light/gp2ap020a00f.c | 64 ++++++++++----------------------
 1 file changed, 20 insertions(+), 44 deletions(-)

diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index c7df4b258e2c..d1a511ac7690 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -31,6 +31,7 @@
  * the other one.
  */
 
+#include <linux/cleanup.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
@@ -1024,17 +1025,14 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
 	bool event_en = false;
 	u8 thresh_val_id;
 	u8 thresh_reg_l;
-	int err = 0;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
 	thresh_val_id = GP2AP020A00F_THRESH_VAL_ID(thresh_reg_l);
 
-	if (thresh_val_id > GP2AP020A00F_THRESH_PH) {
-		err = -EINVAL;
-		goto error_unlock;
-	}
+	if (thresh_val_id > GP2AP020A00F_THRESH_PH)
+		return -EINVAL;
 
 	switch (thresh_reg_l) {
 	case GP2AP020A00F_TH_L_REG:
@@ -1046,30 +1044,23 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
 							&data->flags);
 		break;
 	case GP2AP020A00F_PH_L_REG:
-		if (val == 0) {
-			err = -EINVAL;
-			goto error_unlock;
-		}
+		if (val == 0)
+			return -EINVAL;
+
 		event_en = test_bit(GP2AP020A00F_FLAG_PROX_RISING_EV,
 							&data->flags);
 		break;
 	case GP2AP020A00F_PL_L_REG:
-		if (val == 0) {
-			err = -EINVAL;
-			goto error_unlock;
-		}
+		if (val == 0)
+			return -EINVAL;
+
 		event_en = test_bit(GP2AP020A00F_FLAG_PROX_FALLING_EV,
 							&data->flags);
 		break;
 	}
 
 	data->thresh_val[thresh_val_id] = val;
-	err =  gp2ap020a00f_write_event_threshold(data, thresh_val_id,
-							event_en);
-error_unlock:
-	mutex_unlock(&data->lock);
-
-	return err;
+	return gp2ap020a00f_write_event_threshold(data, thresh_val_id, event_en);
 }
 
 static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
@@ -1081,23 +1072,17 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
 {
 	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
 	u8 thresh_reg_l;
-	int err = IIO_VAL_INT;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
 
-	if (thresh_reg_l > GP2AP020A00F_PH_L_REG) {
-		err = -EINVAL;
-		goto error_unlock;
-	}
+	if (thresh_reg_l > GP2AP020A00F_PH_L_REG)
+		return -EINVAL;
 
 	*val = data->thresh_val[GP2AP020A00F_THRESH_VAL_ID(thresh_reg_l)];
 
-error_unlock:
-	mutex_unlock(&data->lock);
-
-	return err;
+	return IIO_VAL_INT;
 }
 
 static int gp2ap020a00f_write_prox_event_config(struct iio_dev *indio_dev,
@@ -1165,7 +1150,7 @@ static int gp2ap020a00f_write_event_config(struct iio_dev *indio_dev,
 	enum gp2ap020a00f_cmd cmd;
 	int err;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	switch (chan->type) {
 	case IIO_PROXIMITY:
@@ -1186,8 +1171,6 @@ static int gp2ap020a00f_write_event_config(struct iio_dev *indio_dev,
 		err = -EINVAL;
 	}
 
-	mutex_unlock(&data->lock);
-
 	return err;
 }
 
@@ -1199,7 +1182,7 @@ static int gp2ap020a00f_read_event_config(struct iio_dev *indio_dev,
 	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
 	int event_en = 0;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	switch (chan->type) {
 	case IIO_PROXIMITY:
@@ -1223,8 +1206,6 @@ static int gp2ap020a00f_read_event_config(struct iio_dev *indio_dev,
 		break;
 	}
 
-	mutex_unlock(&data->lock);
-
 	return event_en;
 }
 
@@ -1385,7 +1366,7 @@ static int gp2ap020a00f_buffer_postenable(struct iio_dev *indio_dev)
 	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
 	int i, err = 0;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	/*
 	 * Enable triggers according to the scan_mask. Enabling either
@@ -1413,15 +1394,12 @@ static int gp2ap020a00f_buffer_postenable(struct iio_dev *indio_dev)
 	}
 
 	if (err < 0)
-		goto error_unlock;
+		return err;
 
 	data->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
 	if (!data->buffer)
 		err = -ENOMEM;
 
-error_unlock:
-	mutex_unlock(&data->lock);
-
 	return err;
 }
 
@@ -1430,7 +1408,7 @@ static int gp2ap020a00f_buffer_predisable(struct iio_dev *indio_dev)
 	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
 	int i, err = 0;
 
-	mutex_lock(&data->lock);
+	guard(mutex)(&data->lock);
 
 	iio_for_each_active_channel(indio_dev, i) {
 		switch (i) {
@@ -1452,8 +1430,6 @@ static int gp2ap020a00f_buffer_predisable(struct iio_dev *indio_dev)
 	if (err == 0)
 		kfree(data->buffer);
 
-	mutex_unlock(&data->lock);
-
 	return err;
 }
 
-- 
2.53.0

[PATCH v3 2/2] iio: light: gp2ap020a00f: correct return type to int

[Ethan Tidmore]

The function gp2ap020a00f_get_thresh_reg() can return -EINVAL in its
error path. Yet, the function has return type of u8. Added error
checking for gp2ap020a00f_get_thresh_reg() return value.

Detected by Smatch:
drivers/iio/light/gp2ap020a00f.c:1013 gp2ap020a00f_get_thresh_reg() warn:
signedness bug returning '(-22)'

Signed-off-by: Ethan Tidmore <ethantidmore06@gmail.com>
---
v3:
- Remove Fixes: tag.
- Added Smatch warning.
v2:
- Fixed gp2ap020a00f_get_thresh_reg() parameter alignment.
- Removed unneeded whitespace between assignment and check.

 drivers/iio/light/gp2ap020a00f.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c
index d1a511ac7690..fae7605f2f5c 100644
--- a/drivers/iio/light/gp2ap020a00f.c
+++ b/drivers/iio/light/gp2ap020a00f.c
@@ -993,8 +993,8 @@ static irqreturn_t gp2ap020a00f_trigger_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static u8 gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
-					     enum iio_event_direction event_dir)
+static int gp2ap020a00f_get_thresh_reg(const struct iio_chan_spec *chan,
+				       enum iio_event_direction event_dir)
 {
 	switch (chan->type) {
 	case IIO_PROXIMITY:
@@ -1024,11 +1024,14 @@ static int gp2ap020a00f_write_event_val(struct iio_dev *indio_dev,
 	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
 	bool event_en = false;
 	u8 thresh_val_id;
-	u8 thresh_reg_l;
+	int thresh_reg_l;
 
 	guard(mutex)(&data->lock);
 
 	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
+	if (thresh_reg_l < 0)
+		return thresh_reg_l;
+
 	thresh_val_id = GP2AP020A00F_THRESH_VAL_ID(thresh_reg_l);
 
 	if (thresh_val_id > GP2AP020A00F_THRESH_PH)
@@ -1071,11 +1074,13 @@ static int gp2ap020a00f_read_event_val(struct iio_dev *indio_dev,
 				       int *val, int *val2)
 {
 	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
-	u8 thresh_reg_l;
+	int thresh_reg_l;
 
 	guard(mutex)(&data->lock);
 
 	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
+	if (thresh_reg_l < 0)
+		return thresh_reg_l;
 
 	if (thresh_reg_l > GP2AP020A00F_PH_L_REG)
 		return -EINVAL;
-- 
2.53.0

Re: [PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Andy Shevchenko]

On Sun, Feb 15, 2026 at 06:53:16PM -0600, Ethan Tidmore wrote:
> Use the guard() cleanup handler to manage the device lock.
> This simplifies the code by removing the need for manual unlocking
> and goto error handling paths.

...

>  	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);
>  	thresh_val_id = GP2AP020A00F_THRESH_VAL_ID(thresh_reg_l);

>  

Also drop this blank line, it will be aligned with the similar style in the
next patch.

> -	if (thresh_val_id > GP2AP020A00F_THRESH_PH) {
> -		err = -EINVAL;
> -		goto error_unlock;
> -	}
> +	if (thresh_val_id > GP2AP020A00F_THRESH_PH)
> +		return -EINVAL;

...

>  	thresh_reg_l = gp2ap020a00f_get_thresh_reg(chan, dir);

>  

Ditto.

> -	if (thresh_reg_l > GP2AP020A00F_PH_L_REG) {
> -		err = -EINVAL;
> -		goto error_unlock;
> -	}
> +	if (thresh_reg_l > GP2AP020A00F_PH_L_REG)
> +		return -EINVAL;

...

>  	data->buffer = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>  	if (!data->buffer)

>  		err = -ENOMEM;

Now simply

		return -ENOMEM;

> -error_unlock:
> -	mutex_unlock(&data->lock);
> -
>  	return err;

And with the above I believe this becomes

	return 0;

>  }

...

>  	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
>  	int i, err = 0;

Would you need the err assignment now?

> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	iio_for_each_active_channel(indio_dev, i) {
>  		switch (i) {

...

>  	if (err == 0)
>  		kfree(data->buffer);

With the guard()() in place this can be inverted to have something
(I haven't seen the full context, though) like

	if (err)
		return err;

	kfree(...);

> -	mutex_unlock(&data->lock);
> -
>  	return err;

Ideally this should become

	return 0;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] iio: light: gp2ap020a00f: correct return type to int

[Andy Shevchenko]

On Sun, Feb 15, 2026 at 06:53:17PM -0600, Ethan Tidmore wrote:
> The function gp2ap020a00f_get_thresh_reg() can return -EINVAL in its
> error path. Yet, the function has return type of u8. Added error
> checking for gp2ap020a00f_get_thresh_reg() return value.
> 
> Detected by Smatch:
> drivers/iio/light/gp2ap020a00f.c:1013 gp2ap020a00f_get_thresh_reg() warn:
> signedness bug returning '(-22)'

LGTM now,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Ethan Tidmore]

On Mon Feb 16, 2026 at 1:08 AM CST, Andy Shevchenko wrote:
> On Sun, Feb 15, 2026 at 06:53:16PM -0600, Ethan Tidmore wrote:
>
>>  	struct gp2ap020a00f_data *data = iio_priv(indio_dev);
>>  	int i, err = 0;
>
> Would you need the err assignment now?

I'm not familiar with iio_for_each_active_channel() but, if there's a
chance it could run zero times, then it'd run into if (err) with err
not being initialized?

Thanks,

ET

Re: [PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Andy Shevchenko]

On Mon, Feb 16, 2026 at 03:04:55PM -0600, Ethan Tidmore wrote:
> On Mon Feb 16, 2026 at 1:08 AM CST, Andy Shevchenko wrote:
> > On Sun, Feb 15, 2026 at 06:53:16PM -0600, Ethan Tidmore wrote:

...

> >>  	int i, err = 0;
> >
> > Would you need the err assignment now?
> 
> I'm not familiar with iio_for_each_active_channel() but, if there's a
> chance it could run zero times, then it'd run into if (err) with err
> not being initialized?

Have you read my other comments? I also mentioned to replace

		err = -ENOMEM;

with

		return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Ethan Tidmore]

On Tue Feb 17, 2026 at 2:37 AM CST, Andy Shevchenko wrote:
> On Mon, Feb 16, 2026 at 03:04:55PM -0600, Ethan Tidmore wrote:
>> On Mon Feb 16, 2026 at 1:08 AM CST, Andy Shevchenko wrote:
>> > On Sun, Feb 15, 2026 at 06:53:16PM -0600, Ethan Tidmore wrote:
>
> ...
>
>> >>  	int i, err = 0;
>> >
>> > Would you need the err assignment now?
>> 
>> I'm not familiar with iio_for_each_active_channel() but, if there's a
>> chance it could run zero times, then it'd run into if (err) with err
>> not being initialized?
>
> Have you read my other comments? I also mentioned to replace
>
> 		err = -ENOMEM;
>
> with
>
> 		return -ENOMEM;

Sorry about that, when you asked about the assignment, I thought you meant
the initialization (err = 0) vs just declaring it (int err), rather than 
realizing the variable itself is redundant. Will send v4 shortly.

Thanks,

ET

Re: [PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Andy Shevchenko]

On Tue, Feb 17, 2026 at 10:37:18AM +0200, Andy Shevchenko wrote:
> On Mon, Feb 16, 2026 at 03:04:55PM -0600, Ethan Tidmore wrote:
> > On Mon Feb 16, 2026 at 1:08 AM CST, Andy Shevchenko wrote:
> > > On Sun, Feb 15, 2026 at 06:53:16PM -0600, Ethan Tidmore wrote:

...

> > >>  	int i, err = 0;
> > >
> > > Would you need the err assignment now?
> > 
> > I'm not familiar with iio_for_each_active_channel() but, if there's a
> > chance it could run zero times, then it'd run into if (err) with err
> > not being initialized?
> 
> Have you read my other comments? I also mentioned to replace
> 
> 		err = -ENOMEM;
> 
> with
> 
> 		return -ENOMEM;

Looking into the driver code, it has some serious questions about the logic
behind. I will send soon a WIP series for you to take into account. Feel
free to integrate it into your next series.

...

Do you have HW to test?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Ethan Tidmore]

On Tue Feb 17, 2026 at 3:03 AM CST, Andy Shevchenko wrote:
> On Tue, Feb 17, 2026 at 10:37:18AM +0200, Andy Shevchenko wrote:
>> On Mon, Feb 16, 2026 at 03:04:55PM -0600, Ethan Tidmore wrote:
>> > On Mon Feb 16, 2026 at 1:08 AM CST, Andy Shevchenko wrote:
>> > > On Sun, Feb 15, 2026 at 06:53:16PM -0600, Ethan Tidmore wrote:
>
> ...
>
>> > >>  	int i, err = 0;
>> > >
>> > > Would you need the err assignment now?
>> > 
>> > I'm not familiar with iio_for_each_active_channel() but, if there's a
>> > chance it could run zero times, then it'd run into if (err) with err
>> > not being initialized?
>> 
>> Have you read my other comments? I also mentioned to replace
>> 
>> 		err = -ENOMEM;
>> 
>> with
>> 
>> 		return -ENOMEM;
>
> Looking into the driver code, it has some serious questions about the logic
> behind. I will send soon a WIP series for you to take into account. Feel
> free to integrate it into your next series.
>
> ...
>
> Do you have HW to test?

Ignore my previous statement about sending v4 shortly now. No, I don't
have the HW, but I will be happy to integrate your series into this
cleanup.

Thanks,

ET

Re: [PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Andy Shevchenko]

On Tue, Feb 17, 2026 at 03:02:48AM -0600, Ethan Tidmore wrote:
> On Tue Feb 17, 2026 at 2:37 AM CST, Andy Shevchenko wrote:
> > On Mon, Feb 16, 2026 at 03:04:55PM -0600, Ethan Tidmore wrote:
> >> On Mon Feb 16, 2026 at 1:08 AM CST, Andy Shevchenko wrote:
> >> > On Sun, Feb 15, 2026 at 06:53:16PM -0600, Ethan Tidmore wrote:

...

> >> >>  	int i, err = 0;
> >> >
> >> > Would you need the err assignment now?
> >> 
> >> I'm not familiar with iio_for_each_active_channel() but, if there's a
> >> chance it could run zero times, then it'd run into if (err) with err
> >> not being initialized?
> >
> > Have you read my other comments? I also mentioned to replace
> >
> > 		err = -ENOMEM;
> >
> > with
> >
> > 		return -ENOMEM;
> 
> Sorry about that, when you asked about the assignment, I thought you meant
> the initialization (err = 0) vs just declaring it (int err), rather than 
> realizing the variable itself is redundant. Will send v4 shortly.

Please, wait a bit. I will send what I have, you will see better what can be done.
But again, do you have an HW to test?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] iio: light: gp2ap020a00f: simplify locking with guard()

[Andy Shevchenko]

On Tue, Feb 17, 2026 at 03:10:37AM -0600, Ethan Tidmore wrote:
> On Tue Feb 17, 2026 at 3:03 AM CST, Andy Shevchenko wrote:
> > On Tue, Feb 17, 2026 at 10:37:18AM +0200, Andy Shevchenko wrote:

...

> Ignore my previous statement about sending v4 shortly now. No, I don't
> have the HW, but I will be happy to integrate your series into this
> cleanup.

Just have sent it.

-- 
With Best Regards,
Andy Shevchenko