On Tue, Jun 2, 2009 at 5:12 PM, Grant Likely [off-list ref] wr=
ote:

Hi Esben and Ben,

Sorry for the lateness in reviewing this. =A0FWIW, here are my comments.

g.

On Mon, May 18, 2009 at 11:22 PM, Esben Haabendal
[off-list ref] wrote:

quoted

This fixes MAL (arbitration lost) bug caused by illegal use of
RSTA (repeated START) after STOP condition generated after last byte
of reads. With this patch, it is possible to do an i2c_transfer() with
additional i2c_msg's following the I2C_M_RD messages.

It still needs to be resolved if it is possible to fix this issue
by removing the STOP condition after reads in a robust way.

Signed-off-by: Esben Haabendal <redacted>
---
=A0drivers/i2c/busses/i2c-mpc.c | =A0 =A09 +++++++--
=A01 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 4af5c09..0199f9a 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c

@@ -456,17 +456,22 @@ static int mpc_xfer(struct i2c_adapter *adap, stru=

ct

quoted

i2c_msg *msgs, int num)
=A0 =A0 =A0 =A0}

=A0 =A0 =A0 =A0for (i =3D 0; ret >=3D 0 && i < num; i++) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 int restart =3D i;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pmsg =3D &msgs[i];
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_dbg(i2c->dev,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Doing %s %d bytes to 0x%=

02x - %d of %d messages\n",

quoted

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pmsg->flags & I2C_M_RD ? =

"read" : "write",

quoted

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pmsg->len, pmsg->addr, i =

+ 1, num);

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i > 0 && ((pmsg - 1)->flags & I2C_M_RD=

))

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 restart =3D 0;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (pmsg->flags & I2C_M_RD)
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_read(i2c, pmsg=

->addr, pmsg->buf, pmsg->len,

quoted

i);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_read(i2c, pmsg=

->addr, pmsg->buf, pmsg->len,

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =

=A0restart);

quoted

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0else
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_write(i2c, pms=

g->addr, pmsg->buf, pmsg->len,

quoted

i);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_write(i2c, pms=

g->addr, pmsg->buf, pmsg->len,

quoted

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =

=A0 restart);

quoted

=A0 =A0 =A0 =A0}

Hmmm, seems to be a rather convoluted code path. =A0Surely this could be
handled in a clearer way. =A0The whole (pmsg - 1) bit raises my eyebrows
(I'd rather see msgs[i-1]). =A0At the very least this deserves an
comment describing what it is doing. =A0The following might be better
for the next person who has to read this code:

+ =A0 =A0 =A0int restart =3D 0;
=A0 =A0 =A0 for (i =3D 0; ret >=3D 0 && i < num; i++) {
=A0 =A0 =A0 =A0 =A0 =A0 =A0 pmsg =3D &msgs[i];
=A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(i2c->dev,
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "Doing %s %d bytes to 0x%02x =

- %d of %d messages\n",

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pmsg->flags & I2C_M_RD ? "rea=

d" : "write",

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 pmsg->len, pmsg->addr, i + 1,=

 num);

=A0 =A0 =A0 =A0 =A0 =A0 =A0 if (pmsg->flags & I2C_M_RD)
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_read(i2c, pmsg-=
addr, pmsg->buf, pmsg->len,
i);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_read(i2c, pmsg-=
addr, pmsg->buf, pmsg->len,
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

restart);

=A0 =A0 =A0 =A0 =A0 =A0 =A0 else
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D
- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_write(i2c, pmsg=

->addr, pmsg->buf, pmsg->len,

i);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_write(i2c, pmsg=

->addr, pmsg->buf, pmsg->len,

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

 restart);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Only set the restart flag if this was no=

t an

I2C_M_RD transaction
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* because it causes an illegal use of
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* RSTA (repeated START) after STOP condi=

tion

generated after last byte
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* of reads =A0*/
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 restart =3D (pmsg->flags & I2C_M_RD =3D=3D =

0);

=A0 =A0 =A0 }

BTW, since you're touching these lines anyway, please clean up the
whitespace usage.  Since you have to break the line anyway, it no
longer makes sense for the "ret =3D " to be on a separate line anymore.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.