i2c/ smbus question

i2c/ smbus question

[Benjamin Herrenschmidt]

Hi !

Can you confirm the difference between writing a block of data with
I2C_SMBUS_BLOCK_DATA vs I2C_SMBUS_I2C_BLOCK_DATA on the wire ? It's my
understanding that the former will actually send the lenght byte on the
wire before the data while the later won't, though they both send a
subaddress.

I'm completely rewriting the powermac i2c support (consolidating all
busses behind a low level layer that I need to use in circumstances
where the linux i2c layer isn't useable, and with a single driver in
drivers/i2c/busses/* replacing i2c-keywest.c and i2c-pmac-smu.c) and I
think I'm hitting a problem where i2c-keywest didn't implement
I2C_SMBUS_BLOCK_DATA properly (didn't send the lenght byte) and some
drivers (our sound drivers) rely on that behaviour (that's fine, I can
fix them too, I just want to make sure I understand what the semantic
should be).

I'm a bit surprised that there seem to be no wrapper for writing with
I2C_SMBUS_I2C_BLOCK_DATA, only for reading, in i2c-core.c since it
appears to me that it's the most common one, at least for all devices
I've dealt with so far (mostly sound & clock chips in addition to
sensors)...

Cheers,
Ben.

Re: i2c/ smbus question

[Jean Delvare]

Hi Benjamin,

On January 7th, 2006, Benjamin Herrenschmidt asked:
> Can you confirm the difference between writing a block of data with
> I2C_SMBUS_BLOCK_DATA vs I2C_SMBUS_I2C_BLOCK_DATA on the wire ? It's my
> understanding that the former will actually send the length byte on the
> wire before the data while the later won't, though they both send a
> subaddress.

I confirm you are correct.

> I'm completely rewriting the powermac i2c support (consolidating all
> busses behind a low level layer that I need to use in circumstances
> where the linux i2c layer isn't useable, and with a single driver in
> drivers/i2c/busses/* replacing i2c-keywest.c and i2c-pmac-smu.c) and I
> think I'm hitting a problem where i2c-keywest didn't implement
> I2C_SMBUS_BLOCK_DATA properly (didn't send the length byte) and some
> drivers (our sound drivers) rely on that behaviour (that's fine, I can
> fix them too, I just want to make sure I understand what the semantic
> should be).

I just took a quick look at i2c-keywest.c and you seem to right: the
driver supposedly implements SMBus block transfers
(I2C_SMBUS_BLOCK_DATA) but the actual implementation pretty much looks
like I2C block transfers (I2C_SMBUS_I2C_BLOCK_DATA). Good catch.

> I'm a bit surprised that there seem to be no wrapper for writing with
> I2C_SMBUS_I2C_BLOCK_DATA, only for reading, in i2c-core.c since it
> appears to me that it's the most common one, at least for all devices
> I've dealt with so far (mostly sound & clock chips in addition to
> sensors)...

I suspect that these drivers which do I2C block writes do so by calling
i2c_master_send (or even i2c_transfer) directly, rather than relying on
the SMBus compatibility layer.

The i2c_smbus_write_i2c_block_data wrapper function used to be defined,
but was removed in 2.6.10-rc2 together with a couple other similar
wrappers [1] on request by Adrian Bunk, the reason being that they had
no user back then. I was a bit reluctant at first, but we finally agreed
with Adrian to remove the functions, and to reintroduce them later if
they were ever needed.

So, if you need i2c_smbus_write_i2c_block_data(), we can easily
resurrect it. See the patch below. I made the new version a bit faster
(I hope) than the original by using memcpy, please confirm it works for
you.

[1] http://jdelvare.net2.nerim.net/quian/2.6-wrc/diff.php?patch=patch-2.6.10-rc1-rc2.bz2&file=include/linux/i2c.h
    http://jdelvare.net2.nerim.net/quian/2.6-wrc/diff.php?patch=patch-2.6.10-rc1-rc2.bz2&file=drivers/i2c/i2c-core.c

* * * * *

Resurrect i2c_smbus_write_i2c_block_data.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/i2c/i2c-core.c |   15 +++++++++++++++
 include/linux/i2c.h    |    3 +++
 2 files changed, 18 insertions(+)

--- linux-2.6.15-git.orig/drivers/i2c/i2c-core.c	2006-01-08 10:55:58.000000000 +0100
+++ linux-2.6.15-git/drivers/i2c/i2c-core.c	2006-01-08 11:17:57.000000000 +0100
@@ -948,6 +948,20 @@
 	}
 }
 
+s32 i2c_smbus_write_i2c_block_data(struct i2c_client *client, u8 command,
+				   u8 length, u8 *values)
+{
+	union i2c_smbus_data data;
+
+	if (length > I2C_SMBUS_BLOCK_MAX)
+		length = I2C_SMBUS_BLOCK_MAX;
+	data.block[0] = length;
+	memcpy(data.block + 1, values, length);
+	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			      I2C_SMBUS_WRITE, command,
+			      I2C_SMBUS_I2C_BLOCK_DATA, &data);
+}
+
 /* Simulate a SMBus command using the i2c protocol 
    No checking of parameters is done!  */
 static s32 i2c_smbus_xfer_emulated(struct i2c_adapter * adapter, u16 addr, 
@@ -1152,6 +1166,7 @@
 EXPORT_SYMBOL(i2c_smbus_write_word_data);
 EXPORT_SYMBOL(i2c_smbus_write_block_data);
 EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data);
+EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
 
 MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>");
 MODULE_DESCRIPTION("I2C-Bus main module");
--- linux-2.6.15-git.orig/include/linux/i2c.h	2006-01-08 10:56:08.000000000 +0100
+++ linux-2.6.15-git/include/linux/i2c.h	2006-01-08 11:02:00.000000000 +0100
@@ -100,6 +100,9 @@
 /* Returns the number of read bytes */
 extern s32 i2c_smbus_read_i2c_block_data(struct i2c_client * client,
 					 u8 command, u8 *values);
+extern s32 i2c_smbus_write_i2c_block_data(struct i2c_client * client,
+					  u8 command, u8 length,
+					  u8 *values);
 
 /*
  * A driver is capable of handling one or more physical devices present on

-- 
Jean Delvare

Re: i2c/ smbus question

[Benjamin Herrenschmidt]

On Sun, 2006-01-08 at 11:30 +0100, Jean Delvare wrote:

> I suspect that these drivers which do I2C block writes do so by calling
> i2c_master_send (or even i2c_transfer) directly, rather than relying on
> the SMBus compatibility layer.

Ok. Well, it's much more simple for me to implement smbus (with a few extensions
like block transfer) than the low level i2c at the host driver level.
In order to implement subaddress access with repeat start (very common in pretty
much everything nowadays), I need two messages. However, my low level hardware
can't implement everything that can be done with multiple messages. Thus
implementing the stuff using i2c_xfer needs a lot of test & validation all over
the place to coerce those 2 messages into a subaddress + data setup that my low
level hw understands. Implementing only smbus is simpler and fits most of my needs.
 
> The i2c_smbus_write_i2c_block_data wrapper function used to be defined,
> but was removed in 2.6.10-rc2 together with a couple other similar
> wrappers [1] on request by Adrian Bunk, the reason being that they had
> no user back then. I was a bit reluctant at first, but we finally agreed
> with Adrian to remove the functions, and to reintroduce them later if
> they were ever needed.

I find that weird but heh...

> So, if you need i2c_smbus_write_i2c_block_data(), we can easily
> resurrect it. See the patch below. I made the new version a bit faster
> (I hope) than the original by using memcpy, please confirm it works for
> you.

Ok, I'll test with those, I did an equivalent local to my sound drivers
but a wrapper in the i2c layer is probably better. Thanks.

Re: i2c/ smbus question

[Benjamin Herrenschmidt]

On Sun, 2006-01-08 at 11:30 +0100, Jean Delvare wrote:

> The i2c_smbus_write_i2c_block_data wrapper function used to be defined,
> but was removed in 2.6.10-rc2 together with a couple other similar
> wrappers [1] on request by Adrian Bunk, the reason being that they had
> no user back then. I was a bit reluctant at first, but we finally agreed
> with Adrian to remove the functions, and to reintroduce them later if
> they were ever needed.

Argh... Adrian, sometimes I hate you ;-)

> So, if you need i2c_smbus_write_i2c_block_data(), we can easily
> resurrect it. See the patch below. I made the new version a bit faster
> (I hope) than the original by using memcpy, please confirm it works for
> you.

Seems to work. Greg, would you mean boucing that to Linus asap (if you
are ok with it of course) ? I have a pile of patch about to hit him via
the powerpc merge git tree and I'll "fix" some of the mac drivers in
there to use that wrapper, so without that patch, g5 won't build ;)

Thanks !

Ben.

Re: i2c/ smbus question

[Greg KH]

On Mon, Jan 09, 2006 at 09:58:22AM +1100, Benjamin Herrenschmidt wrote:
> On Sun, 2006-01-08 at 11:30 +0100, Jean Delvare wrote:
> 
> > The i2c_smbus_write_i2c_block_data wrapper function used to be defined,
> > but was removed in 2.6.10-rc2 together with a couple other similar
> > wrappers [1] on request by Adrian Bunk, the reason being that they had
> > no user back then. I was a bit reluctant at first, but we finally agreed
> > with Adrian to remove the functions, and to reintroduce them later if
> > they were ever needed.
> 
> Argh... Adrian, sometimes I hate you ;-)
> 
> > So, if you need i2c_smbus_write_i2c_block_data(), we can easily
> > resurrect it. See the patch below. I made the new version a bit faster
> > (I hope) than the original by using memcpy, please confirm it works for
> > you.
> 
> Seems to work. Greg, would you mean boucing that to Linus asap (if you
> are ok with it of course) ? I have a pile of patch about to hit him via
> the powerpc merge git tree and I'll "fix" some of the mac drivers in
> there to use that wrapper, so without that patch, g5 won't build ;)

Sure, Jean, care to resend it to me, as it's now lost in my archives
somewhere :)

thanks,

greg k-h

Re: i2c/ smbus question

[Benjamin Herrenschmidt]

Resurrect i2c_smbus_write_i2c_block_data.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
> Sure, Jean, care to resend it to me, as it's now lost in my archives
> somewhere :)

here it is :-)

 drivers/i2c/i2c-core.c |   15 +++++++++++++++
 include/linux/i2c.h    |    3 +++
 2 files changed, 18 insertions(+)

--- linux-2.6.15-git.orig/drivers/i2c/i2c-core.c	2006-01-08 10:55:58.000000000 +0100
+++ linux-2.6.15-git/drivers/i2c/i2c-core.c	2006-01-08 11:17:57.000000000 +0100
@@ -948,6 +948,20 @@
 	}
 }
 
+s32 i2c_smbus_write_i2c_block_data(struct i2c_client *client, u8 command,
+				   u8 length, u8 *values)
+{
+	union i2c_smbus_data data;
+
+	if (length > I2C_SMBUS_BLOCK_MAX)
+		length = I2C_SMBUS_BLOCK_MAX;
+	data.block[0] = length;
+	memcpy(data.block + 1, values, length);
+	return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			      I2C_SMBUS_WRITE, command,
+			      I2C_SMBUS_I2C_BLOCK_DATA, &data);
+}
+
 /* Simulate a SMBus command using the i2c protocol 
    No checking of parameters is done!  */
 static s32 i2c_smbus_xfer_emulated(struct i2c_adapter * adapter, u16 addr, 
@@ -1152,6 +1166,7 @@
 EXPORT_SYMBOL(i2c_smbus_write_word_data);
 EXPORT_SYMBOL(i2c_smbus_write_block_data);
 EXPORT_SYMBOL(i2c_smbus_read_i2c_block_data);
+EXPORT_SYMBOL(i2c_smbus_write_i2c_block_data);
 
 MODULE_AUTHOR("Simon G. Vogl <simon@tk.uni-linz.ac.at>");
 MODULE_DESCRIPTION("I2C-Bus main module");
--- linux-2.6.15-git.orig/include/linux/i2c.h	2006-01-08 10:56:08.000000000 +0100
+++ linux-2.6.15-git/include/linux/i2c.h	2006-01-08 11:02:00.000000000 +0100
@@ -100,6 +100,9 @@
 /* Returns the number of read bytes */
 extern s32 i2c_smbus_read_i2c_block_data(struct i2c_client * client,
 					 u8 command, u8 *values);
+extern s32 i2c_smbus_write_i2c_block_data(struct i2c_client * client,
+					  u8 command, u8 length,
+					  u8 *values);
 
 /*
  * A driver is capable of handling one or more physical devices present on

-- 
Jean Delvare