Re: [PATCH 7/7] iio: light: opt3001: switch driver to managed resources

[Jonathan Cameron]

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, &reg, 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.


> +
>  	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.

> +
>  	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.

> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						"opt3001", iio);
>  		if (ret)
>  			return dev_err_probe(dev, ret,
>  					     "failed to request IRQ #%d\n",
> @@ -864,34 +894,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);
> -	int ret;
> -	u16 reg;
> -
> -	if (opt->use_irq)
> -		free_irq(client->irq, iio);
> -
> -	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, &reg, 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 const struct opt3001_chip_info opt3001_chip_information = {
>  	.channels = &opt3001_channels,
>  	.chan_type = IIO_LIGHT,
> @@ -930,7 +932,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 = {
>