Re: [PATCH v2] iio: light: iqs621-als: use lock guards

[Jonathan Cameron <jic23@kernel.org>]

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