Re: [PATCH v2 08/10] iio: light: opt3001: move driver to guard(mutex)() use

[Joshua Crofts]

On Sat, 16 May 2026 at 14:05, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 12 May 2026 12:57:28 +0200
> Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@xxxxxxxxxx> wrote:
>
> > From: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> >
> > Move driver to use guard(mutex)() macro, to facilitate automatic
> > locking/unlocking of resources. This modernizes the driver and
> > improves code style.
> >
> > While at it, remove unnecessary gotos and return variables.
> >
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> > Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> A couple of very trivial things where I'd have done it differently.
> I don't mind much however if you want to keep it as is.
>
> > @@ -615,18 +601,12 @@ static int opt3001_write_event_value(struct iio_dev *iio,
> >               opt->low_thresh_exp = exponent;
> >               break;
> >       default:
> > -             ret = -EINVAL;
> > -             goto err;
> > +             return -EINVAL;
> >       }
> >
> >       ret = i2c_smbus_write_word_swapped(client, reg, value);
> > -     if (ret < 0) {
> > +     if (ret < 0)
> >               dev_err(dev, "failed to write register %02x\n", reg);
> > -             goto err;
> Similar to below.  Though very much a matter of personal taste - I'd
> definitely not have asked for a respin for this alone!
> > -     }
> > -
> > -err:
> > -     mutex_unlock(&opt->lock);
> >
> >       return ret;
> >  }
> > @@ -660,7 +640,7 @@ static int opt3001_write_event_config(struct iio_dev *iio,
> >       if (!state && opt->mode == OPT3001_CONFIGURATION_M_SHUTDOWN)
> >               return 0;
> >
> > -     mutex_lock(&opt->lock);
> > +     guard(mutex)(&opt->lock);
> >
> >       mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
> >               : OPT3001_CONFIGURATION_M_SHUTDOWN;
> > @@ -669,21 +649,16 @@ static int opt3001_write_event_config(struct iio_dev *iio,
> >       if (ret < 0) {
> >               dev_err(dev, "failed to read register %02x\n",
> >                       OPT3001_CONFIGURATION);
> > -             goto err;
> > +             return ret;
> >       }
> >
> >       reg = ret;
> >       opt3001_set_mode(opt, &reg, mode);
> >
> >       ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
> > -     if (ret < 0) {
> > +     if (ret < 0)
> >               dev_err(dev, "failed to write register %02x\n",
> >                       OPT3001_CONFIGURATION);
> > -             goto err;
>
> I'd return ret here
>
> > -     }
> > -
>
> Then return 0; here
>
> Makes it a little easier to spot the good path.

Ah, apologies, I've totally missed these paths, normally I would
delete these and add a return.

-- 
Kind regards

CJD