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
