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

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

On Tue, Feb 06, 2024 at 04:51:58PM +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 special cases as well as transmit abort handling and those are
> missing from two polling mode quirks.
> 
> Patch implements a generic polling mode by using existing code for
> interrupt mode. 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 a 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 to determine completed transfer and state
> transitions.
> 
> Loop tries to save power by sleeping "stetson guessed" range between
> 3 and 25 µS which falls between 10 cycles of High-speed mode 3.4 Mb/s
> and Fast mode 400 kHz. With it the CPU usage was reduced under heavy
> Fast mode I2C transfer without much increase in total transfer time but
> otherwise no more effort has been put to optimize this.
> 
> 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.

...

I believe even with a single point of out we may do better this one, see below.

> +static int i2c_dw_wait_transfer(struct dw_i2c_dev *dev)
> +{
> +	unsigned long timeout = dev->adapter.timeout;
> +	unsigned int stat;

> +	int ret = 0;

	int ret;

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

		ret = wait_for_completion_timeout(&dev->cmd_complete, timeout);

> +	} else {
> +		timeout += jiffies;
> +		do {
> +			if (try_wait_for_completion(&dev->cmd_complete))
> +				goto out;

				break;

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

		ret = time_before(jiffies, timeout);

> +	}

> +out:

	if (ret)
		return 0;

	return -ETIMEDOUT;

> +	return ret;
> +}

This will make same error code for both branches without need to synchronise,
meaning better maintenance.

-- 
With Best Regards,
Andy Shevchenko