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

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

[Joshua Crofts]

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>
---
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 v3:
- Move duplicated code blocks to common functions (David)
- Change wrapping of comments and function call (Jonathan)
- Link to v2: https://lore.kernel.org/r/20260711-opt3001-unwind-cleanup-v2-1-47f617e00c65@gmail.com

Changes in v2:
- Remove ternary operator and add if/else (Andy)
- Remove timeout variable (Andy)
- Link to v1: https://lore.kernel.org/r/20260708-opt3001-unwind-cleanup-v1-1-900b2cfddb6a@gmail.com
---
 drivers/iio/light/opt3001.c | 194 +++++++++++++++++++++++++-------------------
 1 file changed, 109 insertions(+), 85 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 2bce6cd5f4e4..c72eb0eaeaef 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -316,35 +316,12 @@ 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_start_conversion(struct opt3001 *opt)
 {
 	struct i2c_client *client = opt->client;
 	struct device *dev = &client->dev;
-	int ret;
-	u16 mantissa;
 	u16 reg;
-	u8 exponent;
-	u16 value;
-	long timeout;
-
-	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;
-	}
+	int ret;
 
 	/* Reset data-ready indicator flag */
 	opt->result_ready = false;
@@ -354,7 +331,7 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 	if (ret < 0) {
 		dev_err(dev, "failed to read register %02x\n",
 			OPT3001_CONFIGURATION);
-		goto err;
+		return ret;
 	}
 
 	reg = ret;
@@ -364,75 +341,122 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 	if (ret < 0) {
 		dev_err(dev, "failed to write register %02x\n",
 			OPT3001_CONFIGURATION);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int opt3001_get_processed_irq(struct opt3001 *opt)
+{
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
+	u16 value;
+	int ret;
+
+	/*
+	 * 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;
+
+	ret = opt3001_start_conversion(opt);
+	if (ret)
 		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.
+	 */
+	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;
 	}
 
+	return 0;
+}
+
+static int opt3001_get_processed_noirq(struct opt3001 *opt)
+{
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
+	int ret;
+
+	ret = opt3001_start_conversion(opt);
+	if (ret)
+		return ret;
+
+	if (opt->int_time == OPT3001_INT_TIME_SHORT)
+		msleep(OPT3001_RESULT_READY_SHORT);
+	else
+		msleep(OPT3001_RESULT_READY_LONG);
+
+	/* 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);
+		return ret;
+	}
+
+	if (!(ret & OPT3001_CONFIGURATION_CRF))
+		return -ETIMEDOUT;
+
+	/* 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);
+		return ret;
+	}
+
+	opt->result = ret;
+	opt->result_ready = true;
+
+	return 0;
+}
+
+static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
+{
+	u16 mantissa;
+	u8 exponent;
+	int ret;
+
+	if (opt->use_irq)
+		ret = opt3001_get_processed_irq(opt);
+	else
+		ret = opt3001_get_processed_noirq(opt);
+	if (ret)
+		return ret;
+
 	exponent = OPT3001_REG_EXPONENT(opt->result);
 	mantissa = OPT3001_REG_MANTISSA(opt->result);
 

---
base-commit: fef4337eb2888c758c7058e1723903204f012a26
change-id: 20260708-opt3001-unwind-cleanup-9aeede365655

Best regards,
-- 
Kind regards

CJD

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

[Andy Shevchenko]

On Sun, Jul 12, 2026 at 12:32:49PM +0200, Joshua Crofts 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.

...

> static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)

>  	if (ret < 0) {
>  		dev_err(dev, "failed to write register %02x\n",
>  			OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	return 0;

Now it can be simply

	if (ret < 0)
		dev_err(dev, "failed to write register %02x\n",
			OPT3001_CONFIGURATION);

	return ret;

...

> +	ret = wait_event_timeout(opt->result_ready_queue,
> +				 opt->result_ready,
> +				 msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
> +	if (ret == 0)
> +		ret = -ETIMEDOUT;

ret is int and for holding an error code, with the above ret is rewritten
with positive number and might lead to subtle issues. The recommended way
to handle is

	if (wait_event_timeout(opt->result_ready_queue, opt->result_ready,
			       msecs_to_jiffies(OPT3001_RESULT_READY_LONG)))
		ret = 0;
	else
		ret = -ETIMEDOUT;

or something like that.

...

> +	/*
> +	 * 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;
>  	}
>  
> +	return 0;

Also can be simply

	return ret;

in both branches.

-- 
With Best Regards,
Andy Shevchenko

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

[Joshua Crofts]

On Sun, 12 Jul 2026 16:58:55 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > +	ret = wait_event_timeout(opt->result_ready_queue,
> > +				 opt->result_ready,
> > +				 msecs_to_jiffies(OPT3001_RESULT_READY_LONG));
> > +	if (ret == 0)
> > +		ret = -ETIMEDOUT;  
> 
> ret is int and for holding an error code, with the above ret is rewritten
> with positive number and might lead to subtle issues. The recommended way
> to handle is
> 
> 	if (wait_event_timeout(opt->result_ready_queue, opt->result_ready,
> 			       msecs_to_jiffies(OPT3001_RESULT_READY_LONG)))
> 		ret = 0;
> 	else
> 		ret = -ETIMEDOUT;
> 
> or something like that.

Sure, I can do that.

...

> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to write register %02x\n",
> > +			OPT3001_LOW_LIMIT);
> > +		return ret;
> >  	}
> >  
> > +	return 0;  
> 
> Also can be simply
> 
> 	return ret;
> 
> in both branches.

Yeah, I agonized over this since I did the change you describe in a
different patchset, and I was advised to do it the way as I did in
this patch so that it's more readable (i.e. we know which path is
the successful one). I'm really indifferent to either change and it
doesn't cut that many lines.

-- 
Kind regards

CJD