Re: [PATCH v2 09/10] iio: light: opt3001: switch driver to managed resources

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 12 May 2026 12:57:29 +0200
Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@kernel.org> wrote:

> From: Joshua Crofts <joshua.crofts1@gmail.com>
> 
> 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@gmail.com>
Some trivial stuff inline.  I'll probably wait for sashiko to catch up with
it's backlog and get to this one.  If nothing comes up there or in other reviews
I'm fine tweaking the stuff below whilst applying the series.

Jonathan

> ---
>  drivers/iio/light/opt3001.c | 75 +++++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 39ea60c0af2c1ec7473d49b73778cac1f9bb6086..1319e5941b66bd82e4dc2badf5ea27cacbcfd54a 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -702,6 +702,31 @@ static int opt3001_read_id(struct opt3001 *opt)
>  	return 0;
>  }
>  
> +static void opt3001_power_off(void *data)
> +{
> +	struct opt3001 *opt = data;
> +	struct i2c_client *client = opt->client;
> +	struct device *dev = opt->dev;
> +
Really trivial but why the blank line here?  If nothing else comes up
I might bother dropping it whilst picking up the series.

> +	int ret;
> +	u16 reg;
Obviously not a change you made - but none the less reg is a bad name.
It is often used for register address rather than value.  So better as
regval.  Again maybe I'll tweak this if nothing comes up but if you
do another version for other reasons nice to change it.

> +
> +	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to read register %02x\n",
> +			OPT3001_CONFIGURATION);
> +		return;
> +	}
> +
> +	reg = ret;
> +	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
> +
> +	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
> +	if (ret < 0)
> +		dev_err(dev, "failed to write to register %02x\n",
> +			OPT3001_CONFIGURATION);
> +}
> +
>  static int opt3001_configure(struct opt3001 *opt)
>  {
>  	struct i2c_client *client = opt->client;
> @@ -740,6 +765,11 @@ static int opt3001_configure(struct opt3001 *opt)
>  		return dev_err_probe(dev, ret, "failed to write register %02x\n",
>  				     OPT3001_CONFIGURATION);
>  
> +	ret = devm_add_action_or_reset(dev, opt3001_power_off, opt);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register power off function\n");
> +
>  	ret = i2c_smbus_read_word_swapped(client, OPT3001_LOW_LIMIT);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "failed to read register %02x\n",
> @@ -834,7 +864,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 ret;
> +
>  	init_waitqueue_head(&opt->result_ready_queue);
>  	i2c_set_clientdata(client, iio);
>  
> @@ -856,13 +889,12 @@ 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,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						"opt3001", iio);
>  		if (ret)
> -			return dev_err_probe(dev, ret,
> -					     "failed to request IRQ #%d\n",
> -					     irq);
> +			return ret;
> +
>  		opt->use_irq = true;
>  	} else {
>  		dev_dbg(dev, "enabling interrupt-less operation\n");
> @@ -876,34 +908,6 @@ static int opt3001_probe(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static void opt3001_remove(struct i2c_client *client)
> -{
> -	struct iio_dev *iio = i2c_get_clientdata(client);
> -	struct opt3001 *opt = iio_priv(iio);
> -	struct device *dev = opt->dev;
> -	int ret;
> -	u16 reg;
> -
> -	if (opt->use_irq)
> -		free_irq(client->irq, iio);
> -
> -	ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to read register %02x\n",
> -			OPT3001_CONFIGURATION);
> -		return;
> -	}
> -
> -	reg = ret;
> -	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SHUTDOWN);
> -
> -	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to write register %02x\n",
> -			OPT3001_CONFIGURATION);
> -	}
> -}
> -
>  static const struct opt3001_chip_info opt3001_chip_information = {
>  	.channels = &opt3001_channels,
>  	.chan_type = IIO_LIGHT,
> @@ -942,7 +946,6 @@ MODULE_DEVICE_TABLE(of, opt3001_of_match);
>  
>  static struct i2c_driver opt3001_driver = {
>  	.probe = opt3001_probe,
> -	.remove = opt3001_remove,
>  	.id_table = opt3001_id,
>  
>  	.driver = {
>