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

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

[Esben Haabendal]

This fixes MAL (arbitration lost) bug caused by illegal use of
RSTA (repeated START) after STOP condition generated after last byte
of reads.

Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
---
  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
  1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 4af5c09..9fe05d9 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, 
struct i2c_msg *msgs, int num)
  	}

  	for (i = 0; ret >= 0 && i < num; i++) {
+		int restart = i;
  		pmsg = &msgs[i];
  		dev_dbg(i2c->dev,
  			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
  			pmsg->flags & I2C_M_RD ? "read" : "write",
  			pmsg->len, pmsg->addr, i + 1, num);
+		if (i > 0 && ((pmsg-1)->flags & I2C_M_RD))
+			restart = 0;
  		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,
+				     restart);
  		else
  			ret =
-			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
+			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
+				      restart);
  	}
  	mpc_i2c_stop(i2c);
  	return (ret < 0) ? ret : num;
-- 
1.6.0.4

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]

On Thu, May 14, 2009 at 10:10:03AM +0200, Esben Haabendal wrote:
> This fixes MAL (arbitration lost) bug caused by illegal use of
> RSTA (repeated START) after STOP condition generated after last byte
> of reads.
>

Could you elaborate a bit, please? Like an example when the original bug
occured (so I could reproduce easily) and how this patch actually solves
the problem.


> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
> ---
>  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 4af5c09..9fe05d9 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,  
> struct i2c_msg *msgs, int num)
>  	}
>
>  	for (i = 0; ret >= 0 && i < num; i++) {
> +		int restart = i;
>  		pmsg = &msgs[i];
>  		dev_dbg(i2c->dev,
>  			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
>  			pmsg->flags & I2C_M_RD ? "read" : "write",
>  			pmsg->len, pmsg->addr, i + 1, num);
> +		if (i > 0 && ((pmsg-1)->flags & I2C_M_RD))

CodingStyle: Spaces around '-'

> +			restart = 0;
>  		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,
> +				     restart);
>  		else
>  			ret =
> -			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> +			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
> +				      restart);
>  	}
>  	mpc_i2c_stop(i2c);
>  	return (ret < 0) ? ret : num;
> -- 
> 1.6.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Esben Haabendal]

Wolfram Sang wrote:
> On Thu, May 14, 2009 at 10:10:03AM +0200, Esben Haabendal wrote:
>   
>> This fixes MAL (arbitration lost) bug caused by illegal use of
>> RSTA (repeated START) after STOP condition generated after last byte
>> of reads.
>>
>>     
>
> Could you elaborate a bit, please? Like an example when the original bug
> occured (so I could reproduce easily) and how this patch actually solves
> the problem.
>   
Sure. I used the following code to initialize a CS4265 chip:
(simplified here for the example)

static int
i2c_init_chip(void)
{
    int err=0;
    char write_dac_control_1[] = { 0x03, chip->regs.dac_control_1 };
    char seek_chip_id[] = { 0x01 };
    char write_power_control[] = { 0x02, chip->regs.power_control };
    struct i2c_msg cs4265_init_cmds[] = {
        { chip->i2c_addr, 0, 2, write_dac_control_1 },
        { chip->i2c_addr, 0, 1, seek_chip_id },
        { chip->i2c_addr, I2C_M_RD, 1, &regs.chip_id },
        { chip->i2c_addr, 0, 2, write_power_control },
    };

    err = i2c_transfer(i2c, cs4265_init_cmds,
               sizeof(cs4265_init_cmds)/sizeof(*cs4265_init_cmds));
    if (err < 0) {
        printk(KERN_ERR "i2c_transfer failed: %d\n", err);
        return err;
    }

    return 0;
}

I have added some additional debug output and traces something like this:

[   36.114297] writeccr 80 [MEN]
[   36.114324] mpc_xfer: 2 bytes to 4f:W - 1 of 5 messages
[   36.120176] next 1 bytes to 4f:W
[   36.124159] mpc_write addr=4f len=2
[   36.128044] writeccr 80 [MEN]
[   36.128062] writeccr f0 [MEN MIEN MSTA MTX] -> START
[   36.128219] I2C: SR=a2
[   36.131528] I2C: SR=a6
[   36.134406] I2C: SR=a2

[   36.137165] mpc_xfer: 1 bytes to 4f:W - 2 of 5 messages
[   36.142802] next 1 bytes to 4f:R
[   36.146699] mpc_write addr=4f len=1
[   36.150583] writeccr f4 [MEN MIEN MSTA MTX RSTA] -> repeated START
[   36.150758] I2C: SR=a2
[   36.153851] I2C: SR=a6

[   36.156236] mpc_xfer: 1 bytes to 4f:R - 3 of 5 messages
[   36.162081] next 2 bytes to 4f:W
[   36.166062] mpc_read addr=4f len=1
[   36.169858] writeccr f4 [MEN MIEN MSTA MTX RSTA] -> repeated START
[   36.170031] I2C: SR=a6
[   36.172993] writeccr e8 [MEN MIEN MSTA TXAK] -> receive & no ACK
[   36.173143] I2C: SR=a7
[   36.175511] writeccr c8 [MEN MIEN TXAK] -> STOP

[   36.175529] mpc_xfer: 2 bytes to 4f:W - 4 of 5 messages
[   36.181804] next 2 bytes to 4f:W
[   36.185788] mpc_write addr=4f len=2
[   36.189672] writeccr f4 [MEN MIEN MSTA MTX RSTA] -> repeated START
[   36.189704] I2C: SR=b7
[   36.192072] I2C: MAL
[   36.195075] i2c_wait(address) error: -5
[   36.199311] writeccr 80

The problem is that after the STOP condition, the following i2c_msg will be
attempted with a repeated START, which according to specification will
cause a MAL, which also happens.  My MPC8360ERM reads:

"Attempting a repeated START at the wrong time (or if the bus is owned
by another master), results in loss of arbitration."

Which I know read as it that we must own the I2C bus when using RSTA.


>
>   
>> Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>
>> ---
>>  drivers/i2c/busses/i2c-mpc.c |    9 +++++++--
>>  1 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
>> index 4af5c09..9fe05d9 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,  
>> struct i2c_msg *msgs, int num)
>>  	}
>>
>>  	for (i = 0; ret >= 0 && i < num; i++) {
>> +		int restart = i;
>>  		pmsg = &msgs[i];
>>  		dev_dbg(i2c->dev,
>>  			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
>>  			pmsg->flags & I2C_M_RD ? "read" : "write",
>>  			pmsg->len, pmsg->addr, i + 1, num);
>> +		if (i > 0 && ((pmsg-1)->flags & I2C_M_RD))
>>     
>
> CodingStyle: Spaces around '-'
>   

Ok.
>   
>> +			restart = 0;
>>  		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,
>> +				     restart);
>>  		else
>>  			ret =
>> -			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
>> +			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len,
>> +				      restart);
>>  	}
>>  	mpc_i2c_stop(i2c);
>>  	return (ret < 0) ? ret : num;
>> -- 
>> 1.6.0.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>     
>
>   

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Joakim Tjernlund]

>
> Wolfram Sang wrote:
> > On Thu, May 14, 2009 at 10:10:03AM +0200, Esben Haabendal wrote:
> >
> >> This fixes MAL (arbitration lost) bug caused by illegal use of
> >> RSTA (repeated START) after STOP condition generated after last byte
> >> of reads.
> >>
> >>
> >
> > Could you elaborate a bit, please? Like an example when the original bug
> > occured (so I could reproduce easily) and how this patch actually solves
> > the problem.
> >
> Sure. I used the following code to initialize a CS4265 chip:
> (simplified here for the example)
>
> static int
> i2c_init_chip(void)
> {
>     int err=0;
>     char write_dac_control_1[] = { 0x03, chip->regs.dac_control_1 };
>     char seek_chip_id[] = { 0x01 };
>     char write_power_control[] = { 0x02, chip->regs.power_control };
>     struct i2c_msg cs4265_init_cmds[] = {
>         { chip->i2c_addr, 0, 2, write_dac_control_1 },
>         { chip->i2c_addr, 0, 1, seek_chip_id },
>         { chip->i2c_addr, I2C_M_RD, 1, &regs.chip_id },
>         { chip->i2c_addr, 0, 2, write_power_control },
>     };
>
>     err = i2c_transfer(i2c, cs4265_init_cmds,
>                sizeof(cs4265_init_cmds)/sizeof(*cs4265_init_cmds));
>     if (err < 0) {
>         printk(KERN_ERR "i2c_transfer failed: %d\n", err);
>         return err;
>     }
>
>     return 0;
> }
>
> I have added some additional debug output and traces something like this:
>
> [   36.114297] writeccr 80 [MEN]
> [   36.114324] mpc_xfer: 2 bytes to 4f:W - 1 of 5 messages
> [   36.120176] next 1 bytes to 4f:W
> [   36.124159] mpc_write addr=4f len=2
> [   36.128044] writeccr 80 [MEN]
> [   36.128062] writeccr f0 [MEN MIEN MSTA MTX] -> START
> [   36.128219] I2C: SR=a2
> [   36.131528] I2C: SR=a6
> [   36.134406] I2C: SR=a2
>
> [   36.137165] mpc_xfer: 1 bytes to 4f:W - 2 of 5 messages
> [   36.142802] next 1 bytes to 4f:R
> [   36.146699] mpc_write addr=4f len=1
> [   36.150583] writeccr f4 [MEN MIEN MSTA MTX RSTA] -> repeated START
> [   36.150758] I2C: SR=a2
> [   36.153851] I2C: SR=a6
>
> [   36.156236] mpc_xfer: 1 bytes to 4f:R - 3 of 5 messages
> [   36.162081] next 2 bytes to 4f:W
> [   36.166062] mpc_read addr=4f len=1
> [   36.169858] writeccr f4 [MEN MIEN MSTA MTX RSTA] -> repeated START
> [   36.170031] I2C: SR=a6
> [   36.172993] writeccr e8 [MEN MIEN MSTA TXAK] -> receive & no ACK
> [   36.173143] I2C: SR=a7
> [   36.175511] writeccr c8 [MEN MIEN TXAK] -> STOP
>
> [   36.175529] mpc_xfer: 2 bytes to 4f:W - 4 of 5 messages
> [   36.181804] next 2 bytes to 4f:W
> [   36.185788] mpc_write addr=4f len=2
> [   36.189672] writeccr f4 [MEN MIEN MSTA MTX RSTA] -> repeated START
> [   36.189704] I2C: SR=b7
> [   36.192072] I2C: MAL
> [   36.195075] i2c_wait(address) error: -5
> [   36.199311] writeccr 80
>
> The problem is that after the STOP condition, the following i2c_msg will be
> attempted with a repeated START, which according to specification will
> cause a MAL, which also happens.  My MPC8360ERM reads:
>
> "Attempting a repeated START at the wrong time (or if the bus is owned
> by another master), results in loss of arbitration."
>
> Which I know read as it that we must own the I2C bus when using RSTA.

I agree with the theory, but isn't the problem here that STOP is performed in
the middle of this transaction?
Remove the STOP and RSTA will work I think.

 Jocke

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Joakim Tjernlund]

Esben Haabendal <esbenhaabendal@gmail.com> wrote on 14/05/2009 12:17:48=
:
> On Thu, May 14, 2009 at 11:58 AM, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
> >>
> >> The problem is that after the STOP condition, the following i2c_ms=
g will be
> >> attempted with a repeated START, which according to specification =
will
> >> cause a MAL, which also happens. =A0My MPC8360ERM reads:
> >>
> >> "Attempting a repeated START at the wrong time (or if the bus is o=
wned
> >> by another master), results in loss of arbitration."
> >>
> >> Which I know read as it that we must own the I2C bus when using RS=
TA.
> >
> > I agree with the theory, but isn't the problem here that STOP is pe=
rformed in
> > the middle of this transaction?
> > Remove the STOP and RSTA will work I think.
>
> That was also my first idea.
>
> But at least for my CS4265, reads does not work without a STOP follow=
ing the
> last byte. I guess I am not the first to experience this, as indicate=
d
> by the current
> i2c-mpc.c implementation.
> As far as I can understand the I2C specification, a STOP should not b=
e required
> after reads, but I am not sure.
>
> The cost of the additional STOP is really small.  The only issue I ca=
n see,
> is that with the STOP in the midle of an i2c_transfer, the I2C bus is=
 released.
> This naturally also happens with the current implementation, so I bel=
ieve
> that this patch is a clean improvement.
>
> Anyways, if someone can figure out how to achive this without STOP,
> I will be happy (re)test with my hardware setup here.

>From just playing a little, I think the below patch will get you far.
However, I had to change this too to make it stable:
static void mpc_i2c_start(struct mpc_i2c *i2c)
{
	/* Clear arbitration */
	writeb(0, i2c->base + MPC_I2C_SR);
#if 1 // fix, not sure why
	writeccr(i2c, 0);
	udelay(5);
#endif
	/* Start with MEN */
	writeccr(i2c, CCR_MEN);
}

PS.
   I added back the linuxppc list, let it stay.


diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.=
c
index 6c1cddd..d7edcd2 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 targe=
t,
 	unsigned timeout =3D i2c->adap.timeout;
 	u32 flags =3D 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 */
@@ -220,9 +217,6 @@ static int mpc_read(struct mpc_i2c *i2c, int target=
,
 	int i, result;
 	u32 flags =3D 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,15 @@ static int mpc_read(struct mpc_i2c *i2c, int targ=
et,
 		readb(i2c->base + MPC_I2C_DR);
 	}

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

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

 	return length;

=

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Esben Haabendal]

Hi

Your patch (and the small addition to mpc_i2c_start) does not work for me.

[   35.765803] mpc_xfer()
[   35.785480] writeccr 0
[   35.785505] writeccr 80
[   35.785523] mpc_xfer: 1 bytes to 2c:W - 1 of 2 messages
[   35.798817] mpc_write addr=3D2c len=3D1 restart=3D0
[   35.815327] writeccr f0
[   35.815503] I2C: SR=3Da2
[   35.818675] I2C: SR=3Da6
[   35.821450] mpc_xfer: 1 bytes to 2c:R - 2 of 2 messages
[   35.827119] mpc_read addr=3D2c len=3D1 restart=3D1
[   35.837463] writeccr f4
[   35.837641] I2C: SR=3Da6
[   35.840011] writeccr e8
[   35.840133] I2C: SR=3Da3
[   35.843596] writeccr 80
[   35.843632] mpc_xfer()
[   35.855068] writeccr 0
[   35.855093] writeccr 80
[   35.855111] mpc_xfer: 1 bytes to 2c:W - 1 of 2 messages
[   35.865346] mpc_write addr=3D2c len=3D1 restart=3D0
[   35.870109] writeccr f0
[   35.870272] I2C: SR=3Da2
[   35.873372] I2C: SR=3Da6
[   35.875757] mpc_xfer: 1 bytes to 2c:R - 2 of 2 messages
[   35.881606] mpc_read addr=3D2c len=3D1 restart=3D1
[   35.886290] writeccr f4
[   35.886463] I2C: SR=3Da6
[   35.889425] writeccr e8
[   35.889575] I2C: SR=3Da7
[   35.891944] writeccr 80
[   35.961177] mpc_xfer()
[   35.972517] writeccr 0
[   35.972541] writeccr 80
[   35.972559] mpc_xfer: 1 bytes to 4e:W - 1 of 20 messages
[   35.982628] mpc_write addr=3D4e len=3D1 restart=3D0
[   35.987389] writeccr f0
[   35.987424] I2C: SR=3Db3
[   35.990386] I2C: MAL
[   35.992971] i2c_wait(address) error: -5
[   35.997215] writeccr 80
[   35.997241] Error: i2c_transfer failed: -5

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.

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.

Best regards,
Esben Haabendal

--=20
Esben Haabendal, Senior Software Consultant
Dor=E9Development ApS, Ved Stranden 1, 9560 Hadsund, DK-Denmark
Phone: +45 51 92 53 93, E-mail: eha@doredevelopment.dk
WWW: http://www.doredevelopment.dk

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Joakim Tjernlund]

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

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

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Esben Haabendal]

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

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Joakim Tjernlund]

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

Re: [PATCH] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

> 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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]