| Message ID | 20170619103002.5d502c92@endymion |
|---|---|
| State | Changes Requested |
| Headers | show |
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
Hi Wolfram, On Tue, 20 Jun 2017 19:28:10 +0200, Wolfram Sang wrote: > 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? There's no problem with the scope. The problem is with the initialization. The way you did, did_stop gets reset to false with every iteration, which isn't what you want. > > (...) > > 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. I looked at how 10-bit addressing works again and in fact it is simply impossible to not use repeated start when reading from a 10-bit address slave. Only the first 2 bits of the 10-bit address are repeated in the read part of the transaction. If there was a stop between the two parts then there would be no way to know which 10-bit slave should send the data. > > (...) > > --- 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 Oops. Right, fixed, thanks. > > > + > > + 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. I'll send a proper patch shortly. > Thanks for the improvement! You're welcome.
Hi Jean, > There's no problem with the scope. The problem is with the > initialization. The way you did, did_stop gets reset to false with > every iteration, which isn't what you want. Ouch, where is my brown paper bag... > I looked at how 10-bit addressing works again and in fact it is simply > impossible to not use repeated start when reading from a 10-bit address > slave. Only the first 2 bits of the 10-bit address are repeated in the > read part of the transaction. If there was a stop between the two parts > then there would be no way to know which 10-bit slave should send the > data. Which reminds me: Have you ever seen a 10-bit client device in the wild? I wanted to buy one for testing reasons but was not able to locate one. I only know a Renesas IP core which has 10-bit slave capability (but no driver support for that yet). Regards, Wolfram
On Wed, 21 Jun 2017 16:30:18 +0200, Wolfram Sang wrote: > Which reminds me: Have you ever seen a 10-bit client device in the wild? > I wanted to buy one for testing reasons but was not able to locate one. > I only know a Renesas IP core which has 10-bit slave capability (but no > driver support for that yet). I remember wishing I could drop support, asking around, and a few people replying to me that 10-bit slaves actually exist. But of course I can't find the discussion thread again. I can see one driver for a 10-bit address I2C device: drivers/media/i2c/tw2804.c (device instantiated from drivers/media/usb/go7007/go7007-usb.c.) However it seems that in many cases I2C_M_TEN is used directly (instead of the more correct I2C_CLIENT_TEN.) See for example drivers/input/touchscreen/atmel_mxt_ts.c, drivers/input/touchscreen/elants_i2c.c, drivers/input/touchscreen/mms114.c, drivers/media/pci/cx23885/cx23885-i2c.c, drivers/media/pci/cx25821/cx25821-i2c.c, which are clearly talking to 10-bit I2C devices, but without instantiating an i2c_client for them. I see also commits explicitly adding or fixing 10-bit address support in various I2C bus drivers, I don't think developers would be doing that if they didn't need it.
> I remember wishing I could drop support, asking around, and a few > people replying to me that 10-bit slaves actually exist. But of course > I can't find the discussion thread again. Dropping 10-bit support would be super nice from a maintenance point of view, but I didn't have hopes for that to happen ;) This is why I wanted to buy a device to be able to test. > However it seems that in many cases I2C_M_TEN is used directly (instead > of the more correct I2C_CLIENT_TEN.) See for example Yeah, good idea to scan for I2C_CLIENT_TEN directly. From a glimpse, I didn't see a device which I could easily buy, connect, and sniff the wires. But it is not urgent anyhow. > I see also commits explicitly adding or fixing 10-bit address support > in various I2C bus drivers, I don't think developers would be doing > that if they didn't need it. Agreed. It will stay, I had no intention of removing it. Thanks for the help!
--- 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); + } + + bit_dbg(3, &i2c_adap->dev, + "emitting repeated start condition\n"); i2c_repstart(adap); } ret = bit_doAddress(i2c_adap, pmsg);