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

[David Lechner <dlechner@baylibre.com>]

On 7/11/26 1:14 AM, 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.
> 
> 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 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.
> +	 */
> +	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;
> +	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;
> +	}
> +

This chunk of code is now repeated in both functions, so could be
moved to a new common function.

> +	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);
> +		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;

This part is repeated in both functions, so can be moved to the
common function below.

> +}
> +
> +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,