Hi Jean,
> > + bool did_stop = false;
>
> I'm pretty certain you want to declare and initialize this variable
> outside the loop.
Why? Well, it doesn't matter anymore but what is wrong with limiting the
variable like this?
> > + if (pmsg->flags & I2C_M_STOP && i != num - 1) {
>
> I recommend adding parentheses around the bit matching when combined
> with &&. I know it is not strictly needed and the compiler doesn't
> care, but I find it easier to read, and there seems to be a consensus
> (90 %) on that in the kernel tree.
Either both in parens, or none ;) But doesn't matter as well anymore.
> 1* Repeated start happens between messages of a same transaction, and
> you handle that case above. However in the case of 10-bit address
> clients, there is also a repeated start happening during the address
> phase of the transaction, if the first message is a read. Did you check
> what the SCCB protocol expects in that case?
SCCB defines addresses to be 7 bit.
> 2* I'm not sure why you add the enforced stop at the end of one
> iteration and the start at the beginning of the next iteration. It
> would be more simple and efficient to do both at the beginning of the
> next iteration. The only case where it would make a difference is if
> both I2C_M_NOSTART and I2C_M_STOP are specified. In this case you
> currently emit a stop condition but no start, which I don't think can
> work at all.
Ehrm, probably I was just too tied to the ordering of "first start -
then data - then stop - then next loop" :)
> What about something like that instead? Or I am missing something?
No, I did miss it.
> --- linux-4.11.orig/drivers/i2c/algos/i2c-algo-bit.c 2017-06-19 09:57:17.949074198 +0200
> +++ linux-4.11/drivers/i2c/algos/i2c-algo-bit.c 2017-06-19 10:23:26.711088536 +0200
> @@ -553,8 +553,17 @@ static int bit_xfer(struct i2c_adapter *
> nak_ok = pmsg->flags & I2C_M_IGNORE_NAK;
> if (!(pmsg->flags & I2C_M_NOSTART)) {
> if (i) {
> - bit_dbg(3, &i2c_adap->dev, "emitting "
> - "repeated start condition\n");
> + if (msgs[i - 1].flags & I2C_M_STOP) {
> + bit_dbg(3, &i2c_adap->dev,
> + "emitting enforced stop condition\n");
> + i2c_stop(adap);
> + bit_dbg(3, &i2c_adap->dev,
> + "emitting start condition\n");
> + i2c_start(adap);
> + }
else
> +
> + bit_dbg(3, &i2c_adap->dev,
> + "emitting repeated start condition\n");
> i2c_repstart(adap);
> }
> ret = bit_doAddress(i2c_adap, pmsg);
A lot better! I like it very much. And also
Tested-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
Do you want to cook up a patch or shall I? I'd just need a SoB then.
Thanks for the improvement!
Wolfram
Attachment:
signature.asc
Description: PGP signature
