On Mon, 2007-05-21 at 17:04 -0500, Andrew Dyer wrote:
Hi, I noticed a bug in soft_i2c.c (or in the README depending on how you think about it).
If CONFIG_SOFT_I2C is defined and using open-collector I/O, then the README says that I2C_TRISTATE can be defined as null. If this is the case, then doing:
imd 29 0.0 2
will cause the following chain of calls in soft_i2c.c:
send_start() write_byte() read_byte() read_byte() send_stop()
The first read_byte() calls send_ack(0). send_ack(0) drives the data line low without releasing it. The following call to read_byte() calls I2C_TRISTATE, but in this case it is defined as NULL per the README. This leaves the data line driven low during the read of the 2nd byte causing data^h^h^h^hhair loss that I can ill afford :-)
I propose the following fix - modify the README to make say that I2C_TRISTATE should be defined even for open-collector setups, and add code to soft_i2c.c to explictly tristate the data at the end of send_ack(0)
Signed-off-by: Andrew Dyer amdyer@gmail.com
I think the README w.r.t I2C_TRISTATE is OK as is(don't change it). I do think the soft i2c driver is broken in several places w.r.t IC2_ACTIVE/I2C_TRISTATE and I2C reset sequence. The below patch is an attempt to fix it, but I havn't tested it.
Signed-off-by: Joakim Tjernlund joakim.tjernlund@transmode.se
diff --git a/common/soft_i2c.c b/common/soft_i2c.c index edad51b..ed3f8f8 100644 --- a/common/soft_i2c.c +++ b/common/soft_i2c.c @@ -100,15 +100,18 @@ static void send_reset(void) #endif I2C_TRISTATE; for(j = 0; j < 9; j++) { + if(I2C_READ) + send_start(); I2C_SCL(0); I2C_DELAY; + I2C_TRISTATE; + I2C_SDA(1); I2C_DELAY; I2C_SCL(1); I2C_DELAY; I2C_DELAY; } send_stop(); - I2C_TRISTATE; }
/*----------------------------------------------------------------------- @@ -124,12 +127,13 @@ static void send_start(void) #endif
I2C_DELAY; + I2C_TRISTATE; I2C_SDA(1); - I2C_ACTIVE; I2C_DELAY; I2C_SCL(1); I2C_DELAY; I2C_SDA(0); + I2C_ACTIVE; I2C_DELAY; }
@@ -152,9 +156,9 @@ static void send_stop(void) I2C_DELAY; I2C_SCL(1); I2C_DELAY; + I2C_TRISTATE; I2C_SDA(1); I2C_DELAY; - I2C_TRISTATE; }
@@ -172,14 +176,17 @@ static void send_ack(int ack)
I2C_SCL(0); I2C_DELAY; - I2C_ACTIVE; I2C_SDA(ack); + I2C_ACTIVE; I2C_DELAY; I2C_SCL(1); I2C_DELAY; I2C_DELAY; I2C_SCL(0); I2C_DELAY; + I2C_TRISTATE; + I2C_SDA(1); + I2C_DELAY; }
@@ -215,8 +222,8 @@ static int write_byte(uchar data) */ I2C_SCL(0); I2C_DELAY; - I2C_SDA(1); I2C_TRISTATE; + I2C_SDA(1); I2C_DELAY; I2C_SCL(1); I2C_DELAY; @@ -224,7 +231,6 @@ static int write_byte(uchar data) nack = I2C_READ; I2C_SCL(0); I2C_DELAY; - I2C_ACTIVE;
return(nack); /* not a nack is an ack */ } @@ -249,6 +255,7 @@ static uchar read_byte(int ack) * Read 8 bits, MSB first. */ I2C_TRISTATE; + I2C_SDA(1); data = 0; for(j = 0; j < 8; j++) { I2C_SCL(0);