Re: [U-Boot-Users] [PATCH] bug in README and soft_i2c.c

On 5/22/07, Joakim Tjernlund joakim.tjernlund@transmode.se wrote:

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;
  

}

/*-----------------------------------------------------------------------

I'm not sure about this part. I think the idea was to send a full transaction. You've added a bunch of repeated start commands in here which may not work as the data is low when it shouldn't be. I think the best you can hope for is to try and clock through whatever state machines might be out there.

A better solution might be to test the state of data (and clock would be good too), and repeat the clearing loop some number of times. If the data never goes high or clock is stuck low, print a message and bail out.

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

}

this looks funky to me for the case where you have tri-state I/O - you are specifically not driving sda high, instead letting it float up with the pullup. If you have active drivers, IMHO you should use them to make nice clean edges, not let the pullup slowly drag the line high :-)

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

}

more relying on a pullup here. You could argue the tristate belongs directly after setting SDA.

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

more relying on a pullup when you don't have to.

@@ -249,6 +255,7 @@ static uchar read_byte(int ack) * Read 8 bits, MSB first.

b> */

    I2C_TRISTATE;

•   I2C_SDA(1);
    data = 0;
    for(j = 0; j < 8; j++) {
            I2C_SCL(0);
  
  
  

again the pullup vs. active drive. I agree about adding this line, but I would put it before the I2C_TRISTATE.