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

[PATCH v2] 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. 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 <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..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, 
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.3.1

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

[Esben Haabendal]

On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> w=
rote:
> 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 <eha@doredevelopment.dk>
> ---
> =A0drivers/i2c/busses/i2c-mpc.c | =A0 =A09 +++++++--
> =A01 files changed, 7 insertions(+), 2 deletions(-)

Any blockers to get this accepted?

/Esben

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

[Ben Dooks]

On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> > 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 <eha@doredevelopment.dk>
> > ---
> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> > ?1 files changed, 7 insertions(+), 2 deletions(-)
> 
> Any blockers to get this accepted?

It would be nice to get an ack from someone who can actually test
the driver before getting this merged.
 
> /Esben
> --
> 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

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

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

[Wolfram Sang]

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

> > Any blockers to get this accepted?
> 
> It would be nice to get an ack from someone who can actually test
> the driver before getting this merged.

I wanted to test it, but it does not apply due to line breaks (check
@@-line). Also, I don't really have the time to dig into the topic, so I
would only test it and give a tested-by-tag if it doesn't break anything
here. I think Joakim would be a good candidate for an acked-by .

-- 
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 v2] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Joakim Tjernlund]

Wolfram Sang <w.sang@pengutronix.de> wrote on 28/05/2009 19:17:26:
>
> > > Any blockers to get this accepted?
> >
> > It would be nice to get an ack from someone who can actually test
> > the driver before getting this merged.
>
> I wanted to test it, but it does not apply due to line breaks (check
> @@-line). Also, I don't really have the time to dig into the topic, so I
> would only test it and give a tested-by-tag if it doesn't break anything
> here. I think Joakim would be a good candidate for an acked-by .

It sure looks OK, even at a closer look :)
Acked-by:  Joakim Tjernlund <Joakim.Tjernlund@transmode.se>

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

[Esben Haabendal]

On Thu, May 28, 2009 at 7:17 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>> > Any blockers to get this accepted?
>>
>> It would be nice to get an ack from someone who can actually test
>> the driver before getting this merged.
>
> I wanted to test it, but it does not apply due to line breaks (check
> @@-line). Also, I don't really have the time to dig into the topic, so I
> would only test it and give a tested-by-tag if it doesn't break anything
> here. I think Joakim would be a good candidate for an acked-by .

I've checked both my copy in my "Sent" folder and the copy received
from the list,
and I cannot see any "line break" breakage of the patch.

It must be your mail client that is messing with it, so I cannot really do it
much better in that way.

If necessary, I can push a branch to a public repo you can pull from.

/Esben

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

[Peter Korsgaard]

>>>>> "Esben" == Esben Haabendal <esbenhaabendal@gmail.com> writes:

Hi,

 >> I wanted to test it, but it does not apply due to line breaks (check
 >> @@-line). Also, I don't really have the time to dig into the topic, so I
 >> would only test it and give a tested-by-tag if it doesn't break anything
 >> here. I think Joakim would be a good candidate for an acked-by .

 Esben> I've checked both my copy in my "Sent" folder and the copy
 Esben> received from the list, and I cannot see any "line break"
 Esben> breakage of the patch.

I guess Wolfram referred to the context line which was clearly word wrapped:

@@ -456,17 +456,22 @@ static int mpc_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)

The other lines look fine.

-- 
Bye, Peter Korsgaard

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

[Esben Haabendal]

> =A0Esben> I've checked both my copy in my "Sent" folder and the copy
> =A0Esben> received from the list, and I cannot see any "line break"
> =A0Esben> breakage of the patch.
>
> I guess Wolfram referred to the context line which was clearly word wrapp=
ed:
>
> @@ -456,17 +456,22 @@ static int mpc_xfer(struct i2c_adapter *adap,
> struct i2c_msg *msgs, int num)
>
> The other lines look fine.

It's strange, that line looks perfectly fine when I check the mail in my GM=
ail
inbox and the outbox from the account I sent it from.

/Esben
--=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 v2] i2c-mpc: generate START condition after STOP caused by read i2c_msg

[Peter Korsgaard]

>>>>> "Esben" == Esben Haabendal <esbenhaabendal@gmail.com> writes:

Hi,

 Esben> It's strange, that line looks perfectly fine when I check the
 Esben> mail in my GMail inbox and the outbox from the account I sent
 Esben> it from.

Well, it is here and in the archive:
http://ozlabs.org/pipermail/linuxppc-dev/2009-May/072274.html

Please consider using git send-email for patches.

-- 
Bye, Peter Korsgaard

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

[Esben Haabendal]

Peter Korsgaard wrote:
>>>>>> "Esben" == Esben Haabendal <esbenhaabendal@gmail.com> writes:
>>>>>>             
>
> Hi,
>
>  Esben> It's strange, that line looks perfectly fine when I check the
>  Esben> mail in my GMail inbox and the outbox from the account I sent
>  Esben> it from.
>
> Well, it is here and in the archive:
> http://ozlabs.org/pipermail/linuxppc-dev/2009-May/072274.html
>   
If you look at
http://www.mail-archive.com/linux-i2c@vger.kernel.org/msg01050.html
instead, the patch is not broken.  I find it more likely that the ozlabs.org
archive is breaking my lines, than mail-archive.com putting broken lines
together again.
> Please consider using git send-email for patches.
>   
I used git imap-send and thunderbird to do it.

I will consider git send-email in the future, although I don't think
my original e-mail were broken ;-)

But

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

[Michael Lawnick]

Ben Dooks said the following:
> On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
>> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
>> > 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 <eha@doredevelopment.dk>
>> > ---
>> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
>> > ?1 files changed, 7 insertions(+), 2 deletions(-)
>> 
>> Any blockers to get this accepted?
> 
> It would be nice to get an ack from someone who can actually test
> the driver before getting this merged.
>  
What is the state of this patch?
Shouldn't we attack the problem on a more general way by inventing a
Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
This way the client driver is able to decide what it needs. If we do the
choice within adapter, chance is about 50% to be wrong.

Just my 2 Cents.

-- 
Michael

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

[Ben Dooks]

On Thu, Dec 03, 2009 at 04:09:57PM +0100, Michael Lawnick wrote:
> Ben Dooks said the following:
> > On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
> >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> >> > 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 <eha@doredevelopment.dk>
> >> > ---
> >> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> >> > ?1 files changed, 7 insertions(+), 2 deletions(-)
> >> 
> >> Any blockers to get this accepted?
> > 
> > It would be nice to get an ack from someone who can actually test
> > the driver before getting this merged.
> >  
> What is the state of this patch?
> Shouldn't we attack the problem on a more general way by inventing a
> Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
> This way the client driver is able to decide what it needs. If we do the
> choice within adapter, chance is about 50% to be wrong.

The documentation for 'struct i2c_msg' already says the STOP should only
be generated for the last message of the transfer. If STOP is being
generated for a message that isn't the last in the transfer than this
is incorrect behaviour.

Unless otherwise indicated, I'll put the patch in the series that I'll
send to Linus early next week.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

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

[Joakim Tjernlund]

>
> On Thu, Dec 03, 2009 at 04:09:57PM +0100, Michael Lawnick wrote:
> > Ben Dooks said the following:
> > > On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
> > >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> > >> > 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 <eha@doredevelopment.dk>
> > >> > ---
> > >> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> > >> > ?1 files changed, 7 insertions(+), 2 deletions(-)
> > >>
> > >> Any blockers to get this accepted?
> > >
> > > It would be nice to get an ack from someone who can actually test
> > > the driver before getting this merged.
> > >
> > What is the state of this patch?
> > Shouldn't we attack the problem on a more general way by inventing a
> > Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
> > This way the client driver is able to decide what it needs. If we do the
> > choice within adapter, chance is about 50% to be wrong.
>
> The documentation for 'struct i2c_msg' already says the STOP should only
> be generated for the last message of the transfer. If STOP is being
> generated for a message that isn't the last in the transfer than this
> is incorrect behaviour.
>
> Unless otherwise indicated, I'll put the patch in the series that I'll
> send to Linus early next week.

Don't, already fixed properly and in Linus tree already.

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

[Michael Lawnick]

Ben Dooks said the following:
> On Thu, Dec 03, 2009 at 04:09:57PM +0100, Michael Lawnick wrote:
>> Ben Dooks said the following:
>> > On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
>> >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
>> >> > 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 <eha@doredevelopment.dk>
>> >> > ---
>> >> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
>> >> > ?1 files changed, 7 insertions(+), 2 deletions(-)
>> >> 
>> >> Any blockers to get this accepted?
>> > 
>> > It would be nice to get an ack from someone who can actually test
>> > the driver before getting this merged.
>> >  
>> What is the state of this patch?
>> Shouldn't we attack the problem on a more general way by inventing a
>> Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
>> This way the client driver is able to decide what it needs. If we do the
>> choice within adapter, chance is about 50% to be wrong.
> 
> The documentation for 'struct i2c_msg' already says the STOP should only
> be generated for the last message of the transfer. If STOP is being
> generated for a message that isn't the last in the transfer than this
> is incorrect behaviour.

Ah, now I see, this is a mpc-only problem of implementation
(automatically generating stop in read function).
I couldn't find the above mentioned specification of STOP/RESTART (seems
I need new glasses) and was worried about whether my own implementation
for OCTEON is correct.

Thank you.
-- 
Michael

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

[Joakim Tjernlund]

>
> Ben Dooks said the following:
> > On Tue, May 26, 2009 at 01:30:21PM +0200, Esben Haabendal wrote:
> >> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk> wrote:
> >> > 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 <eha@doredevelopment.dk>
> >> > ---
> >> > ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> >> > ?1 files changed, 7 insertions(+), 2 deletions(-)
> >>
> >> Any blockers to get this accepted?
> >
> > It would be nice to get an ack from someone who can actually test
> > the driver before getting this merged.
> >
> What is the state of this patch?
> Shouldn't we attack the problem on a more general way by inventing a
> Flag I2C_M_RESTART (or better I2C_M_NO_RESTART for backward compatibility)?
> This way the client driver is able to decide what it needs. If we do the
> choice within adapter, chance is about 50% to be wrong.

I have sent another patch (which I think is in Linus tree already) that
fixes the problem properly.

Can't remember if the driver handles M_RESTART or not. That is
a separate issue though.

      Jocke

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

[Grant Likely]

On Tue, May 26, 2009 at 5:30 AM, Esben Haabendal
<esbenhaabendal@gmail.com> wrote:
> On Tue, May 19, 2009 at 7:22 AM, Esben Haabendal <eha@doredevelopment.dk>=
 wrote:
>> 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 <eha@doredevelopment.dk>
>> ---
>> =A0drivers/i2c/busses/i2c-mpc.c | =A0 =A09 +++++++--
>> =A01 files changed, 7 insertions(+), 2 deletions(-)
>
> Any blockers to get this accepted?

It helps if you cc: developers/maintainers of the device.  ie. Kumar
for mpc8xxx, me for 52xx.

This is the first time I noticed your posting.  It will take me a few
days before I get a chance to review it.

g.

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

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

[Esben Haabendal]

On Thu, May 28, 2009 at 9:31 PM, Grant Likely <grant.likely@secretlab.ca> w=
rote:
> On Tue, May 26, 2009 at 5:30 AM, Esben Haabendal wrote:
>> On Tue, May 19, 2009 at 7:22 AM, 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. 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 <eha@doredevelopment.dk>
>>> ---
>>> =A0drivers/i2c/busses/i2c-mpc.c | =A0 =A09 +++++++--
>>> =A01 files changed, 7 insertions(+), 2 deletions(-)
>>
>> Any blockers to get this accepted?
>
> It helps if you cc: developers/maintainers of the device. =A0ie. Kumar
> for mpc8xxx, me for 52xx.
>
> This is the first time I noticed your posting. =A0It will take me a few
> days before I get a chance to review it.

Kumar, will you take a look at this patch?

/Esben

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

[Ben Dooks]

On Thu, May 28, 2009 at 10:15:22PM +0200, Esben Haabendal wrote:
> On Thu, May 28, 2009 at 9:31 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Tue, May 26, 2009 at 5:30 AM, Esben Haabendal wrote:
> >> On Tue, May 19, 2009 at 7:22 AM, 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. 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 <eha@doredevelopment.dk>
> >>> ---
> >>> ?drivers/i2c/busses/i2c-mpc.c | ? ?9 +++++++--
> >>> ?1 files changed, 7 insertions(+), 2 deletions(-)
> >>
> >> Any blockers to get this accepted?
> >
> > It helps if you cc: developers/maintainers of the device. ?ie. Kumar
> > for mpc8xxx, me for 52xx.
> >
> > This is the first time I noticed your posting. ?It will take me a few
> > days before I get a chance to review it.
> 
> Kumar, will you take a look at this patch?

is anyone else likely to review it, or should I merge?

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

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

[Grant Likely]

Hi Esben and Ben,

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

g.

On Mon, May 18, 2009 at 11:22 PM, Esben Haabendal
<eha@doredevelopment.dk> wrote:
> 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 <eha@doredevelopment.dk>
> ---
> =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, struc=
t
> 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%0=
2x - %d of %d messages\n",
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pmsg->flags & I2C_M_RD ? "=
read" : "write",
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pmsg->len, pmsg->addr, i +=
 1, num);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i > 0 && ((pmsg - 1)->flags & I2C_M_RD)=
)
> + =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,
> 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 =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, 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}

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

+      int restart =3D 0;
       for (i =3D 0; ret >=3D 0 && i < num; i++) {
               pmsg =3D &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 (pmsg->flags & I2C_M_RD)
                       ret =3D
-                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
i);
+                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len,
+                                    restart);
               else
                       ret =3D
-                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len=
,
i);
+                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len=
,
+                                     restart);
+               /* Only set the restart flag if this was not an
I2C_M_RD transaction
+                * because it causes an illegal use of
+                * RSTA (repeated START) after STOP condition
generated after last byte
+                * of reads  */
+               restart =3D (pmsg->flags & I2C_M_RD =3D=3D 0);
       }

g.

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

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

[Grant Likely]

On Tue, Jun 2, 2009 at 5:12 PM, Grant Likely <grant.likely@secretlab.ca> 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
> <eha@doredevelopment.dk> wrote:
>> 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 <eha@doredevelopment.dk>
>> ---
>> =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
>> 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",
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pmsg->flags & I2C_M_RD ? =
"read" : "write",
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pmsg->len, pmsg->addr, i =
+ 1, num);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (i > 0 && ((pmsg - 1)->flags & I2C_M_RD=
))
>> + =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,
>> 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 =
=A0restart);
>> =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,
>> i);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc_write(i2c, pms=
g->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}
>
> 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.

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

[Ben Dooks]

is there a new version of this patch available?

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

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

[Esben Haabendal]

Ben Dooks wrote:
> is there a new version of this patch available?
>
>   
I will catch up on it ASAP.

/Esben