[PATCH] I2C block read

[PATCH] I2C block read

[Alexander Atanasov]

	Hello,

	When doing i2c block read the lenght is passed as the first byte of the buffer,
so we must copy it from user otherwise temp is uninitialized.

Signed-off-by: Alexander Atanasov <alex@ssi.bg>

--
have fun,
alex


--- drivers/i2c/i2c-dev.c.orig	2006-01-04 02:00:00.000000000 +0200
+++ drivers/i2c/i2c-dev.c	2006-06-07 19:46:08.000000000 +0300
@@ -337,6 +337,7 @@
 
 		if ((data_arg.size == I2C_SMBUS_PROC_CALL) || 
 		    (data_arg.size == I2C_SMBUS_BLOCK_PROC_CALL) || 
+		    (data_arg.size == I2C_SMBUS_BLOCK_DATA) ||
 		    (data_arg.read_write == I2C_SMBUS_WRITE)) {
 			if (copy_from_user(&temp, data_arg.data, datasize))
 				return -EFAULT;

Re: [PATCH] I2C block read

[Jean Delvare]

Hi Alexander,

> 	When doing i2c block read the lenght is passed as the first byte of the buffer,
> so we must copy it from user otherwise temp is uninitialized.

> --- drivers/i2c/i2c-dev.c.orig	2006-01-04 02:00:00.000000000 +0200
> +++ drivers/i2c/i2c-dev.c	2006-06-07 19:46:08.000000000 +0300
> @@ -337,6 +337,7 @@
>  
>  		if ((data_arg.size == I2C_SMBUS_PROC_CALL) || 
>  		    (data_arg.size == I2C_SMBUS_BLOCK_PROC_CALL) || 
> +		    (data_arg.size == I2C_SMBUS_BLOCK_DATA) ||
>  		    (data_arg.read_write == I2C_SMBUS_WRITE)) {
>  			if (copy_from_user(&temp, data_arg.data, datasize))
>  				return -EFAULT;

Nack. Firstly, your comment says I2C block read, but your code changes
SMBus block read. These are two different transactions. Secondly, for
SMBus block read, the master doesn't ask for a given number of bytes.
Instead, the chip decides and returns the number of (following) bytes as
the first byte of the read part of the transaction. Check the SMBus
specification.

So your patch is not correct, sorry.

-- 
Jean Delvare

Re: [PATCH] I2C block read

[Alexander Atanasov]

On Wed, 7 Jun 2006 19:49:43 +0200
Jean Delvare wrote:

> Hi Alexander,
> 
> > 	When doing i2c block read the lenght is passed as the first
> > byte of the buffer, so we must copy it from user otherwise temp is
> > uninitialized.
> 
> > --- drivers/i2c/i2c-dev.c.orig	2006-01-04 02:00:00.000000000
> > +0200 +++ drivers/i2c/i2c-dev.c	2006-06-07
> > 19:46:08.000000000 +0300 @@ -337,6 +337,7 @@
> >  
> >  		if ((data_arg.size == I2C_SMBUS_PROC_CALL) || 
> >  		    (data_arg.size == I2C_SMBUS_BLOCK_PROC_CALL)
> > || 
> > +		    (data_arg.size == I2C_SMBUS_BLOCK_DATA) ||
> >  		    (data_arg.read_write == I2C_SMBUS_WRITE)) {
> >  			if (copy_from_user(&temp, data_arg.data,
> > datasize)) return -EFAULT;
> 
> Nack. Firstly, your comment says I2C block read, but your code changes
> SMBus block read. These are two different transactions. Secondly, for
> SMBus block read, the master doesn't ask for a given number of bytes.
> Instead, the chip decides and returns the number of (following) bytes
> as the first byte of the read part of the transaction. Check the SMBus
> specification.
> 
> So your patch is not correct, sorry.


	10x. Here is the case:
I use SMBus block read with scx200_acb as a bus it uses the lenght
which is as i say uninitialized and tries to read a number of random bytes
from the board i use. And it doesn't return the lenght as a first byte,
so the drivers reads until it gets an oops. Which in turn makes me wonder
why with this patch i correcly receive the number of bytes i pass,
looking at the driver i can not see that it gets the lenght from the read data.
I don't know well SMBus/I2C specs but read with buffer and no lenght 
doesn't look sane to me. So how should this be fixed?

--
have fun,
alex

 

	

Re: [PATCH] I2C block read

[Jean Delvare]

Hi Alexander,

Adding Jordan Crouse in the loop, he's working on the scx200_acb driver
these days.

> I use SMBus block read with scx200_acb as a bus it uses the lenght
> which is as i say uninitialized and tries to read a number of random bytes
> from the board i use. And it doesn't return the lenght as a first byte,
> so the drivers reads until it gets an oops. Which in turn makes me wonder
> why with this patch i correcly receive the number of bytes i pass,
> looking at the driver i can not see that it gets the lenght from the read data.
> I don't know well SMBus/I2C specs but read with buffer and no lenght 
> doesn't look sane to me. So how should this be fixed?

There _is_ a length, it's just decided by the target chip rather than
the host. It makes some sense when you think about it, even if it is a
bit susprising at first, I agree. The length is limited to 32 bytes as
per the SMBus specification, so it's safe. I invite you to read the
SMBus 2.0 specification [1] for more details if you're interested.

As for the problem you encounter, it looks like a bug in the scx200_acb
driver to me. It advertises the I2C_FUNC_SMBUS_BLOCK_DATA
functionality, but due to its implementation it can only support SMBus
block writes and not SMBus block reads. I see no reason why the chip
itself couldn't do it, but right now the driver can't. The state
machine the driver is based on would need some rework before the
functionality can be added. In the meantime, the quickest fix is to
have the driver only advertise the functionalities it actually supports:


The scx200_acb advertises SMBus block transactions as supported. In
fact, it supports SMBus block writes, but not SMBus block reads which
are more complex to implement.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/i2c/busses/scx200_acb.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

--- linux-2.6.17-rc6.orig/drivers/i2c/busses/scx200_acb.c	2006-06-07 18:13:53.000000000 +0200
+++ linux-2.6.17-rc6/drivers/i2c/busses/scx200_acb.c	2006-06-07 20:29:27.000000000 +0200
@@ -308,6 +308,12 @@
 		break;
 
 	case I2C_SMBUS_BLOCK_DATA:
+		/* Sanity check */
+		if (rw == I2C_SMBUS_READ) {
+			dev_warn(&adapter->dev, "SMBus block read is not "
+				 "supported!\n");
+			return -EINVAL;
+		}
 		len = data->block[0];
 		buffer = &data->block[1];
 		break;
@@ -372,7 +378,7 @@
 {
 	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
 	       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
-	       I2C_FUNC_SMBUS_BLOCK_DATA;
+	       I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
 }
 
 /* For now, we only handle combined mode (smbus) */


May I ask for what slave SMBus chip you need the SMBus block read
transaction? It's only rarely used, so it could be that it's not what
you need after all.

The scx200_acb code would probably benefit from a general review. It
looks to me like quick command with data bit == 1 would fail, for
example. It's not a very popular transaction either, but i2c bus
drivers should handle all transactions they advertise as supported.

[1] http://www.smbus.org/specs/

-- 
Jean Delvare

Re: I2C block read

[Jordan Crouse]

On 07/06/06 20:50 +0200, Jean Delvare wrote:
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>  drivers/i2c/busses/scx200_acb.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.17-rc6.orig/drivers/i2c/busses/scx200_acb.c	2006-06-07 18:13:53.000000000 +0200
> +++ linux-2.6.17-rc6/drivers/i2c/busses/scx200_acb.c	2006-06-07 20:29:27.000000000 +0200
> @@ -308,6 +308,12 @@
>  		break;
>  
>  	case I2C_SMBUS_BLOCK_DATA:
> +		/* Sanity check */
> +		if (rw == I2C_SMBUS_READ) {
> +			dev_warn(&adapter->dev, "SMBus block read is not "
> +				 "supported!\n");
> +			return -EINVAL;
> +		}
>  		len = data->block[0];
>  		buffer = &data->block[1];
>  		break;
> @@ -372,7 +378,7 @@
>  {
>  	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
>  	       I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> -	       I2C_FUNC_SMBUS_BLOCK_DATA;
> +	       I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
>  }
>  
>  /* For now, we only handle combined mode (smbus) */
> 

ACKed (if my opinion matters).  It will take some more thinking to handle
block reads, and they're really not very interesting anyway.  At the very
least, this will keep implementations from freaking out, and thats a 
Good Thing (TM).

Jordan

--
Jordan Crouse
Senior Linux Engineer
Advanced Micro Devices, Inc.
<www.amd.com/embeddedprocessors>

Re: [PATCH] I2C block read

[Alexander Atanasov]

	Hello,
On Wed, 7 Jun 2006 20:50:25 +0200
Jean Delvare wrote:

> As for the problem you encounter, it looks like a bug in the
> scx200_acb driver to me. It advertises the I2C_FUNC_SMBUS_BLOCK_DATA
> functionality, but due to its implementation it can only support SMBus
> block writes and not SMBus block reads. I see no reason why the chip
> itself couldn't do it, but right now the driver can't. The state
> machine the driver is based on would need some rework before the
> functionality can be added. In the meantime, the quickest fix is to
> 

	OK, i got it now.  Untested patch that should do it.
> 
> May I ask for what slave SMBus chip you need the SMBus block read
> transaction? It's only rarely used, so it could be that it's not what
> you need after all.

	It's custom developed board with using some motorola processor,
and the implementation there is buggy doesn't send the lenght but
this is fixable. So i'll have to fix the state machine after that.
Guy who is developing the board says that he doesn't get the ACK 
after the last byte is read too - one more thing to investigate.

Also i use some pc engines board which has geode cpu on it
and have two access busses one is at 0x820 and the second is at 0x810
but the second is not in the list of probed devices, can it be added?

> 
> The scx200_acb code would probably benefit from a general review. It
> looks to me like quick command with data bit == 1 would fail, for
> example. It's not a very popular transaction either, but i2c bus
> drivers should handle all transactions they advertise as supported.
> 

--
have fun,
alex

--- drivers/i2c/busses/scx200_acb.c.orig	2006-06-08 15:57:38.000000000 +0300
+++ drivers/i2c/busses/scx200_acb.c	2006-06-08 16:24:32.000000000 +0300
@@ -175,12 +175,22 @@
 			iface->state = state_read;
 		} else {
 			outb(iface->address_byte, ACBSDA);
-
 			iface->state = state_write;
 		}
 		break;
 
 	case state_read:
+		if (iface->len == 42) {
+			outb(inb(ACBCTL1) & ~ACBCTL1_ACK, ACBCTL1);
+			iface->len = inb(ACBSDA);
+			if (iface->len > 31) {
+				dev_dbg(&iface->adapter.dev, "Invalid read lenght %d in state %s\n",
+					iface->len, scx200_acb_state_name[iface->state]);
+				errmsg = "Invalid read lenght";
+				goto error;
+			}
+			break;
+		}
 		/* Set ACK if receiving the last byte */
 		if (iface->len == 1)
 			outb(inb(ACBCTL1) | ACBCTL1_ACK, ACBCTL1);
@@ -305,7 +315,11 @@
 		break;
 
 	case I2C_SMBUS_BLOCK_DATA:
-		len = data->block[0];
+		/* 42 is invalid lenght max 32, use it to get real lenght from data */
+		if (rw == I2C_SMBUS_READ)
+			len = 42;
+		else
+			len = data->block[0];
 		buffer = &data->block[1];
 		break;
 

Re: [PATCH] I2C block read

[Jean Delvare]

Hi Alexander,

> > As for the problem you encounter, it looks like a bug in the
> > scx200_acb driver to me. It advertises the I2C_FUNC_SMBUS_BLOCK_DATA
> > functionality, but due to its implementation it can only support SMBus
> > block writes and not SMBus block reads. I see no reason why the chip
> > itself couldn't do it, but right now the driver can't. The state
> > machine the driver is based on would need some rework before the
> > functionality can be added. In the meantime, the quickest fix is to
> 
> 	OK, i got it now.  Untested patch that should do it.

It won't work. There are bugs in the state machine which need to be
fixed before any block transaction will work (be it I2C or SMBus.)

Fix the scx200_acb state machine:

* Nack was sent one byte too late on reads >= 2 bytes.
* Stop bit was set one byte too late on reads.
* Stop bit was set one byte too late on writes.

Credits go to Thomas Andrews for finding and fixing the first
two items.

Testers wanted! Not suitable for inclusion at this point.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/i2c/busses/scx200_acb.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

--- linux-2.6.17-rc6.orig/drivers/i2c/busses/scx200_acb.c	2006-06-11 14:54:02.000000000 +0200
+++ linux-2.6.17-rc6/drivers/i2c/busses/scx200_acb.c	2006-06-11 14:55:23.000000000 +0200
@@ -184,29 +184,28 @@
 		break;
 
 	case state_read:
-		/* Set ACK if receiving the last byte */
-		if (iface->len == 1)
+		/* Set ACK if _next_ byte will be the last one */
+		if (iface->len == 2)
 			outb(inb(ACBCTL1) | ACBCTL1_ACK, ACBCTL1);
 		else
 			outb(inb(ACBCTL1) & ~ACBCTL1_ACK, ACBCTL1);
 
-		*iface->ptr++ = inb(ACBSDA);
-		--iface->len;
-
-		if (iface->len == 0) {
+		if (iface->len == 1) {
 			iface->result = 0;
 			iface->state = state_idle;
 			outb(inb(ACBCTL1) | ACBCTL1_STOP, ACBCTL1);
 		}
 
+		*iface->ptr++ = inb(ACBSDA);
+		--iface->len;
+
 		break;
 
 	case state_write:
-		if (iface->len == 0) {
+		if (iface->len == 1) {
 			iface->result = 0;
 			iface->state = state_idle;
 			outb(inb(ACBCTL1) | ACBCTL1_STOP, ACBCTL1);
-			break;
 		}
 
 		outb(*iface->ptr++, ACBSDA);


> > May I ask for what slave SMBus chip you need the SMBus block read
> > transaction? It's only rarely used, so it could be that it's not what
> > you need after all.
> 
> 	It's custom developed board with using some motorola processor,
> and the implementation there is buggy doesn't send the lenght but
> this is fixable. So i'll have to fix the state machine after that.
> Guy who is developing the board says that he doesn't get the ACK 
> after the last byte is read too - one more thing to investigate.

This sounds like your device is expecting an I2C block read rather than
an SMBus block read. Both transactions look alike, what matters is that
both sides of the transaction agree on which transaction type should be
used. I2C block reads are more simple, the data bytes are returned
directly (no length byte.)

As for ack problem, the patch above should hopefully fix it.

> Also i use some pc engines board which has geode cpu on it
> and have two access busses one is at 0x820 and the second is at 0x810
> but the second is not in the list of probed devices, can it be added?

There is a module parameter which can be used for that. From an
upcoming documentation update for the scx200_acb driver:

The SC1100 WRAP boards are known to use base addresses 0x810 and 0x820.
If the scx200_acb driver is built into the kernel, add the following
parameter to your boot command line:
  scx200_acb.base=0x810,0x820
If the scx200_acb driver is built as a module, add the following line to
the file /etc/modprobe.conf instead:
  options scx200_acb base=0x810,0x820

There might be a way to have the driver auto-detect the correct address
but I didn't have the time to look into it lately.

> --- drivers/i2c/busses/scx200_acb.c.orig	2006-06-08 15:57:38.000000000 +0300
> +++ drivers/i2c/busses/scx200_acb.c	2006-06-08 16:24:32.000000000 +0300
> @@ -175,12 +175,22 @@
>  			iface->state = state_read;
>  		} else {
>  			outb(iface->address_byte, ACBSDA);
> -
>  			iface->state = state_write;
>  		}
>  		break;
>  
>  	case state_read:
> +		if (iface->len == 42) {
> +			outb(inb(ACBCTL1) & ~ACBCTL1_ACK, ACBCTL1);
> +			iface->len = inb(ACBSDA);
> +			if (iface->len > 31) {
> +				dev_dbg(&iface->adapter.dev, "Invalid read lenght %d in state %s\n",
> +					iface->len, scx200_acb_state_name[iface->state]);
> +				errmsg = "Invalid read lenght";
> +				goto error;
> +			}
> +			break;
> +		}
>  		/* Set ACK if receiving the last byte */
>  		if (iface->len == 1)
>  			outb(inb(ACBCTL1) | ACBCTL1_ACK, ACBCTL1);
> @@ -305,7 +315,11 @@
>  		break;
>  
>  	case I2C_SMBUS_BLOCK_DATA:
> -		len = data->block[0];
> +		/* 42 is invalid lenght max 32, use it to get real lenght from data */
> +		if (rw == I2C_SMBUS_READ)
> +			len = 42;
> +		else
> +			len = data->block[0];
>  		buffer = &data->block[1];
>  		break;
>  

I get the idea, something of that kind might work once the rest is
fixed. Of course we want to define a nice constant instead of
hardcoding 42, and also note that length is spelled length.

-- 
Jean Delvare

Re: [PATCH] I2C block read

[Alexander Atanasov]

	Hello,

On Sun, 11 Jun 2006 15:30:34 +0200
Jean Delvare wrote:

> Fix the scx200_acb state machine:
> 
> * Nack was sent one byte too late on reads >= 2 bytes.
> * Stop bit was set one byte too late on reads.
> * Stop bit was set one byte too late on writes.
> 
> Credits go to Thomas Andrews for finding and fixing the first
> two items.
> 
> Testers wanted! Not suitable for inclusion at this point.

	with the patch below i get the correct results.
the patch for testing and some changes for the block reads.
and the ACK seems working - i have some status bits
which get cleared on correct read (i.e. the ACK is sent) and i have them cleared.

> This sounds like your device is expecting an I2C block read rather
> than an SMBus block read. Both transactions look alike, what matters
> is that both sides of the transaction agree on which transaction type
> should be used. I2C block reads are more simple, the data bytes are
> returned directly (no length byte.)
> 
> As for ack problem, the patch above should hopefully fix it.

	SMBus read it's fixed now on the board. As for the ACK problem i think it is ok.

> The SC1100 WRAP boards are known to use base addresses 0x810 and
> 0x820. If the scx200_acb driver is built into the kernel, add the
> following parameter to your boot command line:
>   scx200_acb.base=0x810,0x820
> If the scx200_acb driver is built as a module, add the following line
> to the file /etc/modprobe.conf instead:
>   options scx200_acb base=0x810,0x820
> 
> There might be a way to have the driver auto-detect the correct
> address but I didn't have the time to look into it lately.

	that works. there is a probe function if there is nothing dangerous 
known at this address why it can not be auto probed. any pointers where
to check how to autodetect it ?


--
have fun,
alex

--- drivers/i2c/busses/scx200_acb.c.orig	2006-01-04 02:00:00.000000000 +0200
+++ drivers/i2c/busses/scx200_acb.c	2006-06-12 17:02:02.000000000 +0300
@@ -43,10 +43,11 @@
 MODULE_LICENSE("GPL");
 
 #define MAX_DEVICES 4
-static int base[MAX_DEVICES] = { 0x820, 0x840 };
+static int base[MAX_DEVICES] = { 0x810, 0x820, 0x840 };
 module_param_array(base, int, NULL, 0);
 MODULE_PARM_DESC(base, "Base addresses for the ACCESS.bus controllers");
 
+#define DEBUG
 #ifdef DEBUG
 #define DBG(x...) printk(KERN_DEBUG NAME ": " x)
 #else
@@ -116,6 +117,8 @@
 #define ACBCTL2		(iface->base + 5)
 #define    ACBCTL2_ENABLE	0x01
 
+#define EXPECT_LEN	42
+
 /************************************************************************/
 
 static void scx200_acb_machine(struct scx200_acb_iface *iface, u8 status)
@@ -178,29 +181,50 @@
 		break;
 
 	case state_read:
-		/* Set ACK if receiving the last byte */
-		if (iface->len == 1)
+		if (iface->len == EXPECT_LEN) {
+			/* 
+			 * Len = 1 is valid in the spec
+			 * and we must set ACK since the next
+			 * byte will be the last one
+			 * but how we know to send it
+			 * if we first read length ?
+			 */
+			outb(inb(ACBCTL1) & ~ACBCTL1_ACK, ACBCTL1);
+
+			iface->len= inb(ACBSDA);
+			if (iface->len > 31) {
+				dev_dbg(&iface->adapter.dev, "Invalid read length %d in state %s\n",
+					iface->len, scx200_acb_state_name[iface->state]);
+				errmsg = "Invalid read length";
+				goto error;
+			}
+			/* the first byte in the buffer is expected to be the length */
+			*iface->ptr++ = iface->len;
+			DBG("Got BLOCK READ len=%d\n", iface->len); 
+			break;
+		}
+		/* Set ACK if _next_ byte will be the last one */
+		if (iface->len == 2)
 			outb(inb(ACBCTL1) | ACBCTL1_ACK, ACBCTL1);
 		else
 			outb(inb(ACBCTL1) & ~ACBCTL1_ACK, ACBCTL1);
 
-		*iface->ptr++ = inb(ACBSDA);
-		--iface->len;
-
-		if (iface->len == 0) {
+		if (iface->len == 1) {
 			iface->result = 0;
 			iface->state = state_idle;
 			outb(inb(ACBCTL1) | ACBCTL1_STOP, ACBCTL1);
 		}
 
+		*iface->ptr++ = inb(ACBSDA);
+		--iface->len;
+
 		break;
 
 	case state_write:
-		if (iface->len == 0) {
+		if (iface->len == 1) {
 			iface->result = 0;
 			iface->state = state_idle;
 			outb(inb(ACBCTL1) | ACBCTL1_STOP, ACBCTL1);
-			break;
 		}
 		
 		outb(*iface->ptr++, ACBSDA);
@@ -317,8 +341,13 @@
 	    	buffer = (u8 *)&cur_word;
 		break;
 	case I2C_SMBUS_BLOCK_DATA:
-	    	len = data->block[0];
-	    	buffer = &data->block[1];
+		if (rw == I2C_SMBUS_READ) {
+			len = EXPECT_LEN;
+			buffer = &data->block[0];
+		} else {
+			len = data->block[0];;
+			buffer = &data->block[1];
+		}
 		break;
 	default:
 	    	return -EINVAL;
@@ -373,6 +402,9 @@
 	    	data->word = le16_to_cpu(cur_word);
 
 #ifdef DEBUG
+	if (size == I2C_SMBUS_BLOCK_DATA && rw == I2C_SMBUS_READ) {
+		len = buffer[0] + 1;
+	} 
 	DBG(": transfer done, result: %d", rc);
 	if (buffer) {
 		int i;