Re: [PATCH v2] iio: light: opt3001: split opt3001_get_processed() logic

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 11 Jul 2026 08:14:54 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> Split the logic inside the opt3001_get_processed() function, as the
> current flow is hard to read, mixing IRQ and non-IRQ code blocks.
> 
> Separate the IRQ code path into its own function, same for the
> non-IRQ path.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
Hi Joshua

Thanks for doing.  The only thing I'll add to David's review
is to check if comments or code should be rewrapped after they
change indent.  Won't save many lines, but at least ensures the
code remains consistently formatted.

Jonathan

> ---
> This patch fixes the absolutely horrible code flow in the
> opt3001_get_processed() function, where the original implementation
> used checks whether IRQ mode is enabled to execute blocks of code,
> making the function hard to read. Ideas on how to improve the functions
> are more than welcome.
> 
> Originally suggested by Jonathan Cameron.
> ---
> Changes in v2:
> - Remove ternary operator and add if/else
> - Remove timeout variable
> - Link to v1: https://lore.kernel.org/r/20260708-opt3001-unwind-cleanup-v1-1-900b2cfddb6a@gmail.com
> ---
>  drivers/iio/light/opt3001.c | 197 ++++++++++++++++++++++++++------------------
>  1 file changed, 118 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 2bce6cd5f4e4..d1a2426cd355 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -316,36 +316,33 @@ static const struct iio_chan_spec opt3002_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(1),
>  };
>  
> -static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
> +static int opt3001_get_processed_irq(struct opt3001 *opt, int *val, int *val2)
>  {
>  	struct i2c_client *client = opt->client;
>  	struct device *dev = &client->dev;
> -	int ret;
>  	u16 mantissa;
> -	u16 reg;
>  	u8 exponent;
>  	u16 value;
> -	long timeout;
> +	u16 reg;
> +	int ret;
>  
> -	if (opt->use_irq) {
> -		/*
> -		 * Enable the end-of-conversion interrupt mechanism. Note that
> -		 * doing so will overwrite the low-level limit value however we
> -		 * will restore this value later on.
> -		 */
> -		ret = i2c_smbus_write_word_swapped(client,
> -						   OPT3001_LOW_LIMIT,
> -						   OPT3001_LOW_LIMIT_EOC_ENABLE);
> -		if (ret < 0) {
> -			dev_err(dev, "failed to write register %02x\n",
> -				OPT3001_LOW_LIMIT);
> -			return ret;
> -		}
> -
> -		/* Allow IRQ to access the device despite lock being set */
> -		opt->ok_to_ignore_lock = true;
> +	/*
> +	 * Enable the end-of-conversion interrupt mechanism. Note that
> +	 * doing so will overwrite the low-level limit value however we
> +	 * will restore this value later on.

Wrap comments that have change indent.

> +	 */
> +	ret = i2c_smbus_write_word_swapped(client,
> +					   OPT3001_LOW_LIMIT,
> +					   OPT3001_LOW_LIMIT_EOC_ENABLE);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to write register %02x\n",
> +			OPT3001_LOW_LIMIT);
> +		return ret;
>  	}
>  
> +	/* Allow IRQ to access the device despite lock being set */
> +	opt->ok_to_ignore_lock = true;
> +
>  	/* Reset data-ready indicator flag */
>  	opt->result_ready = false;
>  
> @@ -367,70 +364,33 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
>  		goto err;
>  	}
>  
> -	if (opt->use_irq) {
> -		/* Wait for the IRQ to indicate the conversion is complete */
> -		ret = wait_event_timeout(opt->result_ready_queue,
> -					 opt->result_ready,
> -					 msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
> -		if (ret == 0) {
> -			ret = -ETIMEDOUT;
> -			goto err;
> -		}
> -	} else {
> -		/* Sleep for result ready time */
> -		timeout = (opt->int_time == OPT3001_INT_TIME_SHORT) ?
> -			OPT3001_RESULT_READY_SHORT : OPT3001_RESULT_READY_LONG;
> -		msleep(timeout);
> -
> -		/* Check result ready flag */
> -		ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
> -		if (ret < 0) {
> -			dev_err(dev, "failed to read register %02x\n",
> -				OPT3001_CONFIGURATION);
> -			goto err;
> -		}
> -
> -		if (!(ret & OPT3001_CONFIGURATION_CRF)) {
> -			ret = -ETIMEDOUT;
> -			goto err;
> -		}
> -
> -		/* Obtain value */
> -		ret = i2c_smbus_read_word_swapped(client, OPT3001_RESULT);
> -		if (ret < 0) {
> -			dev_err(dev, "failed to read register %02x\n",
> -				OPT3001_RESULT);
> -			goto err;
> -		}
> -		opt->result = ret;
> -		opt->result_ready = true;
> -	}
> +	ret = wait_event_timeout(opt->result_ready_queue,
> +				 opt->result_ready,
> +				 msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
> +	if (ret == 0)
> +		ret = -ETIMEDOUT;
>  
>  err:
> -	if (opt->use_irq)
> -		/* Disallow IRQ to access the device while lock is active */
> -		opt->ok_to_ignore_lock = false;
> +	opt->ok_to_ignore_lock = false;
>  
>  	if (ret < 0)
>  		return ret;
>  
> -	if (opt->use_irq) {
> -		/*
> -		 * Disable the end-of-conversion interrupt mechanism by
> -		 * restoring the low-level limit value (clearing
> -		 * OPT3001_LOW_LIMIT_EOC_ENABLE). Note that selectively clearing
> -		 * those enable bits would affect the actual limit value due to
> -		 * bit-overlap and therefore can't be done.
> -		 */
> -		value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
> -		ret = i2c_smbus_write_word_swapped(client,
> -						   OPT3001_LOW_LIMIT,
> -						   value);
> -		if (ret < 0) {
> -			dev_err(dev, "failed to write register %02x\n",
> -				OPT3001_LOW_LIMIT);
> -			return ret;
> -		}
> +	/*
> +	 * Disable the end-of-conversion interrupt mechanism by
> +	 * restoring the low-level limit value (clearing
> +	 * OPT3001_LOW_LIMIT_EOC_ENABLE). Note that selectively clearing
> +	 * those enable bits would affect the actual limit value due to
> +	 * bit-overlap and therefore can't be done.
And this one.
> +	 */
> +	value = (opt->low_thresh_exp << 12) | opt->low_thresh_mantissa;
> +	ret = i2c_smbus_write_word_swapped(client,
> +					   OPT3001_LOW_LIMIT,
> +					   value);

I'd rewrap this as well probably. I'm fine with it one slightly long line.

> +	if (ret < 0) {
> +		dev_err(dev, "failed to write register %02x\n",
> +			OPT3001_LOW_LIMIT);
> +		return ret;
>  	}
>  
>  	exponent = OPT3001_REG_EXPONENT(opt->result);