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

[PATCH] 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.
---
 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..8430f4ddc23a 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.
+	 */
+	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.
+	 */
+	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;
 	}
 
 	exponent = OPT3001_REG_EXPONENT(opt->result);
@@ -441,6 +401,85 @@ static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
 	return IIO_VAL_INT_PLUS_MICRO;
 }
 
+static int opt3001_get_processed_noirq(struct opt3001 *opt, int *val, int *val2)
+{
+	struct i2c_client *client = opt->client;
+	struct device *dev = &client->dev;
+	long timeout;
+	u16 mantissa;
+	u8 exponent;
+	int ret;
+	u16 reg;
+
+	/* Reset data-ready indicator flag */
+	opt->result_ready = false;
+
+	/* Configure for single-conversion mode and start a new conversion */
+	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;
+	}
+
+	reg = ret;
+	opt3001_set_mode(opt, &reg, OPT3001_CONFIGURATION_M_SINGLE);
+
+	ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
+	if (ret < 0) {
+		dev_err(dev, "failed to write register %02x\n",
+			OPT3001_CONFIGURATION);
+		goto err;
+	}
+
+	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;
+
+err:
+	if (ret < 0)
+		return ret;
+
+	exponent = OPT3001_REG_EXPONENT(opt->result);
+	mantissa = OPT3001_REG_MANTISSA(opt->result);
+
+	opt3001_to_iio_ret(opt, exponent, mantissa, val, val2);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int opt3001_get_processed(struct opt3001 *opt, int *val, int *val2)
+{
+	if (opt->use_irq)
+		return opt3001_get_processed_irq(opt, val, val2);
+
+	return opt3001_get_processed_noirq(opt, val, val2);
+}
+
 static int opt3001_get_int_time(struct opt3001 *opt, int *val, int *val2)
 {
 	*val = 0;

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

Best regards,
-- 
Kind regards

CJD

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

[Andy Shevchenko]

On Wed, Jul 08, 2026 at 09:33:20PM +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.

> +	timeout = (opt->int_time == OPT3001_INT_TIME_SHORT) ?
> +		   OPT3001_RESULT_READY_SHORT : OPT3001_RESULT_READY_LONG;
> +	msleep(timeout);

It's not used after, so

	if (opt->int_time == OPT3001_INT_TIME_SHORT)
		msleep(OPT3001_RESULT_READY_SHORT);
	else
		msleep(OPT3001_RESULT_READY_LONG);

-- 
With Best Regards,
Andy Shevchenko