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

[PATCH v4] 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 v4:
- Simplify error checks and end of functions (Andy)
- Refactor timeout check (Andy)
- Link to v3: https://lore.kernel.org/r/20260712-opt3001-unwind-cleanup-v3-1-5fa336876b08@gmail.com

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 | 192 ++++++++++++++++++++++++--------------------
 1 file changed, 106 insertions(+), 86 deletions(-)

diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
index 434ef7e034fe..12cfb0e58870 100644
--- a/drivers/iio/light/opt3001.c
+++ b/drivers/iio/light/opt3001.c
@@ -315,35 +315,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;
@@ -353,85 +330,128 @@ 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;
 	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SINGLE);
 
 	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(dev, "failed to write register %02x\n",
 			OPT3001_CONFIGURATION);
+
+	return ret;
+}
+
+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;
-	}
+	if (wait_event_timeout(opt->result_ready_queue, opt->result_ready,
+			       msecs_to_jiffies(OPT3001_RESULT_READY_LONG)))
+		ret = 0;
+	else
+		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;
+}
+
+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: 2e2f2de7532cbbc2269de8be20ec709606c6e79b
change-id: 20260708-opt3001-unwind-cleanup-9aeede365655

Best regards,
-- 
Kind regards,
Joshua Crofts

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

[Andy Shevchenko]

On Tue, Jul 14, 2026 at 05:36:48PM +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.

LGTM, FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

...

>  	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
> -	if (ret < 0) {
> +	if (ret < 0)

At some point consider dropping ' < 0' pieces for the i2c_smbus_write_*() cases.
Read cases are different, of course.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Joshua Crofts]

On Tue, 14 Jul 2026 20:04:10 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > -	if (ret < 0) {
> > +	if (ret < 0)  
> 
> At some point consider dropping ' < 0' pieces for the i2c_smbus_write_*() cases.
> Read cases are different, of course.
> 
> >  		dev_err(dev, "failed to write register %02x\n",
> >  			OPT3001_CONFIGURATION);
> > +
> > +	return ret;  
> 

Even after an 8-part patch set, this driver still needs cleaning up!

-- 
Kind regards

CJD