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
