Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1394332 > unrolled thread

[PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP David Wu <david.wu@rock-chips.com> - 2016-05-04 16:20 +0200
    Re: [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP Doug Anderson <dianders@chromium.org> - 2016-05-06 01:00 +0200
      Re: [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add  STATE_SETUP "David.Wu" <david.wu@rock-chips.com> - 2016-05-06 05:30 +0200

#1394332 — [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP

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(-)

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;
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1395455

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

[toc] | [prev] | [next] | [standalone]

#1395589 — Re: [PATCH v7 4/9] i2c: rk3x: Move setting STATE_START and add STATE_SETUP

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
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web