| Message ID | 1462371194-5809-5-git-send-email-david.wu@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
David, On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote: > Signed-off-by: David Wu <david.wu@rock-chips.com> > --- > 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
Hi Doug, ? 2016/5/6 6:56, Doug Anderson ??: > David, > > On Wed, May 4, 2016 at 7:13 AM, David Wu <david.wu@rock-chips.com> wrote: >> Signed-off-by: David Wu <david.wu@rock-chips.com> >> --- >> 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? > Yes, I would drop it next version?STATE_SETUP didn't make sense, it was not much more important, because there was a error printk for rk3x_i2c_setup() called failed. > ...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 > > >
diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c index 9eeb4e5..0838fcf 100644 --- a/drivers/i2c/busses/i2c-rk3x.c +++ b/drivers/i2c/busses/i2c-rk3x.c @@ -87,6 +87,7 @@ struct rk3x_i2c_calced_timings { enum rk3x_i2c_state { STATE_IDLE, + STATE_SETUP, STATE_START, STATE_READ, STATE_WRITE, @@ -174,6 +175,7 @@ static void rk3x_i2c_start(struct rk3x_i2c *i2c) { u32 val; + i2c->state = STATE_START; i2c_writel(i2c, REG_INT_START, REG_IEN); /* enable adapter with correct mode, send START condition */ @@ -451,6 +453,7 @@ static irqreturn_t rk3x_i2c_irq(int irqno, void *dev_id) rk3x_i2c_handle_stop(i2c, ipd); break; case STATE_IDLE: + case STATE_SETUP: break; } @@ -781,7 +784,7 @@ static int rk3x_i2c_setup(struct rk3x_i2c *i2c, struct i2c_msg *msgs, int num) i2c->addr = msgs[0].addr; i2c->busy = true; - i2c->state = STATE_START; + i2c->state = STATE_SETUP; i2c->processed = 0; i2c->error = 0;
Signed-off-by: David Wu <david.wu@rock-chips.com> --- Change in v7: - none drivers/i2c/busses/i2c-rk3x.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)