[PATCH 0/2] Short timeout for I2C transfers

[PATCH 0/2] Short timeout for I2C transfers

[Samu Onkalo]

There are chips which doesn't work perfectly in certain
I2C operations. For example lp5523 led driver chip causes
always I2C timeout when SW reset is commanded to the chip.

Patches add a possibility to tell that this access is likely
to cause time out and there is no need to wait normal time.
For example in omap time out is 1 second and the bus is reserved
all the time.
By setting I2C_M_SHORT_TIMEOUT flag to i2c-message, adapter is
requested to use shorter timeout.

Samu Onkalo (2):
  drivers: i2c-core: Add a flag to allow short timeout
  drivers: i2c-omap: Add support for shorten I2C timeout

 drivers/i2c/busses/i2c-omap.c |    9 ++++++++-
 drivers/i2c/i2c-core.c        |    2 +-
 include/linux/i2c.h           |    1 +
 3 files changed, 10 insertions(+), 2 deletions(-)

[PATCH 1/2] drivers: i2c-core: Add a flag to allow short timeout

[Samu Onkalo]

A flag for i2c-messages to allow short time out. Some chips causes
time out always with certain kind of operations (like sw-reset commands).
There is no point of waiting full timeout in that case.
When I2C message is send with I2C_M_SHORT_TIME out flag, adapter is allowed
to use shorter timeout and release the bus faster.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/i2c-core.c |    2 +-
 include/linux/i2c.h    |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6649176..8c0e443 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2077,7 +2077,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 	int try;
 	s32 res;
 
-	flags &= I2C_M_TEN | I2C_CLIENT_PEC;
+	flags &= I2C_M_TEN | I2C_CLIENT_PEC | I2C_M_SHORT_TIMEOUT;
 
 	if (adapter->algo->smbus_xfer) {
 		i2c_lock_adapter(adapter);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 4bae0b7..5fc016a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -514,6 +514,7 @@ struct i2c_msg {
 #define I2C_M_IGNORE_NAK	0x1000	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_NO_RD_ACK		0x0800	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_RECV_LEN		0x0400	/* length will be first received byte */
+#define I2C_M_SHORT_TIMEOUT	0x0200	/* Use with known timeout cases */
 	__u16 len;		/* msg length				*/
 	__u8 *buf;		/* pointer to msg data			*/
 };
-- 
1.6.0.4

[PATCH 2/2] drivers: i2c-omap: Add support for shorten I2C timeout

[Samu Onkalo]

Use short I2C timeout if requested by i2c-message flag.
This is for cases where the it is known that target chip causes timeout.

Signed-off-by: Samu Onkalo <samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b33c785..d7ed7ca 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -50,6 +50,7 @@
 
 /* timeout waiting for the controller to respond */
 #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
+#define OMAP_I2C_SHORT_TIMEOUT (msecs_to_jiffies(50))
 
 /* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
 enum {
@@ -548,6 +549,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 			     struct i2c_msg *msg, int stop)
 {
 	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
+	unsigned long timeout;
 	int r;
 	u16 w;
 
@@ -618,8 +620,13 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	 */
 	if (dev->set_mpu_wkup_lat != NULL)
 		dev->set_mpu_wkup_lat(dev->dev, dev->latency);
+	if (unlikely((msg->flags & I2C_M_SHORT_TIMEOUT)))
+		timeout = OMAP_I2C_SHORT_TIMEOUT;
+	else
+		timeout = OMAP_I2C_TIMEOUT;
+
 	r = wait_for_completion_timeout(&dev->cmd_complete,
-					OMAP_I2C_TIMEOUT);
+					timeout);
 	if (dev->set_mpu_wkup_lat != NULL)
 		dev->set_mpu_wkup_lat(dev->dev, -1);
 	dev->buf_len = 0;
-- 
1.6.0.4

Re: [PATCH 0/2] Short timeout for I2C transfers

[Jean Delvare]

Hi Samu,

On Wed, 13 Oct 2010 16:45:08 +0300, Samu Onkalo wrote:
> There are chips which doesn't work perfectly in certain
> I2C operations. For example lp5523 led driver chip causes
> always I2C timeout when SW reset is commanded to the chip.

The timeout is a symptom as the controller level. What is the root
cause at the LP5523 device level? What exactly happens on the wire?

I am asking because maybe some of the already available protocol
mangling flags would work for you, in particular I2C_M_IGNORE_NAK. See
Documentation/i2c/i2c-protocol.

> Patches add a possibility to tell that this access is likely
> to cause time out and there is no need to wait normal time.
> For example in omap time out is 1 second and the bus is reserved
> all the time.

You should ask yourself whether a 1 second timeout is a sane thing to
have in the first place.

You should also check why you hit the timeout condition. Ideally the
hardware would report problems as they happen, quickly, rather than
having to rely on the driver's timeout mechanism. The timeout should
really only be a safety mechanism, for when the bus controller itself
misbehaves at the hardware level.

> By setting I2C_M_SHORT_TIMEOUT flag to i2c-message, adapter is
> requested to use shorter timeout.
> 
> Samu Onkalo (2):
>   drivers: i2c-core: Add a flag to allow short timeout
>   drivers: i2c-omap: Add support for shorten I2C timeout
> 
>  drivers/i2c/busses/i2c-omap.c |    9 ++++++++-
>  drivers/i2c/i2c-core.c        |    2 +-
>  include/linux/i2c.h           |    1 +
>  3 files changed, 10 insertions(+), 2 deletions(-)

I'm skeptical. If you know that the transfer will "fail" and you don't
care, why do you want to wait a short time, rather than not waiting at
all?

-- 
Jean Delvare

RE: [PATCH 0/2] Short timeout for I2C transfers

[samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w]

>-----Original Message-----
>From: ext Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
>Sent: 15 October, 2010 15:52
>To: Onkalo Samu.P (Nokia-MS/Tampere)
>Cc: tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Subject: Re: [PATCH 0/2] Short timeout for I2C transfers
>
>Hi Samu,
>
>On Wed, 13 Oct 2010 16:45:08 +0300, Samu Onkalo wrote:
>> There are chips which doesn't work perfectly in certain
>> I2C operations. For example lp5523 led driver chip causes
>> always I2C timeout when SW reset is commanded to the chip.
>
>The timeout is a symptom as the controller level. What is the root
>cause at the LP5523 device level? What exactly happens on the wire?
>

Chip seems to release ACK signal too soon causing rising edge while
SCL is high. This violates bus protocol and causes timeout at the
controller. It is no recognized as ACK nor NACK.

>I am asking because maybe some of the already available protocol
>mangling flags would work for you, in particular I2C_M_IGNORE_NAK. See
>Documentation/i2c/i2c-protocol.
>
>> Patches add a possibility to tell that this access is likely
>> to cause time out and there is no need to wait normal time.
>> For example in omap time out is 1 second and the bus is reserved
>> all the time.
>
>You should ask yourself whether a 1 second timeout is a sane thing to
>have in the first place.

hmm.. 1 second is quite long time out for i2c.

>
>You should also check why you hit the timeout condition. Ideally the
>hardware would report problems as they happen, quickly, rather than
>having to rely on the driver's timeout mechanism. The timeout should
>really only be a safety mechanism, for when the bus controller itself
>misbehaves at the hardware level.
>
>> By setting I2C_M_SHORT_TIMEOUT flag to i2c-message, adapter is
>> requested to use shorter timeout.
>>
>> Samu Onkalo (2):
>>   drivers: i2c-core: Add a flag to allow short timeout
>>   drivers: i2c-omap: Add support for shorten I2C timeout
>>
>>  drivers/i2c/busses/i2c-omap.c |    9 ++++++++-
>>  drivers/i2c/i2c-core.c        |    2 +-
>>  include/linux/i2c.h           |    1 +
>>  3 files changed, 10 insertions(+), 2 deletions(-)
>
>I'm skeptical. If you know that the transfer will "fail" and you don't
>care, why do you want to wait a short time, rather than not waiting at
>all?

Well, at least some time should be spend to allow bits to go out.
Maybe controller provides something for that.

-Samu

RE: [PATCH 0/2] Short timeout for I2C transfers

[samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w]

>-----Original Message-----
>From: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-i2c-
>owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of ext samu.p.onkalo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org


>>The timeout is a symptom as the controller level. What is the root
>>cause at the LP5523 device level? What exactly happens on the wire?
>>
>
>Chip seems to release ACK signal too soon causing rising edge while
>SCL is high. This violates bus protocol and causes timeout at the
>controller. It is no recognized as ACK nor NACK.
>
>>I am asking because maybe some of the already available protocol
>>mangling flags would work for you, in particular I2C_M_IGNORE_NAK. See
>>Documentation/i2c/i2c-protocol.
>>
>>
>>You should also check why you hit the timeout condition. Ideally the
>>hardware would report problems as they happen, quickly, rather than
>>having to rely on the driver's timeout mechanism. The timeout should
>>really only be a safety mechanism, for when the bus controller itself
>>misbehaves at the hardware level.
>>

It seems that controller detects bus free condition when this happens.
So, instead of the timeout, bus free condition should be detected.
There is no such interrupt so this must be done by polling
the status register. In general this is not a good idea.

Would it make sense somehow tell that use polling and not
the interrupt / timeout control? I.e. by setting a flag that
this transfer doesn't end properly.

I2C_M_IGNORE_TIMEOUT?
In such case just either wait for bus free if supported by HW or
wait for some short time after start of the transmission

-Samu