Re: [PATCH] i2c: algo-bit: add support for I2C_M_STOP

[Wolfram Sang <wsa@the-dreams.de>]

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

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@sang-engineering.com>

Do you want to cook up a patch or shall I? I'd just need a SoB then.

Thanks for the improvement!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]