| Message ID | 4A124202.4010201@doredevelopment.dk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Kumar Gala |
| Headers | show |
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? /Esben
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
> > 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 .
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>
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> >> --- >> 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. g.
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
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? /Esben
>>>>> "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.
> 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. It's strange, that line looks perfectly fine when I check the mail in my GMail inbox and the outbox from the account I sent it from. /Esben
>>>>> "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.
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
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?
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> > --- > 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); > } 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 = 0; for (i = 0; ret >= 0 && i < num; 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 (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); + /* 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 = (pmsg->flags & I2C_M_RD == 0); } g.
On Tue, Jun 2, 2009 at 5:12 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > 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> >> --- >> 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); >> } > > 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 = 0; > for (i = 0; ret >= 0 && i < num; 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 (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); > + /* 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 = (pmsg->flags & I2C_M_RD == 0); > } 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 = " to be on a separate line anymore. g.
is there a new version of this patch available?
Ben Dooks wrote: > is there a new version of this patch available? > > I will catch up on it ASAP. /Esben
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.
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 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
> > 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.
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.
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);
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(-) return (ret < 0) ? ret : num;