Re: [PATCH] I2C block read

[Jean Delvare <khali@linux-fr.org>]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/