Re: mistake the i2c_bitbang direction

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

kiyohara%kk.iij4u.or.jp@localhost wrote:

> > > > DIR() should be used only for SDA, not for SCL.
> > > > As I noted in the log message, using DIR(INPUT) to read SCL
> > > > in i2c_wait_for_scl() causes unexpected stop condition
> > > > in SCL=H, SDA=L and DIR(OUTPUT) case.
> > > 
> > > Furthermore, we have to do SETBITS() before DIR(OUTPUT) in some case
> > > otherwise the device might put unexpected glitch during delay between
> > > DIR(OUTPUT) and SETBITS().
> > 
> > Perhaps, it understood.
> > Then, we should not call i2c_wait_for_scl() in i2c_bitbang_send_start()
> > and i2c_bitbang_{read,write}_byte().  He will read the i2c bus.  If he is
> > called, we should set DIR(INPUT) it ahead of that.
> > Moreover, it will have to be checked that a variable 'bit' is not SDA in
> > i2c_bitbang_{read,write}_byte().  Possibly set DIR(OUTPUT). 
> 
> hmm... I am confusing.  ;-<
> However very noisy.  I will try to once debug.

We should define behavior of bitbang ops as MI APIs first.
I'd suggest the following definitions:

1. DIR()
 a. DIR(OUTPUT) enables an output driver (usually open collector/drain
    transistor) of SDA line. The output driver is turned on if 
    current SDA value is Low. On some hardware implementation without
    open-collector ports, it emulates open-collector by disabling output.
    Some other hardware implementation uses non open-collector drivers
    with some limitation (no multi-master etc) and protection registers.
 b. DIR(INPUT) disables (turns off) the output driver of SDA line.
 c. DIR(OUTPUT) shouldn't disable read ops of SDA. (see 3-b)
 d. DIR() controls only SDA, and doesn't control SCL.
    An output driver of SCL should always be enabled nevertheless
    current DIR() settings.
    (though output driver will be turned off if SCL is set to H
     and the driver is open-collector)

2. SETBITS()
 a. SETBITS() updates current status of both SDA and SCL.
 b. SCL output will follow the updated status immediately
    nevertheless current DIR() settings.
 c. SDA output will follow the updated status if current DIR() is OUTPUT.
 d. If current DIR() is INPUT, SDA output is not changed even if
    SDA status value is updated. The SDA output will be updated
    once after DIR() is changed to OUTPUT in such case.

3. READ
 a. READ ops against SDA during DIR(INPUT) should return current
    H/L status of SDA line.
 b. READ ops against SDA during DIR(OUTPUT) should still be valid
    (i.e. DIR(OUTPUT) shouldn't disable input ops), and it should return
    current H/L status of SDA line, though some hardware implementation
    can't return proper value (it might return output status of SDA).
    (on such implementation, arbitration with multi-masters won't work)
 c. READ ops against SCL should always be valid and it should return
    current H/L status of SCL line, though some hardware implementation
    can't return proper value (it might return output status of SCL).
    (on such implementation, wait interval requests from slaves won't work)

In this scope,

- DIR(INPUT) is not required on reading SCL.
  It causes a bad side effect because it could change SDA line
  during write ops.

- "Read in output mode" should not be warned by MD drivers.
  It's a valid op, though some hardware implemetation won't
  return an expected value.

- "SDA low with no output enable" warning might be worth to detect
  unexpected possible Low glitches on INPUT->OUTPUT transition,
  but I don't think it should be done by indivitual drivers.
  MD drivers shouldn't rely on such assumption anyway and
  open-collector emulation can be implemented without it.
  Or, is it still better to set SDA on all SETBITS() ops during DIR(INPUT)
  (and clear SDA right after DIR() is changed to OUTPUT) in i2c_bitbang.c
  as the attached patch?

Opinions?
---
Izumi Tsutsui


Index: i2c_bitbang.c
===================================================================
RCS file: /cvsroot/src/sys/dev/i2c/i2c_bitbang.c,v
retrieving revision 1.11
diff -u -r1.11 i2c_bitbang.c
--- i2c_bitbang.c       1 Jun 2008 01:13:18 -0000       1.11
+++ i2c_bitbang.c       8 Jul 2008 15:25:33 -0000
@@ -169,11 +169,12 @@
                val <<= 1;
 
                /* data is set at SCL H->L edge */
-               SETBITS(  0 |   0);
+               /* SDA is set here because DIR() is INPUT */
+               SETBITS(SDA |   0);
                delay(5);       /* clock low time (4.7 us) */
 
                /* read data at SCL L->H edge */
-               SETBITS(  0 | SCL);
+               SETBITS(SDA | SCL);
                if (i2c_wait_for_scl(v, ops) != 0)
                        return EIO;
                if (READ & SDA)
@@ -181,7 +182,7 @@
                delay(4);       /* clock high time (4.0 us) */
        }
        /* set SCL H->L before set SDA direction OUTPUT */
-       SETBITS(  0 |   0);
+       SETBITS(SDA |   0);
 
        /* set ack after SCL H->L edge */
        bit = (flags & I2C_F_LAST) ? SDA : 0;
@@ -199,7 +200,6 @@
        SETBITS(bit |   0);
 
        /* leave SCL=L and SDL=L to avoid unexpected start/stop condition */
-       DIR(INPUT);
        SETBITS(  0 |   0);
 
 
@@ -243,16 +243,18 @@
        delay(5);       /* clock low time (4.7 us) */
 
        /* read ack at L->H edge */
-       SETBITS(  0 | SCL);
+       /* SDA is set here because DIR() is INPUT */
+       SETBITS(SDA | SCL);
        if (i2c_wait_for_scl(v, ops) != 0)
                return EIO;
        error = (READ & SDA) ? EIO : 0;
        delay(4);       /* clock high time (4.0 us) */
 
-       /* leave SCL=L and SDL=L to avoid unexpected start/stop condition */
        /* set SCL H->L before set SDA direction OUTPUT */
-       SETBITS(  0 |   0);
+       SETBITS(SDA |   0);
        DIR(OUTPUT);
+       /* leave SCL=L and SDL=L to avoid unexpected start/stop condition */
+       SETBITS(  0 |   0);
 
        if (flags & I2C_F_STOP)
                (void) i2c_bitbang_send_stop(v, flags, ops);