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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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