David,

On Wed, May 4, 2016 at 7:13 AM, David Wu [off-list ref] wrote:

Signed-off-by: David Wu <redacted>
---
Change in v7:
- none

 drivers/i2c/busses/i2c-rk3x.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Probably this change could be dropped now.  Switching the location of
setting START_START was much more important when you were supporting
HIGH SPEED mode.  I don't think we need it anymore, right?

...if we want to keep this change, I'd say:

1. Add a description, like maybe:

To help with debugging add a STATE_SETUP between STATE_IDLE and
STATE_START to make it more obvious that we're not actually idle but we
also haven't initiated the start bit.  This change is not expected to
have any impact but it does delay the changing of state to STATE_START.
If previously we were getting an erroneous interrupt before we actually
sent the start bit we'll now be treating that differently.  The new
behavior (catching the erroneous interrupt) should be better.

2. Change "i2c->state = STATE_SETUP" to the _start_ of
rk3x_i2c_setup().  That would have a better chance of catching a
spurious interrupt.

3. Add an error check at the start of rk3x_i2c_irq() similar to the
check for STATE_IDLE (or use the same check and modify the printk).
Specifically the justification for adding STATE_SETUP is to help with
debugging (catch interrupts that were unexpected and print more info
about our state), so we should make it useful for this.


-Doug