Re: [PATCH 7/7] iio: light: opt3001: switch driver to managed resources
From: Joshua Crofts
Date: Mon May 11 2026 - 09:43:03 EST
On Mon, 11 May 2026 at 15:32, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Mon, 11 May 2026 12:04:12 +0200
> Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@xxxxxxxxxx> wrote:
>
> > From: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> >
> > Move the driver to use devm_* functions to automate resource
> > management and simplify error handling. This also allows removal
> > of the opt3001_remove() function.
> >
> > Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> Hi Joshua
>
> A few things inline.
>
> Jonathan
>
> > ---
> > drivers/iio/light/opt3001.c | 67 +++++++++++++++++++++++----------------------
> > 1 file changed, 34 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> > index 155794bb28f68a945e20b083e382561ca6ad462e..3ea18d8993da627ae226ac414e8035d8c968861b 100644
> > --- a/drivers/iio/light/opt3001.c
> > +++ b/drivers/iio/light/opt3001.c
> > @@ -804,6 +804,29 @@ static irqreturn_t opt3001_irq(int irq, void *_iio)
> > return IRQ_HANDLED;
> > }
> >
> > +static void opt3001_power_off(void *data)
> > +{
> > + struct opt3001 *opt = data;
> > + int ret;
> > + u16 reg;
> > +
> > + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > + if (ret < 0) {
> > + dev_err(opt->dev, "failed to read register %02x\n",
> > + OPT3001_CONFIGURATION);
> > + return;
> > + }
> > +
> > + reg = ret;
> > + opt3001_set_mode(opt, ®, OPT3001_CONFIGURATION_M_SHUTDOWN);
> > +
> > + ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION,
> > + reg);
> > + if (ret < 0)
> > + dev_err(opt->dev, "failed to write register %02x\n",
> > + OPT3001_CONFIGURATION);
> > +}
> > +
> > static int opt3001_probe(struct i2c_client *client)
> > {
> > struct device *dev = &client->dev;
> > @@ -822,7 +845,10 @@ static int opt3001_probe(struct i2c_client *client)
> > opt->dev = dev;
> > opt->chip_info = i2c_get_match_data(client);
> >
> > - mutex_init(&opt->lock);
> > + ret = devm_mutex_init(dev, &opt->lock);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to initialize mutex\n");
>
> Don't print on this one. I'm fairly sure it can only return -ENOMEM as
> part of devm registration failing and for those we don't print errors.
> dev_err_probe() won't print anything anyway so it's just noise having
> it here.
Agreed.
>
> > +
> > init_waitqueue_head(&opt->result_ready_queue);
> > i2c_set_clientdata(client, iio);
> >
> > @@ -836,6 +862,10 @@ static int opt3001_probe(struct i2c_client *client)
> > if (ret)
> > return ret;
> >
> > + ret = devm_add_action_or_reset(dev, opt3001_power_off, opt);
> > + if (ret)
> > + return ret;
>
> This is undoing only part of what happens in the previous call (I think)
> and if we get an error in there (opt3001_configure()) we don't turn the
> power off again. I'd move this registration into that function - probably after
> the configuration write but before the thresholds etc are set.
>
> That way those error paths are covered and it's more obvious what this
> is undoing.
Okay, seems logical.
> > +
> > iio->name = client->name;
> > iio->channels = *opt->chip_info->channels;
> > iio->num_channels = opt->chip_info->num_channels;
> > @@ -849,9 +879,9 @@ static int opt3001_probe(struct i2c_client *client)
> >
> > /* Make use of INT pin only if valid IRQ no. is given */
> > if (irq > 0) {
> > - ret = request_threaded_irq(irq, NULL, opt3001_irq,
> > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > - "opt3001", iio);
> > + ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
>
> Hmm. Without hardware too risky I think to touch it, but why is this only registering
> the interrupt after the userspace interfaces? That seems to be a nasty race.
Good point - this is the way it was ordered originally, so this potential
race has been sitting in the driver for 11 years.
--
Kind regards
CJD