* [PATCH v2] iio: light: iqs621-als: use lock guards
@ 2026-04-21 4:00 Pedro Barletta Gennari
2026-04-21 9:58 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Pedro Barletta Gennari @ 2026-04-21 4:00 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy; +Cc: pedro.pbg, linux-iio
Use guard(mutex)() for handling mutex lock instead of
manually locking and unlocking the mutex. This prevents forgotten
locks due to early exits and remove the need of gotos.
Signed-off-by: Pedro Barletta Gennari <pedro.pbg@usp.br>
---
v2:
- Keep include list ordered
- Remove redundant 'else'
- Remove unnecessary variable 'ret'
---
drivers/iio/light/iqs621-als.c | 85 +++++++++++-----------------------
1 file changed, 28 insertions(+), 57 deletions(-)
diff --git a/drivers/iio/light/iqs621-als.c b/drivers/iio/light/iqs621-als.c
index b9f230210f07..8d08f28c642d 100644
--- a/drivers/iio/light/iqs621-als.c
+++ b/drivers/iio/light/iqs621-als.c
@@ -5,6 +5,7 @@
* Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com>
*/
+#include <linux/cleanup.h>
#include <linux/device.h>
#include <linux/iio/events.h>
#include <linux/iio/iio.h>
@@ -107,25 +108,21 @@ static int iqs621_als_notifier(struct notifier_block *notifier,
indio_dev = iqs621_als->indio_dev;
timestamp = iio_get_time_ns(indio_dev);
- mutex_lock(&iqs621_als->lock);
+ guard(mutex)(&iqs621_als->lock);
if (event_flags & BIT(IQS62X_EVENT_SYS_RESET)) {
ret = iqs621_als_init(iqs621_als);
if (ret) {
dev_err(indio_dev->dev.parent,
"Failed to re-initialize device: %d\n", ret);
- ret = NOTIFY_BAD;
- } else {
- ret = NOTIFY_OK;
+ return NOTIFY_BAD;
}
-
- goto err_mutex;
+ return NOTIFY_OK;
}
if (!iqs621_als->light_en && !iqs621_als->range_en &&
!iqs621_als->prox_en) {
- ret = NOTIFY_DONE;
- goto err_mutex;
+ return NOTIFY_DONE;
}
/* IQS621 only */
@@ -181,12 +178,7 @@ static int iqs621_als_notifier(struct notifier_block *notifier,
iqs621_als->als_flags = event_data->als_flags;
iqs621_als->ir_flags = event_data->ir_flags;
- ret = NOTIFY_OK;
-
-err_mutex:
- mutex_unlock(&iqs621_als->lock);
-
- return ret;
+ return NOTIFY_OK;
}
static void iqs621_als_notifier_unregister(void *context)
@@ -241,30 +233,22 @@ static int iqs621_als_read_event_config(struct iio_dev *indio_dev,
enum iio_event_direction dir)
{
struct iqs621_als_private *iqs621_als = iio_priv(indio_dev);
- int ret;
- mutex_lock(&iqs621_als->lock);
+ guard(mutex)(&iqs621_als->lock);
switch (chan->type) {
case IIO_LIGHT:
- ret = iqs621_als->light_en;
- break;
+ return iqs621_als->light_en;
case IIO_INTENSITY:
- ret = iqs621_als->range_en;
- break;
+ return iqs621_als->range_en;
case IIO_PROXIMITY:
- ret = iqs621_als->prox_en;
- break;
+ return iqs621_als->prox_en;
default:
- ret = -EINVAL;
+ return -EINVAL;
}
-
- mutex_unlock(&iqs621_als->lock);
-
- return ret;
}
static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
@@ -278,11 +262,11 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
unsigned int val;
int ret;
- mutex_lock(&iqs621_als->lock);
+ guard(mutex)(&iqs621_als->lock);
ret = regmap_read(iqs62x->regmap, iqs62x->dev_desc->als_flags, &val);
if (ret)
- goto err_mutex;
+ return ret;
iqs621_als->als_flags = val;
switch (chan->type) {
@@ -293,7 +277,7 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
0xFF);
if (!ret)
iqs621_als->light_en = state;
- break;
+ return ret;
case IIO_INTENSITY:
ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
@@ -302,12 +286,12 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
0xFF);
if (!ret)
iqs621_als->range_en = state;
- break;
+ return ret;
case IIO_PROXIMITY:
ret = regmap_read(iqs62x->regmap, IQS622_IR_FLAGS, &val);
if (ret)
- goto err_mutex;
+ return ret;
iqs621_als->ir_flags = val;
ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
@@ -315,16 +299,11 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
state ? 0 : 0xFF);
if (!ret)
iqs621_als->prox_en = state;
- break;
+ return ret;
default:
- ret = -EINVAL;
+ return -EINVAL;
}
-
-err_mutex:
- mutex_unlock(&iqs621_als->lock);
-
- return ret;
}
static int iqs621_als_read_event_value(struct iio_dev *indio_dev,
@@ -335,33 +314,28 @@ static int iqs621_als_read_event_value(struct iio_dev *indio_dev,
int *val, int *val2)
{
struct iqs621_als_private *iqs621_als = iio_priv(indio_dev);
- int ret = IIO_VAL_INT;
- mutex_lock(&iqs621_als->lock);
+ guard(mutex)(&iqs621_als->lock);
switch (dir) {
case IIO_EV_DIR_RISING:
*val = iqs621_als->thresh_light * 16;
- break;
+ return IIO_VAL_INT;
case IIO_EV_DIR_FALLING:
*val = iqs621_als->thresh_dark * 4;
- break;
+ return IIO_VAL_INT;
case IIO_EV_DIR_EITHER:
if (iqs621_als->ir_flags_mask == IQS622_IR_FLAGS_TOUCH)
*val = iqs621_als->thresh_prox * 4;
else
*val = iqs621_als->thresh_prox;
- break;
+ return IIO_VAL_INT;
default:
- ret = -EINVAL;
+ return -EINVAL;
}
-
- mutex_unlock(&iqs621_als->lock);
-
- return ret;
}
static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
@@ -377,7 +351,7 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
u8 ir_flags_mask, *thresh_cache;
int ret = -EINVAL;
- mutex_lock(&iqs621_als->lock);
+ guard(mutex)(&iqs621_als->lock);
switch (dir) {
case IIO_EV_DIR_RISING:
@@ -426,29 +400,26 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
break;
default:
- goto err_mutex;
+ return ret;
}
thresh_cache = &iqs621_als->thresh_prox;
break;
default:
- goto err_mutex;
+ return ret;
}
if (thresh_val > 0xFF)
- goto err_mutex;
+ return ret;
ret = regmap_write(iqs62x->regmap, thresh_reg, thresh_val);
if (ret)
- goto err_mutex;
+ return ret;
*thresh_cache = thresh_val;
iqs621_als->ir_flags_mask = ir_flags_mask;
-err_mutex:
- mutex_unlock(&iqs621_als->lock);
-
return ret;
}
--
2.51.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iio: light: iqs621-als: use lock guards
2026-04-21 4:00 [PATCH v2] iio: light: iqs621-als: use lock guards Pedro Barletta Gennari
@ 2026-04-21 9:58 ` Jonathan Cameron
2026-04-26 16:17 ` Pedro Barletta Gennari
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2026-04-21 9:58 UTC (permalink / raw)
To: Pedro Barletta Gennari; +Cc: dlechner, nuno.sa, andy, linux-iio
On Tue, 21 Apr 2026 01:00:11 -0300
Pedro Barletta Gennari <pedro.pbg@usp.br> wrote:
> Use guard(mutex)() for handling mutex lock instead of
> manually locking and unlocking the mutex. This prevents forgotten
> locks due to early exits and remove the need of gotos.
>
> Signed-off-by: Pedro Barletta Gennari <pedro.pbg@usp.br>
> ---
> v2:
> - Keep include list ordered
> - Remove redundant 'else'
> - Remove unnecessary variable 'ret'
Hi Pedro,
The changes here enable a few additional code improvements that
I'd like to see made in the same patch.
See below,
Thanks,
Jonathan
> ---
> drivers/iio/light/iqs621-als.c | 85 +++++++++++-----------------------
> 1 file changed, 28 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/iio/light/iqs621-als.c b/drivers/iio/light/iqs621-als.c
> index b9f230210f07..8d08f28c642d 100644
> --- a/drivers/iio/light/iqs621-als.c
> +++ b/drivers/iio/light/iqs621-als.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com>
> */
>
> +#include <linux/cleanup.h>
> #include <linux/device.h>
> #include <linux/iio/events.h>
> #include <linux/iio/iio.h>
> @@ -107,25 +108,21 @@ static int iqs621_als_notifier(struct notifier_block *notifier,
> indio_dev = iqs621_als->indio_dev;
> timestamp = iio_get_time_ns(indio_dev);
>
> - mutex_lock(&iqs621_als->lock);
> + guard(mutex)(&iqs621_als->lock);
>
> if (event_flags & BIT(IQS62X_EVENT_SYS_RESET)) {
> ret = iqs621_als_init(iqs621_als);
> if (ret) {
> dev_err(indio_dev->dev.parent,
> "Failed to re-initialize device: %d\n", ret);
> - ret = NOTIFY_BAD;
> - } else {
> - ret = NOTIFY_OK;
> + return NOTIFY_BAD;
> }
> -
> - goto err_mutex;
> + return NOTIFY_OK;
> }
>
> if (!iqs621_als->light_en && !iqs621_als->range_en &&
> !iqs621_als->prox_en) {
if (!iqs621_als->light_en && !iqs621_als->range_en && !iqs621_als->prox_en)
After change noted below, I'd just make this a slightly long single line.
We have gotten more relaxed on going a little over 80 chars since this
code was written.
> - ret = NOTIFY_DONE;
> - goto err_mutex;
> + return NOTIFY_DONE;
> }
Single statement so { } not needed
>
>
> static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> @@ -278,11 +262,11 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> unsigned int val;
> int ret;
>
> - mutex_lock(&iqs621_als->lock);
> + guard(mutex)(&iqs621_als->lock);
>
> ret = regmap_read(iqs62x->regmap, iqs62x->dev_desc->als_flags, &val);
> if (ret)
> - goto err_mutex;
> + return ret;
> iqs621_als->als_flags = val;
>
> switch (chan->type) {
> @@ -293,7 +277,7 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> 0xFF);
> if (!ret)
> iqs621_als->light_en = state;
> - break;
> + return ret;
Same as two cases below
>
> case IIO_INTENSITY:
> ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
> @@ -302,12 +286,12 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> 0xFF);
> if (!ret)
> iqs621_als->range_en = state;
> - break;
> + return ret;
As below, flip this to
if (ret)
return ret;
iqs621_als->range_en = state;
return 0;
>
> case IIO_PROXIMITY:
> ret = regmap_read(iqs62x->regmap, IQS622_IR_FLAGS, &val);
> if (ret)
> - goto err_mutex;
> + return ret;
> iqs621_als->ir_flags = val;
>
> ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
> @@ -315,16 +299,11 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> state ? 0 : 0xFF);
> if (!ret)
> iqs621_als->prox_en = state;
Please flip this to the more common form. Makes reading the code a little easier
if errors are handled out of line as early as possible.
if (ret)
return ret;
iqs621_als->prox_en = state;
return 0;
> - break;
> + return ret;
>
> default:
> - ret = -EINVAL;
> + return -EINVAL;
> }
> -
> -err_mutex:
> - mutex_unlock(&iqs621_als->lock);
> -
> - return ret;
> }
> static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> @@ -377,7 +351,7 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> u8 ir_flags_mask, *thresh_cache;
> int ret = -EINVAL;
Drop this initialization and instead return the specific value on error paths.
>
> - mutex_lock(&iqs621_als->lock);
> + guard(mutex)(&iqs621_als->lock);
>
> switch (dir) {
> case IIO_EV_DIR_RISING:
> @@ -426,29 +400,26 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> break;
>
> default:
> - goto err_mutex;
> + return ret;
> }
>
> thresh_cache = &iqs621_als->thresh_prox;
> break;
>
> default:
> - goto err_mutex;
> + return ret;
return -EINVAL;
> }
>
> if (thresh_val > 0xFF)
> - goto err_mutex;
> + return ret;
return -EINVAL;
>
> ret = regmap_write(iqs62x->regmap, thresh_reg, thresh_val);
> if (ret)
> - goto err_mutex;
> + return ret;
>
> *thresh_cache = thresh_val;
> iqs621_als->ir_flags_mask = ir_flags_mask;
>
> -err_mutex:
> - mutex_unlock(&iqs621_als->lock);
> -
> return ret;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iio: light: iqs621-als: use lock guards
2026-04-21 9:58 ` Jonathan Cameron
@ 2026-04-26 16:17 ` Pedro Barletta Gennari
2026-04-27 9:51 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Pedro Barletta Gennari @ 2026-04-26 16:17 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: dlechner, nuno.sa, andy, linux-iio
On Tue, Apr 21, 2026 at 6:58 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 21 Apr 2026 01:00:11 -0300
> Pedro Barletta Gennari <pedro.pbg@usp.br> wrote:
>
> > Use guard(mutex)() for handling mutex lock instead of
> > manually locking and unlocking the mutex. This prevents forgotten
> > locks due to early exits and remove the need of gotos.
> >
> > Signed-off-by: Pedro Barletta Gennari <pedro.pbg@usp.br>
> > ---
> > v2:
> > - Keep include list ordered
> > - Remove redundant 'else'
> > - Remove unnecessary variable 'ret'
> Hi Pedro,
>
> The changes here enable a few additional code improvements that
> I'd like to see made in the same patch.
>
> See below,
>
> Thanks,
>
> Jonathan
>
Hi Jonathan,
Just to be sure, do you want everything to be in the same commit?
> > @@ -107,25 +108,21 @@ static int iqs621_als_notifier(struct notifier_block *notifier,
> > indio_dev = iqs621_als->indio_dev;
> > timestamp = iio_get_time_ns(indio_dev);
> >
> > - mutex_lock(&iqs621_als->lock);
> > + guard(mutex)(&iqs621_als->lock);
> >
> > if (event_flags & BIT(IQS62X_EVENT_SYS_RESET)) {
> > ret = iqs621_als_init(iqs621_als);
> > if (ret) {
> > dev_err(indio_dev->dev.parent,
> > "Failed to re-initialize device: %d\n", ret);
> > - ret = NOTIFY_BAD;
> > - } else {
> > - ret = NOTIFY_OK;
> > + return NOTIFY_BAD;
> > }
> > -
> > - goto err_mutex;
> > + return NOTIFY_OK;
> > }
> >
> > if (!iqs621_als->light_en && !iqs621_als->range_en &&
> > !iqs621_als->prox_en) {
> if (!iqs621_als->light_en && !iqs621_als->range_en && !iqs621_als->prox_en)
>
> After change noted below, I'd just make this a slightly long single line.
> We have gotten more relaxed on going a little over 80 chars since this
> code was written.
>
> > - ret = NOTIFY_DONE;
> > - goto err_mutex;
> > + return NOTIFY_DONE;
> > }
>
> Single statement so { } not needed
>
Ok!
> > @@ -293,7 +277,7 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> > 0xFF);
> > if (!ret)
> > iqs621_als->light_en = state;
> > - break;
> > + return ret;
> Same as two cases below
> >
> > case IIO_INTENSITY:
> > ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
> > @@ -302,12 +286,12 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> > 0xFF);
> > if (!ret)
> > iqs621_als->range_en = state;
> > - break;
> > + return ret;
> As below, flip this to
> if (ret)
> return ret;
>
> iqs621_als->range_en = state;
>
> return 0;
>
> >
> > case IIO_PROXIMITY:
> > ret = regmap_read(iqs62x->regmap, IQS622_IR_FLAGS, &val);
> > if (ret)
> > - goto err_mutex;
> > + return ret;
> > iqs621_als->ir_flags = val;
> >
> > ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
> > @@ -315,16 +299,11 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> > state ? 0 : 0xFF);
> > if (!ret)
> > iqs621_als->prox_en = state;
>
> Please flip this to the more common form. Makes reading the code a little easier
> if errors are handled out of line as early as possible.
>
> if (ret)
> return ret;
>
> iqs621_als->prox_en = state;
>
> return 0;
>
Ok!
> > static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> > @@ -377,7 +351,7 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> > u8 ir_flags_mask, *thresh_cache;
> > int ret = -EINVAL;
>
> Drop this initialization and instead return the specific value on error paths.
>
Ok!
> > @@ -426,29 +400,26 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> > break;
> >
> > default:
> > - goto err_mutex;
> > + return ret;
> > }
> >
> > thresh_cache = &iqs621_als->thresh_prox;
> > break;
> >
> > default:
> > - goto err_mutex;
> > + return ret;
>
> return -EINVAL;
>
> > }
> >
> > if (thresh_val > 0xFF)
> > - goto err_mutex;
> > + return ret;
>
> return -EINVAL;
>
> >
> > ret = regmap_write(iqs62x->regmap, thresh_reg, thresh_val);
> > if (ret)
> > - goto err_mutex;
> > + return ret;
> >
> > *thresh_cache = thresh_val;
> > iqs621_als->ir_flags_mask = ir_flags_mask;
> >
> > -err_mutex:
> > - mutex_unlock(&iqs621_als->lock);
> > -
> > return ret;
> > }
> >
>
In this case, should this last return be "return 0;"?
--
Thanks,
Pedro
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] iio: light: iqs621-als: use lock guards
2026-04-26 16:17 ` Pedro Barletta Gennari
@ 2026-04-27 9:51 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2026-04-27 9:51 UTC (permalink / raw)
To: Pedro Barletta Gennari; +Cc: dlechner, nuno.sa, andy, linux-iio
On Sun, 26 Apr 2026 13:17:01 -0300
Pedro Barletta Gennari <pedro.pbg@usp.br> wrote:
> On Tue, Apr 21, 2026 at 6:58 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 21 Apr 2026 01:00:11 -0300
> > Pedro Barletta Gennari <pedro.pbg@usp.br> wrote:
> >
> > > Use guard(mutex)() for handling mutex lock instead of
> > > manually locking and unlocking the mutex. This prevents forgotten
> > > locks due to early exits and remove the need of gotos.
> > >
> > > Signed-off-by: Pedro Barletta Gennari <pedro.pbg@usp.br>
> > > ---
> > > v2:
> > > - Keep include list ordered
> > > - Remove redundant 'else'
> > > - Remove unnecessary variable 'ret'
> > Hi Pedro,
> >
> > The changes here enable a few additional code improvements that
> > I'd like to see made in the same patch.
> >
> > See below,
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Hi Jonathan,
>
> Just to be sure, do you want everything to be in the same commit?
The places where I'm asking you to flip if (!ret) to if (ret)
logic should be a second patch. Do them in which ever order
makes for cleanest series.
I've commented more on individual changes below.
>
> > > @@ -107,25 +108,21 @@ static int iqs621_als_notifier(struct notifier_block *notifier,
> > > indio_dev = iqs621_als->indio_dev;
> > > timestamp = iio_get_time_ns(indio_dev);
> > >
> > > - mutex_lock(&iqs621_als->lock);
> > > + guard(mutex)(&iqs621_als->lock);
> > >
> > > if (event_flags & BIT(IQS62X_EVENT_SYS_RESET)) {
> > > ret = iqs621_als_init(iqs621_als);
> > > if (ret) {
> > > dev_err(indio_dev->dev.parent,
> > > "Failed to re-initialize device: %d\n", ret);
> > > - ret = NOTIFY_BAD;
> > > - } else {
> > > - ret = NOTIFY_OK;
> > > + return NOTIFY_BAD;
> > > }
> > > -
> > > - goto err_mutex;
> > > + return NOTIFY_OK;
> > > }
> > >
> > > if (!iqs621_als->light_en && !iqs621_als->range_en &&
> > > !iqs621_als->prox_en) {
> > if (!iqs621_als->light_en && !iqs621_als->range_en && !iqs621_als->prox_en)
> >
> > After change noted below, I'd just make this a slightly long single line.
> > We have gotten more relaxed on going a little over 80 chars since this
> > code was written.
This one is fine rolled into the guard() patch as you'll be changing it anyway
to drop the {
> >
> > > - ret = NOTIFY_DONE;
> > > - goto err_mutex;
> > > + return NOTIFY_DONE;
> > > }
> >
> > Single statement so { } not needed
> >
>
> Ok!
>
> > > @@ -293,7 +277,7 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> > > 0xFF);
> > > if (!ret)
> > > iqs621_als->light_en = state;
> > > - break;
> > > + return ret;
> > Same as two cases below
> > >
> > > case IIO_INTENSITY:
> > > ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
> > > @@ -302,12 +286,12 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> > > 0xFF);
> > > if (!ret)
> > > iqs621_als->range_en = state;
> > > - break;
> > > + return ret;
> > As below, flip this to
> > if (ret)
> > return ret;
> >
> > iqs621_als->range_en = state;
This ideally in a different patch.
> >
> > return 0;
> >
> > >
> > > case IIO_PROXIMITY:
> > > ret = regmap_read(iqs62x->regmap, IQS622_IR_FLAGS, &val);
> > > if (ret)
> > > - goto err_mutex;
> > > + return ret;
> > > iqs621_als->ir_flags = val;
> > >
> > > ret = regmap_update_bits(iqs62x->regmap, IQS620_GLBL_EVENT_MASK,
> > > @@ -315,16 +299,11 @@ static int iqs621_als_write_event_config(struct iio_dev *indio_dev,
> > > state ? 0 : 0xFF);
> > > if (!ret)
> > > iqs621_als->prox_en = state;
> >
> > Please flip this to the more common form. Makes reading the code a little easier
> > if errors are handled out of line as early as possible.
> >
> > if (ret)
> > return ret;
> >
> > iqs621_als->prox_en = state;
> >
> > return 0;
> >
>
> Ok!
>
> > > static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> > > @@ -377,7 +351,7 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> > > u8 ir_flags_mask, *thresh_cache;
> > > int ret = -EINVAL;
> >
> > Drop this initialization and instead return the specific value on error paths.
> >
>
> Ok!
>
> > > @@ -426,29 +400,26 @@ static int iqs621_als_write_event_value(struct iio_dev *indio_dev,
> > > break;
> > >
> > > default:
> > > - goto err_mutex;
> > > + return ret;
> > > }
> > >
> > > thresh_cache = &iqs621_als->thresh_prox;
> > > break;
> > >
> > > default:
> > > - goto err_mutex;
> > > + return ret;
> >
> > return -EINVAL;
> >
> > > }
> > >
> > > if (thresh_val > 0xFF)
> > > - goto err_mutex;
> > > + return ret;
> >
> > return -EINVAL;
> >
> > >
> > > ret = regmap_write(iqs62x->regmap, thresh_reg, thresh_val);
> > > if (ret)
> > > - goto err_mutex;
> > > + return ret;
> > >
> > > *thresh_cache = thresh_val;
> > > iqs621_als->ir_flags_mask = ir_flags_mask;
> > >
> > > -err_mutex:
> > > - mutex_unlock(&iqs621_als->lock);
> > > -
> > > return ret;
> > > }
> > >
> >
>
> In this case, should this last return be "return 0;"?
Yes.
>
> --
> Thanks,
>
> Pedro
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-04-27 9:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-21 4:00 [PATCH v2] iio: light: iqs621-als: use lock guards Pedro Barletta Gennari
2026-04-21 9:58 ` Jonathan Cameron
2026-04-26 16:17 ` Pedro Barletta Gennari
2026-04-27 9:51 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox