* Re: [PATCH v2] iio: light: iqs621-als: use lock guards
2026-04-26 16:17 ` [PATCH v2] iio: light: iqs621-als: use lock guards Pedro Barletta Gennari
@ 2026-04-27 9:51 ` Jonathan Cameron
0 siblings, 0 replies; 2+ 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] 2+ messages in thread