Re: [PATCH 5/5] i2c: designware: Implement generic polling mode code for Wangxun 10Gb NIC

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

On Wed, Jan 31, 2024 at 04:16:52PM +0200, Jarkko Nikula wrote:
> I got an idea the i2c-designware should not need duplicated state
> machines for the interrupt and polling modes. The IP is practically the
> same and state transitions happens in response to the events that can be
> observed from the DW_IC_RAW_INTR_STAT register. Either by interrupts or
> by polling.
> 
> Another reasons are the interrupt mode is the most tested, has handling
> for many exceptions as well as transmit abort handling and those are
> missing from two polling mode quirks.
> 
> Patch implements generic polling mode code which shares the same code
> with interrupt mode code. This is done by moving event handling from the
> i2c_dw_isr() into a new i2c_dw_process_transfer() that will be called
> both from the i2c_dw_isr() and polling loop.
> 
> Polling loop is implemented in a new i2c_dw_wait_transfer() that is
> shared between both modes. In interrupt mode it waits for the completion
> object as before. In polling mode both completion object and
> DW_IC_RAW_INTR_STAT are polled.
> 
> I decided to convert the txgbe_i2c_dw_xfer_quirk() straight to generic
> polling mode code in this patch. It doesn't have HW dependent quirks
> like the amd_i2c_dw_xfer_quirk() does have and without users this patch
> is needless.

...

> +	regmap_read(dev->map, DW_IC_ENABLE, &enabled);
> +	regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &stat);
> +	if (!enabled || !(stat & ~DW_IC_INTR_ACTIVITY))
> +		return IRQ_NONE;
> +	if (pm_runtime_suspended(dev->dev) || stat == GENMASK(31, 0))
> +		return IRQ_NONE;

Hmm... This was in the original code, but to me checking for stat = ~0 is
logically to have done before checking for (set / cleared) bits in it.

...

> +{
> +	unsigned long timeout = dev->adapter.timeout;
> +	unsigned int stat;

> +	int ret = 0;

> +	if (!(dev->flags & ACCESS_POLLING)) {
> +		ret = wait_for_completion_timeout(&dev->cmd_complete,
> +						  timeout) ? 0 : -ETIMEDOUT;



> +	} else {

...this is redundant 'else' as redundant label below.

You can return earlier. But it might be done this way to have single point of
the (same) error code.

	if (...) {
		ret = ...;
		if (ret)
			return 0;
	} else {
		...
			if (try_wait_for_completion(&dev->cmd_complete))
				return 0;
		...
	}

	return ETIMEDOUT;


> +		timeout += jiffies;
> +		do {
> +			if (try_wait_for_completion(&dev->cmd_complete))
> +				goto out;
> +
> +			stat = i2c_dw_read_clear_intrbits(dev);
> +			if (stat)
> +				i2c_dw_process_transfer(dev, stat);
> +			else
> +				/* Try save some power */
> +				usleep_range(3, 25);
> +		} while (time_before(jiffies, timeout));
> +		ret = ETIMEDOUT;
> +	}
> +
> +out:
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko