diff mbox

i2c-mpc: generate START condition after STOP caused by read i2c_msg

Message ID OFCADC684E.A2599121-ONC12575B7.003B8AB5-C12575B7.003CF795@transmode.se (mailing list archive)
State Superseded, archived
Delegated to: Kumar Gala
Headers show

Commit Message

Joakim Tjernlund May 15, 2009, 11:05 a.m. UTC 
Esben Haabendal <esbenhaabendal@gmail.com> wrote on 15/05/2009 12:18:39:

>
> I have now spent a few hours trying a lot of different paths to fix
> this approach, but I simply cannot find a way to get i2c read to work
> without a trailing STOP condition with this controller.

I found a bug which lets me remove the "fix" and seems related to your
problem. Updated patch last in mail

>
> Is there a problem in applying my patch, as it should be "clean"
> improvement over the current implementation, which uses STOP
> condition, but does not work.
>
> Until Isomeone finds a way to get it to work without STOP, we will
> have a working driver, which I assume is what we want.

Your patch appears OK too. I just wanted to see if a better fix was
possible. Your patch is less risky and it is the safe bet so soon before
release.

 Jocke

Comments

Esben Haabendal May 15, 2009, 12:52 p.m. UTC | #1 
Your new patch also does not work.

Have you tested it?

I already tried something very much what you sent here, I believe the
only difference was that I named the "last" variable "stop". I also
tried several other aproaches, and none of them worked.  I would
appreciate not to have to test all of them seperately again through
this mailing list ;-)

Anyway, your patch also is in conflict with the MPC8360ERM. The spec
specifies that a repeated start must follow an ACK, and not a "NO
ACK".

When doing a repeated start after a NO ACK, the slave does not ACK the
address (I get an RXAK).  When doing as specified, ACK'ing the last
byte read and then doing a repeated START, i2c_wait() fails due to
CSR_MCF bit missing.  I thought it would be possible to find somewhere
to do a dummy read to get around this, but failed to do so without
breaking something else.

Could we go forward with my initial patch, and then continue the work
on this repeated START approach for future releases?

/Esben
Joakim Tjernlund May 15, 2009, 2:16 p.m. UTC | #2 
Esben Haabendal <esbenhaabendal@gmail.com> wrote on 15/05/2009 14:52:28:
>
> Your new patch also does not work.
>
> Have you tested it?

sure, works fine. I haven't stressed it too much though.

>
> I already tried something very much what you sent here, I believe the
> only difference was that I named the "last" variable "stop". I also
> tried several other aproaches, and none of them worked.  I would
> appreciate not to have to test all of them seperately again through
> this mailing list ;-)

:), point taken.

>
> Anyway, your patch also is in conflict with the MPC8360ERM. The spec
> specifies that a repeated start must follow an ACK, and not a "NO
> ACK".

Ouch, will have to check too, but later.

>
> When doing a repeated start after a NO ACK, the slave does not ACK the
> address (I get an RXAK).  When doing as specified, ACK'ing the last
> byte read and then doing a repeated START, i2c_wait() fails due to
> CSR_MCF bit missing.  I thought it would be possible to find somewhere
> to do a dummy read to get around this, but failed to do so without
> breaking something else.
>
> Could we go forward with my initial patch, and then continue the work
> on this repeated START approach for future releases?

Yes, go ahead.

 Jocke
Wolfram Sang May 18, 2009, 11:04 a.m. UTC | #3 
> Could we go forward with my initial patch, and then continue the work
> on this repeated START approach for future releases?

Then could you please send another version of your patch with the CodingStyle
issue removed and an updated description what this patch fixes and what still
needs to be resolved?

Regards,

   Wolfram
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 6c1cddd..04eff40 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -189,9 +189,6 @@  static int mpc_write(struct mpc_i2c *i2c, int target,
 	unsigned timeout = i2c->adap.timeout;
 	u32 flags = restart ? CCR_RSTA : 0;

-	/* Start with MEN */
-	if (!restart)
-		writeccr(i2c, CCR_MEN);
 	/* Start as master */
 	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX | flags);
 	/* Write target byte */
@@ -214,15 +211,12 @@  static int mpc_write(struct mpc_i2c *i2c, int target,
 }

 static int mpc_read(struct mpc_i2c *i2c, int target,
-		    u8 * data, int length, int restart)
+		    u8 * data, int length, int restart, int last)
 {
 	unsigned timeout = i2c->adap.timeout;
 	int i, result;
 	u32 flags = restart ? CCR_RSTA : 0;

-	/* Start with MEN */
-	if (!restart)
-		writeccr(i2c, CCR_MEN);
 	/* Switch to read - restart */
 	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX | flags);
 	/* Write target address byte - this time with the read flag set */
@@ -241,18 +235,18 @@  static int mpc_read(struct mpc_i2c *i2c, int target,
 		readb(i2c->base + MPC_I2C_DR);
 	}

-	for (i = 0; i < length; i++) {
+	for (i = length; i ; --i) {
 		result = i2c_wait(i2c, timeout, 0);
 		if (result < 0)
 			return result;

 		/* Generate txack on next to last byte */
-		if (i == length - 2)
+		if (i == 2)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
-		/* Generate stop on last byte */
-		if (i == length - 1)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
-		data[i] = readb(i2c->base + MPC_I2C_DR);
+		/* Generate stop on last byte, iff last transaction */
+		if (i == 1 && last)
+			writeccr(i2c, CCR_MIEN | CCR_MEN);
+		data[length-i] = readb(i2c->base + MPC_I2C_DR);
 	}

 	return length;
@@ -294,7 +288,7 @@  static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		tm_i2c_select_mux(pmsg->addr);
 		if (pmsg->flags & I2C_M_RD)
 			ret =
-			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
+			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i, i == num-1);
 		else
 			ret =
 			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);