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

[Andrew Dyer]

On 5/22/07, Joakim Tjernlund <joakim.tjernlund at 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 at 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.


-- 
Hardware, n.:
        The parts of a computer system that can be kicked.