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

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

[Wolfram Sang]

Support for enforced STOPs will allow us to use SCCB compatible devices.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Notes: I don't actually have any SCCB compatible sensor but verified with a
logic analyzer that repeated starts got replaced with a stop + start sequence.
However, we have members in our team who might need this feature soon. I'll
likely wait for their Tested-by unless something unforseen happens.

 drivers/i2c/algos/i2c-algo-bit.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
index a8e89df665b904..4758058352959d 100644
--- a/drivers/i2c/algos/i2c-algo-bit.c
+++ b/drivers/i2c/algos/i2c-algo-bit.c
@@ -549,12 +549,17 @@ static int bit_xfer(struct i2c_adapter *i2c_adap,
 	bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
 	i2c_start(adap);
 	for (i = 0; i < num; i++) {
+		bool did_stop = false;
+
 		pmsg = &msgs[i];
 		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 (did_stop) {
+				bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
+				i2c_start(adap);
+				did_stop = false;
+			} else if (i) {
+				bit_dbg(3, &i2c_adap->dev, "emitting repeated start condition\n");
 				i2c_repstart(adap);
 			}
 			ret = bit_doAddress(i2c_adap, pmsg);
@@ -588,6 +593,12 @@ static int bit_xfer(struct i2c_adapter *i2c_adap,
 				goto bailout;
 			}
 		}
+
+		if (pmsg->flags & I2C_M_STOP && i != num - 1) {
+			bit_dbg(3, &i2c_adap->dev, "emitting enforced stop condition\n");
+			i2c_stop(adap);
+			did_stop = true;
+		}
 	}
 	ret = i;
 
-- 
2.11.0

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

[Jean Delvare]

Hi Wolfram,

On Sat, 17 Jun 2017 19:12:38 +0200, Wolfram Sang wrote:
> Support for enforced STOPs will allow us to use SCCB compatible devices.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Notes: I don't actually have any SCCB compatible sensor but verified with a
> logic analyzer that repeated starts got replaced with a stop + start sequence.
> However, we have members in our team who might need this feature soon. I'll
> likely wait for their Tested-by unless something unforseen happens.
> 
>  drivers/i2c/algos/i2c-algo-bit.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
> index a8e89df665b904..4758058352959d 100644
> --- a/drivers/i2c/algos/i2c-algo-bit.c
> +++ b/drivers/i2c/algos/i2c-algo-bit.c
> @@ -549,12 +549,17 @@ static int bit_xfer(struct i2c_adapter *i2c_adap,
>  	bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
>  	i2c_start(adap);
>  	for (i = 0; i < num; i++) {
> +		bool did_stop = false;

I'm pretty certain you want to declare and initialize this variable
outside the loop.

> +
>  		pmsg = &msgs[i];
>  		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 (did_stop) {
> +				bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
> +				i2c_start(adap);
> +				did_stop = false;
> +			} else if (i) {
> +				bit_dbg(3, &i2c_adap->dev, "emitting repeated start condition\n");
>  				i2c_repstart(adap);
>  			}
>  			ret = bit_doAddress(i2c_adap, pmsg);
> @@ -588,6 +593,12 @@ static int bit_xfer(struct i2c_adapter *i2c_adap,
>  				goto bailout;
>  			}
>  		}
> +
> +		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.

> +			bit_dbg(3, &i2c_adap->dev, "emitting enforced stop condition\n");
> +			i2c_stop(adap);
> +			did_stop = true;
> +		}
>  	}
>  	ret = i;
>  

I have 2 questions:

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?

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.

What about something like that instead? Or I am missing something?

--- 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);


-- 
Jean Delvare
SUSE L3 Support

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

[Wolfram Sang]

[-- 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 --]

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

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

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

[Wolfram Sang]

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

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


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

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

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

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

[Wolfram Sang]

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


> 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!


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