[PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Jean Delvare]

As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.

In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Douglas Gilbert <dgilbert-qazKcTl6WRFWk0Htik3J/w@public.gmane.org>
---
This is an alternative to Doug's implementation. Doug, I sent this to
you one week ago, did you have the time to give it a try, do you have
any comment? Again I can't test this myself so someone else will have
to do it.

 drivers/i2c/i2c-dev.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

--- linux-3.3-rc7.orig/drivers/i2c/i2c-dev.c	2012-03-15 18:00:02.502988008 +0100
+++ linux-3.3-rc7/drivers/i2c/i2c-dev.c	2012-03-15 18:04:46.394992216 +0100
@@ -265,19 +265,30 @@ static noinline int i2cdev_ioctl_rdrw(st
 
 	res = 0;
 	for (i = 0; i < rdwr_arg.nmsgs; i++) {
-		/* Limit the size of the message to a sane amount;
-		 * and don't let length change either. */
-		if ((rdwr_pa[i].len > 8192) ||
-		    (rdwr_pa[i].flags & I2C_M_RECV_LEN)) {
+		/* Limit the size of the message to a sane amount */
+		if (rdwr_pa[i].len > 8192) {
 			res = -EINVAL;
 			break;
 		}
+
+		/* Use the same RECV_LEN semantics as SMBus */
+		if (rdwr_pa[i].flags & I2C_M_RECV_LEN) {
+			if (rdwr_pa[i].len < I2C_SMBUS_BLOCK_MAX + 1) {
+				res = -EINVAL;
+				break;
+			}
+		}
+
 		data_ptrs[i] = (u8 __user *)rdwr_pa[i].buf;
 		rdwr_pa[i].buf = memdup_user(data_ptrs[i], rdwr_pa[i].len);
 		if (IS_ERR(rdwr_pa[i].buf)) {
 			res = PTR_ERR(rdwr_pa[i].buf);
 			break;
 		}
+
+		/* Bus driver will add the value of the first byte received */
+		if (rdwr_pa[i].flags & I2C_M_RECV_LEN)
+			rdwr_pa[i].len -= I2C_SMBUS_BLOCK_MAX;
 	}
 	if (res < 0) {
 		int j;


-- 
Jean Delvare

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Jean Delvare]

On Thu, 15 Mar 2012 18:08:35 +0100, Jean Delvare wrote:
> As the bus driver side implementation of I2C_M_RECV_LEN is heavily
> tied to SMBus, we can't support received length over 32 bytes, but
> let's at least support that.
> 
> In practice, the caller will have to setup a buffer large enough to
> cover the case where received length byte has value 32, so minimum
> 32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
> added for the specific slave (for example a checksum.)
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Douglas Gilbert <dgilbert-qazKcTl6WRFWk0Htik3J/w@public.gmane.org>
> ---
> This is an alternative to Doug's implementation. Doug, I sent this to
> you one week ago, did you have the time to give it a try, do you have
> any comment? Again I can't test this myself so someone else will have
> to do it.

Douglas, please?

-- 
Jean Delvare

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Douglas Gilbert]

On 12-03-31 02:19 AM, Jean Delvare wrote:
> On Thu, 15 Mar 2012 18:08:35 +0100, Jean Delvare wrote:
>> As the bus driver side implementation of I2C_M_RECV_LEN is heavily
>> tied to SMBus, we can't support received length over 32 bytes, but
>> let's at least support that.
>>
>> In practice, the caller will have to setup a buffer large enough to
>> cover the case where received length byte has value 32, so minimum
>> 32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
>> added for the specific slave (for example a checksum.)
>>
>> Signed-off-by: Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
>> Cc: Douglas Gilbert<dgilbert-qazKcTl6WRFWk0Htik3J/w@public.gmane.org>
>> ---
>> This is an alternative to Doug's implementation. Doug, I sent this to
>> you one week ago, did you have the time to give it a try, do you have
>> any comment? Again I can't test this myself so someone else will have
>> to do it.
>
> Douglas, please?

Jean,
Sorry about the delay in responding. The patch didn't work
in the case of the Sonmicro SM130 RFID but I could see it was close.

The correct response (for the SM130) when reading the firmware
version is this 10 byte response:
   08 81 49 32 43 20 32 2e 38 ff     ["I2C 2.8"]
so the count in the first byte excludes itself and the checksum
trailing byte. With the I2C_M_RECV_LEN patch I see this response:
   08 81 49 32 43 20 32 2e 00 00
so the count (leading) byte includes itself and makes no
provision for a checksum. [So 8 bytes are returned and the two
trailing zeros are just buffer pre-fill.]

You might argue that the I2C_M_RECV_LEN patch is sensible
and the SM130 is strange. My solution is to read 32 bytes
which is more than I ever expect.

Doug Gilbert

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Jean Delvare]

Hi Douglas,

On Wed, 04 Apr 2012 18:54:20 -0400, Douglas Gilbert wrote:
> Sorry about the delay in responding. The patch didn't work
> in the case of the Sonmicro SM130 RFID but I could see it was close.
> 
> The correct response (for the SM130) when reading the firmware
> version is this 10 byte response:
>    08 81 49 32 43 20 32 2e 38 ff     ["I2C 2.8"]
> so the count in the first byte excludes itself and the checksum
> trailing byte. With the I2C_M_RECV_LEN patch I see this response:
>    08 81 49 32 43 20 32 2e 00 00
> so the count (leading) byte includes itself and makes no
> provision for a checksum. [So 8 bytes are returned and the two
> trailing zeros are just buffer pre-fill.]

What value did you set msg->buf[0] to before calling? You were supposed
to set it to 2 in your case, exactly because the driver can't guess how
many extra bytes the chip will return, that aren't included in the byte
count. Your results suggest that you let msg->buf[0] to 0.

I've improved my patch to properly reject the transaction if buf[0] is
not set properly. Please test and report.

> You might argue that the I2C_M_RECV_LEN patch is sensible
> and the SM130 is strange. My solution is to read 32 bytes
> which is more than I ever expect.

The SM130 is a bit strange but it should be supportable.

* * * * *

As the bus driver side implementation of I2C_M_RECV_LEN is heavily
tied to SMBus, we can't support received length over 32 bytes, but
let's at least support that.

In practice, the caller will have to setup a buffer large enough to
cover the case where received length byte has value 32, so minimum
32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
added for the specific slave (for example a checksum.)

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Douglas Gilbert <dgilbert-qazKcTl6WRFWk0Htik3J/w@public.gmane.org>
---
 drivers/i2c/i2c-dev.c |   30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

--- linux-3.4-rc1.orig/drivers/i2c/i2c-dev.c	2012-04-02 17:16:53.000000000 +0200
+++ linux-3.4-rc1/drivers/i2c/i2c-dev.c	2012-04-05 09:12:26.385033151 +0200
@@ -265,19 +265,41 @@ static noinline int i2cdev_ioctl_rdrw(st
 
 	res = 0;
 	for (i = 0; i < rdwr_arg.nmsgs; i++) {
-		/* Limit the size of the message to a sane amount;
-		 * and don't let length change either. */
-		if ((rdwr_pa[i].len > 8192) ||
-		    (rdwr_pa[i].flags & I2C_M_RECV_LEN)) {
+		/* Limit the size of the message to a sane amount */
+		if (rdwr_pa[i].len > 8192) {
 			res = -EINVAL;
 			break;
 		}
+
 		data_ptrs[i] = (u8 __user *)rdwr_pa[i].buf;
 		rdwr_pa[i].buf = memdup_user(data_ptrs[i], rdwr_pa[i].len);
 		if (IS_ERR(rdwr_pa[i].buf)) {
 			res = PTR_ERR(rdwr_pa[i].buf);
 			break;
 		}
+
+		/*
+		 * If the message length is received from the slave (similar
+		 * to SMBus block read), we must ensure that the buffer will
+		 * be large enough to cope with a message length of
+		 * I2C_SMBUS_BLOCK_MAX as this is the maximum underlying bus
+		 * drivers allow. The first byte in the buffer must be
+		 * pre-filled with the number of extra bytes, which must be
+		 * at least one to hold the message length, but can be
+		 * greater (for example to account for a checksum byte at
+		 * the end of the message.)
+		 */
+		if (rdwr_pa[i].flags & I2C_M_RECV_LEN) {
+			if (!(rdwr_pa[i].flags & I2C_M_RD) ||
+			    rdwr_pa[i].buf[0] < 1 ||
+			    rdwr_pa[i].len < rdwr_pa[i].buf[0] +
+					     I2C_SMBUS_BLOCK_MAX) {
+				res = -EINVAL;
+				break;
+			}
+
+			rdwr_pa[i].len = rdwr_pa[i].buf[0];
+		}
 	}
 	if (res < 0) {
 		int j;


-- 
Jean Delvare

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Douglas Gilbert]

On 12-04-05 03:24 AM, Jean Delvare wrote:
> Hi Douglas,
>
> On Wed, 04 Apr 2012 18:54:20 -0400, Douglas Gilbert wrote:
>> Sorry about the delay in responding. The patch didn't work
>> in the case of the Sonmicro SM130 RFID but I could see it was close.
>>
>> The correct response (for the SM130) when reading the firmware
>> version is this 10 byte response:
>>     08 81 49 32 43 20 32 2e 38 ff     ["I2C 2.8"]
>> so the count in the first byte excludes itself and the checksum
>> trailing byte. With the I2C_M_RECV_LEN patch I see this response:
>>     08 81 49 32 43 20 32 2e 00 00
>> so the count (leading) byte includes itself and makes no
>> provision for a checksum. [So 8 bytes are returned and the two
>> trailing zeros are just buffer pre-fill.]
>
> What value did you set msg->buf[0] to before calling? You were supposed
> to set it to 2 in your case, exactly because the driver can't guess how
> many extra bytes the chip will return, that aren't included in the byte
> count. Your results suggest that you let msg->buf[0] to 0.
>
> I've improved my patch to properly reject the transaction if buf[0] is
> not set properly. Please test and report.
>
>> You might argue that the I2C_M_RECV_LEN patch is sensible
>> and the SM130 is strange. My solution is to read 32 bytes
>> which is more than I ever expect.
>
> The SM130 is a bit strange but it should be supportable.
>
> * * * * *
>
> As the bus driver side implementation of I2C_M_RECV_LEN is heavily
> tied to SMBus, we can't support received length over 32 bytes, but
> let's at least support that.
>
> In practice, the caller will have to setup a buffer large enough to
> cover the case where received length byte has value 32, so minimum
> 32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
> added for the specific slave (for example a checksum.)

Jean,
Either I am misunderstanding how to use this new patch or it is
broken. After replacing the original patch with this one, setting
msg->buf[0] to 2, my test program only sees the first two bytes
of expected data:
   08 81
That is down from 8 bytes from the previous patch and 10 bytes
expected from the SM130.

Doug Gilbert


> Signed-off-by: Jean Delvare<khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Douglas Gilbert<dgilbert-qazKcTl6WRFWk0Htik3J/w@public.gmane.org>
> ---
>   drivers/i2c/i2c-dev.c |   30 ++++++++++++++++++++++++++----
>   1 file changed, 26 insertions(+), 4 deletions(-)
>
> --- linux-3.4-rc1.orig/drivers/i2c/i2c-dev.c	2012-04-02 17:16:53.000000000 +0200
> +++ linux-3.4-rc1/drivers/i2c/i2c-dev.c	2012-04-05 09:12:26.385033151 +0200
> @@ -265,19 +265,41 @@ static noinline int i2cdev_ioctl_rdrw(st
>
>   	res = 0;
>   	for (i = 0; i<  rdwr_arg.nmsgs; i++) {
> -		/* Limit the size of the message to a sane amount;
> -		 * and don't let length change either. */
> -		if ((rdwr_pa[i].len>  8192) ||
> -		    (rdwr_pa[i].flags&  I2C_M_RECV_LEN)) {
> +		/* Limit the size of the message to a sane amount */
> +		if (rdwr_pa[i].len>  8192) {
>   			res = -EINVAL;
>   			break;
>   		}
> +
>   		data_ptrs[i] = (u8 __user *)rdwr_pa[i].buf;
>   		rdwr_pa[i].buf = memdup_user(data_ptrs[i], rdwr_pa[i].len);
>   		if (IS_ERR(rdwr_pa[i].buf)) {
>   			res = PTR_ERR(rdwr_pa[i].buf);
>   			break;
>   		}
> +
> +		/*
> +		 * If the message length is received from the slave (similar
> +		 * to SMBus block read), we must ensure that the buffer will
> +		 * be large enough to cope with a message length of
> +		 * I2C_SMBUS_BLOCK_MAX as this is the maximum underlying bus
> +		 * drivers allow. The first byte in the buffer must be
> +		 * pre-filled with the number of extra bytes, which must be
> +		 * at least one to hold the message length, but can be
> +		 * greater (for example to account for a checksum byte at
> +		 * the end of the message.)
> +		 */
> +		if (rdwr_pa[i].flags&  I2C_M_RECV_LEN) {
> +			if (!(rdwr_pa[i].flags&  I2C_M_RD) ||
> +			    rdwr_pa[i].buf[0]<  1 ||
> +			    rdwr_pa[i].len<  rdwr_pa[i].buf[0] +
> +					     I2C_SMBUS_BLOCK_MAX) {
> +				res = -EINVAL;
> +				break;
> +			}
> +
> +			rdwr_pa[i].len = rdwr_pa[i].buf[0];
> +		}
>   	}
>   	if (res<  0) {
>   		int j;
>
>

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Jean Delvare]

On Thu, 05 Apr 2012 20:01:43 -0400, Douglas Gilbert wrote:
> On 12-04-05 03:24 AM, Jean Delvare wrote:
> > As the bus driver side implementation of I2C_M_RECV_LEN is heavily
> > tied to SMBus, we can't support received length over 32 bytes, but
> > let's at least support that.
> >
> > In practice, the caller will have to setup a buffer large enough to
> > cover the case where received length byte has value 32, so minimum
> > 32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
> > added for the specific slave (for example a checksum.)
> 
> Either I am misunderstanding how to use this new patch or it is
> broken. After replacing the original patch with this one, setting
> msg->buf[0] to 2, my test program only sees the first two bytes
> of expected data:
>    08 81
> That is down from 8 bytes from the previous patch and 10 bytes
> expected from the SM130.

Does your I2C bus driver process I2C_M_RECV_LEN at all? I bet not.
You'll have to fix that. It's fairly easy, see the reference
implementation in i2c-algo-bit.c:readbytes(). The completely untested
attempt below may do, if not you'll have to fix my code:

---
 drivers/i2c/busses/i2c-at91.c |   17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

--- linux-3.4-rc1.orig/drivers/i2c/busses/i2c-at91.c	2012-04-06 08:27:38.000000000 +0200
+++ linux-3.4-rc1/drivers/i2c/busses/i2c-at91.c	2012-04-06 08:36:47.379360574 +0200
@@ -159,13 +159,14 @@ static short at91_poll_status(unsigned l
 	return (loop_cntr > 0);
 }
 
-static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length)
+static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length,
+		     int recv_len)
 {
 	int nack_seen = 0;
 	int sent_stop = 0;
 
 	/* Send Start */
-	if (1 == length) {
+	if ((1 == length) && !recv_len) {
 	    at91_twi_write(AT91_TWI_CR, AT91_TWI_START | AT91_TWI_STOP);
 	    sent_stop = 1;
 	} else
@@ -174,7 +175,7 @@ static int xfer_read(struct i2c_adapter
 	/* Read data */
 	while (length--) {
 		/* send Stop before reading last byte (if not already done) */
-		if ((0 == length) && (0 == sent_stop))
+		if ((0 == length) && (0 == sent_stop) && !recv_len)
 			at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
 		if (!at91_poll_status(AT91_TWI_RXRDY, &nack_seen)) {
 			dev_dbg(&adap->dev, "RXRDY timeout\n");
@@ -184,7 +185,12 @@ static int xfer_read(struct i2c_adapter
 			/* NACK supplies Stop */
 			return -EREMOTEIO;
 		}
-		*buf++ = (at91_twi_read(AT91_TWI_RHR) & 0xff);
+		*buf = (at91_twi_read(AT91_TWI_RHR) & 0xff);
+		if (recv_len) {
+			length += *buf;
+			recv_len = 0;
+		}
+		buf++;
 	}
 
 	return 0;
@@ -257,7 +263,8 @@ static int at91_xfer(struct i2c_adapter
 
 		if (pmsg->len && pmsg->buf) {	/* sanity check */
 			if (pmsg->flags & I2C_M_RD)
-				ret = xfer_read(adap, pmsg->buf, pmsg->len);
+				ret = xfer_read(adap, pmsg->buf, pmsg->len,
+						pmsg->flags & I2C_M_RECV_LEN);
 			else
 				ret = xfer_write(adap, pmsg->buf, pmsg->len);
 

-- 
Jean Delvare

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Douglas Gilbert]

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

On 12-04-06 02:37 AM, Jean Delvare wrote:
> On Thu, 05 Apr 2012 20:01:43 -0400, Douglas Gilbert wrote:
>> On 12-04-05 03:24 AM, Jean Delvare wrote:
>>> As the bus driver side implementation of I2C_M_RECV_LEN is heavily
>>> tied to SMBus, we can't support received length over 32 bytes, but
>>> let's at least support that.
>>>
>>> In practice, the caller will have to setup a buffer large enough to
>>> cover the case where received length byte has value 32, so minimum
>>> 32 + 1 = 33 bytes, possibly more if there is a fixed number of bytes
>>> added for the specific slave (for example a checksum.)
>>
>> Either I am misunderstanding how to use this new patch or it is
>> broken. After replacing the original patch with this one, setting
>> msg->buf[0] to 2, my test program only sees the first two bytes
>> of expected data:
>>     08 81
>> That is down from 8 bytes from the previous patch and 10 bytes
>> expected from the SM130.
>
> Does your I2C bus driver process I2C_M_RECV_LEN at all? I bet not.
> You'll have to fix that. It's fairly easy, see the reference
> implementation in i2c-algo-bit.c:readbytes(). The completely untested
> attempt below may do, if not you'll have to fix my code:

Jean,
Yes again, the modified i2c-at91.c driver that I am using does not
have support for I2C_M_RECV_LEN.

However stepping back from the minor I2C_M_RECV_LEN issue and looking
directly at the i2c-at91 driver itself. My patch to this broken
driver is included below and applies clean against lk 3.2.8
(but I note that it needs work to apply against lk 3.3). My patch
works for the AT91SAM9G20 and I have positive feedback from
the users of board-foxg20 based on that MCU.

However I see that Nikolaus Voss has submitted a replacement driver
for the i2c-at91 that works for the G45 which has two TWI units.
Is that driver going into the mainline? Surely it is much better
than the broken i2c-at91 driver that has been sitting broken for
way too long. Atmel are bringing out new MCUs in that family which
have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
about repeated starts surely Atmel have fixed the problems referred
to in existing mainline i2c-at91.c driver from circa 2006.

My vote would be to add Nikolaus Voss's driver ASAP.

Doug Gilbert

[-- Attachment #2: i2c-at91_32dpg1.patch --]
[-- Type: text/x-patch, Size: 11369 bytes --]

--- a/drivers/i2c/busses/Kconfig	2009-12-18 17:27:07.000000000 -0500
+++ b/drivers/i2c/busses/Kconfig	2010-01-21 00:28:07.000000000 -0500
@@ -283,7 +283,7 @@
 
 config I2C_AT91
 	tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
-	depends on ARCH_AT91 && EXPERIMENTAL && BROKEN
+	depends on ARCH_AT91 && EXPERIMENTAL
 	help
 	  This supports the use of the I2C interface on Atmel AT91
 	  processors.
--- a/drivers/i2c/busses/i2c-at91.c	2010-08-03 18:04:08.000000000 -0400
+++ b/drivers/i2c/busses/i2c-at91.c	2012-02-26 22:44:08.856672677 -0500
@@ -11,8 +11,38 @@
     it under the terms of the GNU General Public License as published by
     the Free Software Foundation; either version 2 of the License, or
     (at your option) any later version.
+
+    D. Gilbert (dpg) [20100318+20110219 tested on AT91SAM9G20]
+	- Check for NACK, a NACK will abort current tranfser,
+          returned as errno=EREMOTEIO unless I2C_M_IGNORE_NAK is set
+        - Only supports 7 bit I2C device (slave) address
+	- clockrate adjustable (module_param)
+	- see "AT91-AN01: Using the Two-wire interface (TWI) in Master
+	  Mode on AT91SAM Microcontrollers" from Atmel
+    dpg [20120225]
+	- treat i2c_rdwr_ioctl_data::nmsgs=0 is TWI software reset
 */
 
+/* This module source file is called i2c-at91.c but it is named at91_i2c by
+ * this code. [It was called at91_i2c in the past.] In sysfs it is found
+ * under the /sys/module/i2c_at91 directory (even when it is built in) .
+ *
+ * Once successfully initialized this driver will return one of two errors:
+ *   - EREMOTEIO: most likely the addressed slave is not present or not
+ *                responding. The I2C_M_IGNORE_NAK flag can be used to bypass
+ *                the lacks of ACKs from the slave.
+ *   - ETIMEDOUT: probably some slave is holding down the bus. Both SDA and
+ *                SCL need to be high (probably around 3 volts) when not
+ *                active. If SDA or SCL are around 0 volts then the bus is
+ *                being held down by a slave (or perhaps the master). The
+ *                irresponsible slave needs to be reset (some I2C devices
+ *                have a reset line). Power cycling the slave works as a
+ *                reset.
+ */
+
+/* Uncomment following line to see dev_dbg() output in logs */
+/* #define DEBUG 1 */
+
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/err.h>
@@ -29,73 +59,143 @@
 #include <mach/board.h>
 #include <mach/cpu.h>
 
-#define TWI_CLOCK		100000		/* Hz. max 400 Kbits/sec */
+#define DRV_VERSION	"2.1"
+#define TWI_CLOCK	100000	/* Hertz, I2C standard mode clock */
 
 
+static unsigned int clockrate = TWI_CLOCK;
+static unsigned int prev_clockrate = TWI_CLOCK;
 static struct clk *twi_clk;
 static void __iomem *twi_base;
 
 #define at91_twi_read(reg)		__raw_readl(twi_base + (reg))
 #define at91_twi_write(reg, val)	__raw_writel((val), twi_base + (reg))
 
-
 /*
- * Initialize the TWI hardware registers.
+ * Set TWI clock dividers based on clockrate (clock rate for SCL)
  */
-static void __devinit at91_twi_hwinit(void)
+static void at91_twi_clock_dividers(void)
 {
-	unsigned long cdiv, ckdiv;
+	unsigned ckdiv, cldiv, chdiv;
 
-	at91_twi_write(AT91_TWI_IDR, 0xffffffff);	/* Disable all interrupts */
-	at91_twi_write(AT91_TWI_CR, AT91_TWI_SWRST);	/* Reset peripheral */
-	at91_twi_write(AT91_TWI_CR, AT91_TWI_MSEN);	/* Set Master mode */
+	/* Restrict clockrate to be between 1 kHz and 4 Mhz */
+	if (clockrate < 1000)
+		clockrate = 1000;	
+	else if (clockrate > 4000000)
+		clockrate = 4000000;	
 
-	/* Calcuate clock dividers */
-	cdiv = (clk_get_rate(twi_clk) / (2 * TWI_CLOCK)) - 3;
-	cdiv = cdiv + 1;	/* round up */
+	/* According to G20 and G45 manuals we want to solve these: */
+	/* T_low = ((cldiv * (2 ** ckdiv)) + 4) * T_mclk  */
+	/* T_high = ((chdiv * (2 ** ckdiv)) + 4) * T_mclk  */
+	/* where T_low = T_high; cldiv and chdiv are 8 bits each and */
+	/* ckdiv is no more that 7 (sometimes less) */
 	ckdiv = 0;
-	while (cdiv > 255) {
-		ckdiv++;
-		cdiv = cdiv >> 1;
+	/* The twi_clk is the Mclk on the G20 and G45 */
+	cldiv = clk_get_rate(twi_clk) / (2 * clockrate);
+	if (cldiv < 4)
+		cldiv = 4;
+	cldiv -= 4;
+	while (cldiv > 255) {
+		cldiv >>= 1;
+		++ckdiv;
 	}
+	chdiv = cldiv;	/* 1:1 mark space ratio (why get such a choice) */
 
-	if (cpu_is_at91rm9200()) {			/* AT91RM9200 Errata #22 */
+	if (cpu_is_at91rm9200()) {		/* AT91RM9200 Errata #22 */
 		if (ckdiv > 5) {
-			printk(KERN_ERR "AT91 I2C: Invalid TWI_CLOCK value!\n");
+			printk(KERN_ERR "i2c-at91: AT91RM9200 clock rate too "
+			       "low, ckdiv=%u, set to 5\n", ckdiv);
 			ckdiv = 5;
 		}
+	} else if (cpu_is_at91sam9g20()) {
+		/* AT91SAM9G20 has 3 bits for ckdiv so it cannot exceed 7 */
+		if (ckdiv > 7) {
+			printk(KERN_ERR "i2c-at91: AT91SAM9G20 clock rate "
+			       "too low, ckdiv=%u, set to 7\n", ckdiv);
+			ckdiv = 7;
+		}
+	} else {
+		/* Assume others in AT91 family have 3 bits for ckdiv */
+		if (ckdiv > 7) {
+			printk(KERN_ERR "i2c-at91: AT91 clock rate too low, "
+			       "ckdiv=%u, set to 7\n", ckdiv);
+			ckdiv = 7;
+		}
 	}
 
-	at91_twi_write(AT91_TWI_CWGR, (ckdiv << 16) | (cdiv << 8) | cdiv);
+	at91_twi_write(AT91_TWI_CWGR, (ckdiv << 16) | (chdiv << 8) | cldiv);
+	prev_clockrate = clockrate;
 }
 
 /*
- * Poll the i2c status register until the specified bit is set.
- * Returns 0 if timed out (100 msec).
+ * Initialize the TWI hardware registers.
  */
-static short at91_poll_status(unsigned long bit)
+static void at91_twi_hwinit(void)
 {
-	int loop_cntr = 10000;
+	at91_twi_write(AT91_TWI_IDR, 0xffffffff);  /* Disable all interrupts */
+	at91_twi_write(AT91_TWI_CR, AT91_TWI_SWRST);     /* Reset peripheral */
+	/* Set Master mode; Atmel suggests disabling slave mode */
+	at91_twi_write(AT91_TWI_CR, AT91_TWI_MSEN | AT91_TWI_SVDIS);
 
+	at91_twi_clock_dividers();
+}
+
+/*
+ * Poll the i2c status register until the specified bit is set or a NACK
+ * occurs. Returns 0 if timed out (50 msec). If nack_seen_p is non-NULL
+ * then write 0 to it first, then if the NACK bit is set in the status
+ * register then write 1 to it and immediately return with a value of 1.
+ */
+static short at91_poll_status(unsigned long bit, int * nack_seen_p)
+{
+	int loop_cntr = 5000;
+	unsigned long stat;
+
+	if (nack_seen_p)
+		*nack_seen_p = 0;
+	if (clockrate <= 20000)
+		loop_cntr = 100;
 	do {
-		udelay(10);
-	} while (!(at91_twi_read(AT91_TWI_SR) & bit) && (--loop_cntr > 0));
+		if (clockrate <= 20000)
+			udelay(100);
+		else if (clockrate <= 100000)
+			udelay(10);
+		else
+			udelay(3);
+		stat = at91_twi_read(AT91_TWI_SR);
+		if ((stat & AT91_TWI_NACK) && nack_seen_p) {
+			*nack_seen_p = 1;
+			return 1;
+		}
+	} while (!(stat & bit) && (--loop_cntr > 0));
 
 	return (loop_cntr > 0);
 }
 
 static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length)
 {
+	int nack_seen = 0;
+	int sent_stop = 0;
+
 	/* Send Start */
-	at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
+	if (1 == length) {
+	    at91_twi_write(AT91_TWI_CR, AT91_TWI_START | AT91_TWI_STOP);
+	    sent_stop = 1;
+	} else
+	    at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
 
 	/* Read data */
 	while (length--) {
-		if (!length)	/* need to send Stop before reading last byte */
+		/* send Stop before reading last byte (if not already done) */
+		if ((0 == length) && (0 == sent_stop))
 			at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
-		if (!at91_poll_status(AT91_TWI_RXRDY)) {
+		if (!at91_poll_status(AT91_TWI_RXRDY, &nack_seen)) {
 			dev_dbg(&adap->dev, "RXRDY timeout\n");
 			return -ETIMEDOUT;
+		} else if (nack_seen) {
+			dev_dbg(&adap->dev, "read NACKed\n");
+			/* NACK supplies Stop */
+			return -EREMOTEIO;
 		}
 		*buf++ = (at91_twi_read(AT91_TWI_RHR) & 0xff);
 	}
@@ -105,16 +205,24 @@
 
 static int xfer_write(struct i2c_adapter *adap, unsigned char *buf, int length)
 {
+	int nack_seen = 0;
+
 	/* Load first byte into transmitter */
 	at91_twi_write(AT91_TWI_THR, *buf++);
 
-	/* Send Start */
+	/* Send Start [AT91SAM9G20 does not need this on write] */
 	at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
 
 	do {
-		if (!at91_poll_status(AT91_TWI_TXRDY)) {
+		if (!at91_poll_status(AT91_TWI_TXRDY, &nack_seen)) {
 			dev_dbg(&adap->dev, "TXRDY timeout\n");
+			/* Set Master mode again */
+			at91_twi_write(AT91_TWI_CR, AT91_TWI_MSEN);
 			return -ETIMEDOUT;
+		} else if (nack_seen) {
+			dev_dbg(&adap->dev, "write NACKed\n");
+			/* NACK supplies Stop */
+			return -EREMOTEIO;
 		}
 
 		length--;	/* byte was transmitted */
@@ -123,7 +231,7 @@
 			at91_twi_write(AT91_TWI_THR, *buf++);
 	} while (length);
 
-	/* Send Stop */
+	/* Send Stop [AT91SAM9G20 does not need this on write] */
 	at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
 
 	return 0;
@@ -136,11 +244,25 @@
  * Instead the "internal device address" has to be written using a separate
  * i2c message.
  * http://lists.arm.linux.org.uk/pipermail/linux-arm-kernel/2004-September/024411.html
+ * [dpg] By 2010 silicon bugs should be fixed, will need IADR for 10 bit device address
  */
 static int at91_xfer(struct i2c_adapter *adap, struct i2c_msg *pmsg, int num)
 {
 	int i, ret;
-
+	int nack_seen = 0;
+ 
+	if (0 == num) {
+		dev_dbg(&adap->dev, "at91_xfer: treat num==0 as at91 twi "
+			"reset\n");
+		at91_twi_hwinit();
+		return num;
+	}
+	if (prev_clockrate != clockrate) {
+		dev_dbg(&adap->dev, "at91_xfer: prev_clockrate=%u "
+			"clockrate=%u, change\n", prev_clockrate, clockrate);
+		at91_twi_clock_dividers();
+		msleep(1);	/* let things settle */
+	}
 	dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
 
 	for (i = 0; i < num; i++) {
@@ -158,13 +280,23 @@
 			else
 				ret = xfer_write(adap, pmsg->buf, pmsg->len);
 
-			if (ret)
-				return ret;
-
+			if (ret) {
+				if ((I2C_M_IGNORE_NAK & pmsg->flags) &&
+				    (-EREMOTEIO == ret)) {
+					dev_dbg(&adap->dev, "transfer "
+						"NACKed, skip to next\n");
+					pmsg++;
+					continue;
+				} else
+					return ret;
+			}
 			/* Wait until transfer is finished */
-			if (!at91_poll_status(AT91_TWI_TXCOMP)) {
+			if (!at91_poll_status(AT91_TWI_TXCOMP, &nack_seen)) {
 				dev_dbg(&adap->dev, "TXCOMP timeout\n");
 				return -ETIMEDOUT;
+			} else if (nack_seen) {
+				dev_dbg(&adap->dev, "TXCOMP NACKed\n");
+				return -EREMOTEIO;
 			}
 		}
 		dev_dbg(&adap->dev, "transfer complete\n");
@@ -239,7 +371,8 @@
 		goto fail3;
 	}
 
-	dev_info(&pdev->dev, "AT91 i2c bus driver.\n");
+	dev_info(&pdev->dev, "Atmel TWI (I2C) master [SCL %d Hz], version: "
+		 "%s\n", clockrate, DRV_VERSION);
 	return 0;
 
 fail3:
@@ -287,7 +420,11 @@
 
 static int at91_i2c_resume(struct platform_device *pdev)
 {
-	return clk_enable(twi_clk);
+	int res;
+
+	res = clk_enable(twi_clk);
+	at91_twi_hwinit();
+	return res;
 }
 
 #else
@@ -295,6 +432,11 @@
 #define at91_i2c_resume		NULL
 #endif
 
+/* I2C clock speed, in Hz 0-400kHz*/
+module_param(clockrate, uint,  S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(clockrate,
+		 "SCL clock rate, 1000 to 4000000 Hz (def: 100 kHz)");
+
 /* work with "modprobe at91_i2c" from hotplugging or coldplugging */
 MODULE_ALIAS("platform:at91_i2c");
 
@@ -323,5 +465,6 @@
 module_exit(at91_i2c_exit);
 
 MODULE_AUTHOR("Rick Bronson");
-MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
+MODULE_DESCRIPTION("I2C (TWI) master for Atmel AT91 series");
 MODULE_LICENSE("GPL");
+MODULE_VERSION(DRV_VERSION);

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Jean Delvare]

On Fri, 06 Apr 2012 12:16:19 -0400, Douglas Gilbert wrote:
> Yes again, the modified i2c-at91.c driver that I am using does not
> have support for I2C_M_RECV_LEN.

Did you try my patch?

> However stepping back from the minor I2C_M_RECV_LEN issue and looking
> directly at the i2c-at91 driver itself. My patch to this broken
> driver is included below and applies clean against lk 3.2.8
> (but I note that it needs work to apply against lk 3.3). My patch
> works for the AT91SAM9G20 and I have positive feedback from
> the users of board-foxg20 based on that MCU.

I picked this patch from your website already, and forward ported it,
my own patch was on top of yours.

> However I see that Nikolaus Voss has submitted a replacement driver
> for the i2c-at91 that works for the G45 which has two TWI units.
> Is that driver going into the mainline? Surely it is much better
> than the broken i2c-at91 driver that has been sitting broken for
> way too long. Atmel are bringing out new MCUs in that family which
> have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
> about repeated starts surely Atmel have fixed the problems referred
> to in existing mainline i2c-at91.c driver from circa 2006.
> 
> My vote would be to add Nikolaus Voss's driver ASAP.

I'm not into embedded devices, so this isn't my call.

-- 
Jean Delvare

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Douglas Gilbert]

On 12-04-06 12:25 PM, Jean Delvare wrote:
> On Fri, 06 Apr 2012 12:16:19 -0400, Douglas Gilbert wrote:
>> Yes again, the modified i2c-at91.c driver that I am using does not
>> have support for I2C_M_RECV_LEN.
>
> Did you try my patch?

No. I didn't realize it was on top of my patches to the
i2c-at91 driver, sorry.

>> However stepping back from the minor I2C_M_RECV_LEN issue and looking
>> directly at the i2c-at91 driver itself. My patch to this broken
>> driver is included below and applies clean against lk 3.2.8
>> (but I note that it needs work to apply against lk 3.3). My patch
>> works for the AT91SAM9G20 and I have positive feedback from
>> the users of board-foxg20 based on that MCU.
>
> I picked this patch from your website already, and forward ported it,
> my own patch was on top of yours.

So I have now applied your patch over my patch to the i2c-at91
driver and tested it. The result is the same as the previous
iteration: only two non-zero bytes: "08 81".

>> However I see that Nikolaus Voss has submitted a replacement driver
>> for the i2c-at91 that works for the G45 which has two TWI units.
>> Is that driver going into the mainline? Surely it is much better
>> than the broken i2c-at91 driver that has been sitting broken for
>> way too long. Atmel are bringing out new MCUs in that family which
>> have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
>> about repeated starts surely Atmel have fixed the problems referred
>> to in existing mainline i2c-at91.c driver from circa 2006.
>>
>> My vote would be to add Nikolaus Voss's driver ASAP.
>
> I'm not into embedded devices, so this isn't my call.

A pity. I checked lk 3.4-rc1 and the bad old driver is still there.

Doug Gilbert

Re: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Jean Delvare]

On Fri, 06 Apr 2012 13:04:32 -0400, Douglas Gilbert wrote:
> So I have now applied your patch over my patch to the i2c-at91
> driver and tested it. The result is the same as the previous
> iteration: only two non-zero bytes: "08 81".

Bah. I already spent more time on this than I wanted, and it seems I am
going nowhere, due to the lack of hardware to test my changes. Are you
willing to debug my code and fix it?

-- 
Jean Delvare

RE: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Voss, Nikolaus]

Hi,

Douglas Gilbert wrote on 2012-04-06:
> On 12-04-06 12:25 PM, Jean Delvare wrote:
>> On Fri, 06 Apr 2012 12:16:19 -0400, Douglas Gilbert wrote:
>>> However I see that Nikolaus Voss has submitted a replacement driver
>>> for the i2c-at91 that works for the G45 which has two TWI units.
>>> Is that driver going into the mainline? Surely it is much better
>>> than the broken i2c-at91 driver that has been sitting broken for
>>> way too long. Atmel are bringing out new MCUs in that family which
>>> have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
>>> about repeated starts surely Atmel have fixed the problems referred
>>> to in existing mainline i2c-at91.c driver from circa 2006.
>>> 
>>> My vote would be to add Nikolaus Voss's driver ASAP.
>> 
>> I'm not into embedded devices, so this isn't my call.
> 
> A pity. I checked lk 3.4-rc1 and the bad old driver is still there.

as I see it, my driver has only been tested with the G45. If you
can confirm it works with other devices, too, maybe this would help
to get it into next.

Niko

RE: [PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Voss, Nikolaus]

Hi,

Douglas Gilbert wrote on 2012-04-06:
> On 12-04-06 12:25 PM, Jean Delvare wrote:
>> On Fri, 06 Apr 2012 12:16:19 -0400, Douglas Gilbert wrote:
>>> However I see that Nikolaus Voss has submitted a replacement driver
>>> for the i2c-at91 that works for the G45 which has two TWI units.
>>> Is that driver going into the mainline? Surely it is much better
>>> than the broken i2c-at91 driver that has been sitting broken for
>>> way too long. Atmel are bringing out new MCUs in that family which
>>> have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
>>> about repeated starts surely Atmel have fixed the problems referred
>>> to in existing mainline i2c-at91.c driver from circa 2006.
>>> 
>>> My vote would be to add Nikolaus Voss's driver ASAP.
>> 
>> I'm not into embedded devices, so this isn't my call.
> 
> A pity. I checked lk 3.4-rc1 and the bad old driver is still there.

as I see it, my driver has only been tested with the G45. If you
can confirm it works with other devices, too, maybe this would help
to get it into next.

Niko

[PATCH] i2c-dev: Add support for I2C_M_RECV_LEN

[Voss, Nikolaus]

Hi,

Douglas Gilbert wrote on 2012-04-06:
> On 12-04-06 12:25 PM, Jean Delvare wrote:
>> On Fri, 06 Apr 2012 12:16:19 -0400, Douglas Gilbert wrote:
>>> However I see that Nikolaus Voss has submitted a replacement driver
>>> for the i2c-at91 that works for the G45 which has two TWI units.
>>> Is that driver going into the mainline? Surely it is much better
>>> than the broken i2c-at91 driver that has been sitting broken for
>>> way too long. Atmel are bringing out new MCUs in that family which
>>> have 3 TWI units (e.g. AT91SAM9G25). Apart from the limitations
>>> about repeated starts surely Atmel have fixed the problems referred
>>> to in existing mainline i2c-at91.c driver from circa 2006.
>>> 
>>> My vote would be to add Nikolaus Voss's driver ASAP.
>> 
>> I'm not into embedded devices, so this isn't my call.
> 
> A pity. I checked lk 3.4-rc1 and the bad old driver is still there.

as I see it, my driver has only been tested with the G45. If you
can confirm it works with other devices, too, maybe this would help
to get it into next.

Niko