David, On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@xxxxxxxxxxxxxx> wrote: > Signed-off-by: David Wu <david.wu@xxxxxxxxxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
