i2c-algo-bit should support I2C_FUNC_I2C

Hi,

Very few drivers seem to support the I2C_FUNC_I2C functionality, is there a reason for that?

I have an I2C bus on my platform constructed from a couple of GPIO lines using the i2c-algo-bit driver. The device on the bus is a DS1307 I2C RTC and the driver for that currently checks for
I2C_FUNC_SMBUS_WORD_DATA | I2C_FUNC_SMBUS_WRITE_BYTE
however the datasheet suggests it is a simple i2c device with none of this smbus stuff, Russell King queried this here
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id 21/1

If I change it to a check for I2C_FUNC_I2C and change the algo-bit driver to declare I2C_FUNC_I2C then the driver continues to work fine.

The ds1307 is described here:
http://www.maxim-ic.com/quick_view2.cfm?qv_pk&88

Given the above, is the following patch appropriate, or is there something about the relationship between i2c and smbus that I don't understand.

Index: 2.6/drivers/i2c/algos/i2c-algo-bit.c
=================================--- 2.6.orig/drivers/i2c/algos/i2c-algo-bit.c 2004-10-20 10:43:57.000000000 +0100
+++ 2.6/drivers/i2c/algos/i2c-algo-bit.c 2004-11-30 09:26:26.706069433 +0000
@@ -511,8 +511,8 @@

static u32 bit_func(struct i2c_adapter *adap)
{
- return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
- I2C_FUNC_PROTOCOL_MANGLING;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+ I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
}

Ian Campbell, Senior Design Engineer
                                        Web: http://www.arcom.com
Arcom, Clifton Road,                    Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom       Phone:  +44 (0)1223 411 200


_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end.  Email to and from Arcom is automatically monitored for operational and lawful business reasons.

This message has been virus scanned by MessageLabs.

No problem! I hope you got it sorted...

I had an inkling that this might be the case, thanks for confirming it.

OK I'll check out smbus_read_i2c_block and see if I can make use of it, I reckon it looks pretty likely.

I started from a ds1307 driver from the 2.4 -rmk ARM Linux patches, forward ported to 2.6 -- I don't know the relation with your 2.4 driver -- I'll check it out though.

PCF and PCA both say "i2c" in the datasheets and don't mention smbus at all, so I reckon I2C_FUNC_I2C would be appropriate. I'll put together a patch tomorrow.

I can't speak for the ite driver.

Very much clearer, thank you... If I have further questions I'll pester you further, no doubt about that :-)

Cheers,
Ian.

Ian Campbell, Senior Design Engineer
                                        Web: http://www.arcom.com
Arcom, Clifton Road,                    Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom       Phone:  +44 (0)1223 411 200


_____________________________________________________________________
The message in this transmission is sent in confidence for the attention of the addressee only and should not be disclosed to any other party. Unauthorised recipients are requested to preserve this confidentiality. Please advise the sender if the addressee is not resident at the receiving end.  Email to and from Arcom is automatically monitored for operational and lawful business reasons.

This message has been virus scanned by MessageLabs.

The following adds I2C_FUNC_I2C to the functionality of the PCF and PCA i2c algorithm drivers.

I also sneaked in a little change which makes the PCA driver a little quieter in the non-debug case...

Index: 2.6/drivers/i2c/algos/i2c-algo-pca.c
=================================--- 2.6.orig/drivers/i2c/algos/i2c-algo-pca.c 2004-12-06 10:38:41.000000000 +0000
+++ 2.6/drivers/i2c/algos/i2c-algo-pca.c 2004-12-07 10:06:22.635455798 +0000
@@ -189,7 +189,7 @@

state = pca_status(adap);
if ( state != 0xF8 ) {
- printk(KERN_ERR DRIVER ": bus is not idle. status is %#04x\n", state );
+ DEB1(KERN_ERR DRIVER ": bus is not idle. status is %#04x\n", state );
/* FIXME: what to do. Force stop ? */ return -EREMOTEIO;
}
@@ -328,7 +328,7 @@

static u32 pca_func(struct i2c_adapter *adap)
{
- return I2C_FUNC_SMBUS_EMUL;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
}

static int pca_init(struct i2c_algo_pca_data *adap)
Index: 2.6/drivers/i2c/algos/i2c-algo-pcf.c
=================================--- 2.6.orig/drivers/i2c/algos/i2c-algo-pcf.c 2004-10-20 10:43:57.000000000 +0100
+++ 2.6/drivers/i2c/algos/i2c-algo-pcf.c 2004-12-07 10:06:05.001239873 +0000
@@ -414,8 +414,8 @@

static u32 pcf_func(struct i2c_adapter *adap)
{
- return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
- I2C_FUNC_PROTOCOL_MANGLING;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+ I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
}

/* -----exported algorithm data: ------------------------------------- */

Ian Campbell, Senior Design Engineer
                                        Web: http://www.arcom.com
Arcom, Clifton Road,                    Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom       Phone:  +44 (0)1223 411 200

I think it can be considered a debug statement since I am reasonably sure that it is a situation which cannot arise. I've only ever seen the error with a pre-release of a slave part on the bus. The slave's i2c implementation was fairly buggy, it ignored stop bits and hung the bus if you sent it a duff command. The next rev of this particular slave device shouldn't have this problem and I certainly wouldn't expect it in released silicon.

If it turns out that in practise (with production slave devices) we can find that we hit this check then it would make sense to loop for a bit until the bus becomes idle or we timeout. However I'd rather see it happen with a production slave device before doing that.

I've changed it to dev_dbg.

Index: 2.6/drivers/i2c/algos/i2c-algo-pca.c
=================================--- 2.6.orig/drivers/i2c/algos/i2c-algo-pca.c 2004-12-06 10:38:41.000000000 +0000
+++ 2.6/drivers/i2c/algos/i2c-algo-pca.c 2004-12-07 11:14:08.669738602 +0000
@@ -189,7 +189,7 @@

state = pca_status(adap);
if ( state != 0xF8 ) {
- printk(KERN_ERR DRIVER ": bus is not idle. status is %#04x\n", state );
+ dev_dbg(&i2c_adap->dev, "bus is not idle. status is %#04x\n", state );
/* FIXME: what to do. Force stop ? */ return -EREMOTEIO;
}
@@ -328,7 +328,7 @@

static u32 pca_func(struct i2c_adapter *adap)
{
- return I2C_FUNC_SMBUS_EMUL;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
}

static int pca_init(struct i2c_algo_pca_data *adap)
Index: 2.6/drivers/i2c/algos/i2c-algo-pcf.c
=================================--- 2.6.orig/drivers/i2c/algos/i2c-algo-pcf.c 2004-10-20 10:43:57.000000000 +0100
+++ 2.6/drivers/i2c/algos/i2c-algo-pcf.c 2004-12-07 10:14:58.000000000 +0000
@@ -414,8 +414,8 @@

static u32 pcf_func(struct i2c_adapter *adap)
{
- return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
- I2C_FUNC_PROTOCOL_MANGLING;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+ I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
}

/* -----exported algorithm data: ------------------------------------- */

Ian Campbell, Senior Design Engineer
                                        Web: http://www.arcom.com
Arcom, Clifton Road,                    Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom       Phone:  +44 (0)1223 411 200

I've checked it out and I don't think they are similar enough, however I would appreciate your input.

The ds1338 (same i2c interface as ds1307 and I just happen to have the data sheet in front of me) expects a write (slave receiving) to look like this (I hope my notation makes sense...)

S <slave addr> W ACK <address(n)> A <data(n)> A <data(n+1)> A <....> A P

Where address(n) sets the register address to start writing at. There are then as many data bytes as you like, each acknowledged, followed by the stop bit.

SMBUS block write is

S <slave_addr> W A <command> A <count> A <data(1)> A <....> <data(count)> A P

The SMBUS <command> could be considered to be the ds1338 <address(n)> but then there is an extra byte in the smbus for the count.

The read commands follow the same basic scheme in both cases.

I can't see another SMBUS transfer that would fit, unless there is some sort of 'repeated send byte' that I haven't spotted.

On a separate thread -- I had a thought that perhaps I2C_FUNC_I2C should include the SMBUS_EMUL definitions since (I think you said) that any i2c device can emulate smbus...

Ian.

Ian Campbell, Senior Design Engineer
                                        Web: http://www.arcom.com
Arcom, Clifton Road,                    Direct: +44 (0)1223 403 465
Cambridge CB1 7EA, United Kingdom       Phone:  +44 (0)1223 411 200

Hi Ian,

Sorry for the delay, I have been encountering severe mail problems and lost 6 days worth of data, including your latest post about I2C_FUNC_I2C in i2c-algo-bit. I am copying your questions from the mailing-list archive.

Yes, most bus drivers are for SMBus, not I2C, masters. SMBus is a subset of I2C. These SMBus master are fully I2C-capable, although in most cases it doesn't matter. Most chip drivers are for SMBus clients as well. Almost all hardware monitoring chips are SMBus devices. So it's not surprising not to see I2C_FUNC_I2C widely used.

First, note that all SMBus commands are valid I2C transfers. In fact, the SMBus specification doesn't do much except put names on specific I2C transfers, and sometimes give meanings to the data being transfered. As a result, "I2C" clients, although not specifically SMBus-compatible, may enjoy SMBus commands such as SMBUS_READ_BYTE, SMBUS_READ_BYTE_DATA and SMBUS_WRITE_BYTE_DATA. One typical example of that are EEPROMs and the eeprom driver. It relies on i2c_smbus_write_byte and i2c_smbus_read_byte (BTW I just noticed that it doesn't properly check for these functionalities... have to fix that) because it's so much easier to call these standard functions than rewrite I2C code manually. Also, this let us access the EEPROMs on SMBus (non-I2C) busses.

Now, note that Russell is not quite correct in is assertion: "Do we really require SMBUS functionality, or is i2c functionality sufficient?" It's actually the other way around. SMBus puts restrictions on what a valid I2C transfer is. "Do we need the full I2C functionality or is the SMBus subset sufficient?" would make more sense.

As for the exact functionality you need to check, let's just see how you access the bus. As far as I can see (providing that the code below Russell's comments is still valid), you rely on:
* i2c_smbus_write_byte_data
* i2c_smbus_read_byte_data
* i2c_transfer

So yo *need* to check for the availability of I2C_FUNC_SMBUS_BYTE_DATA on the adapter (which is part of I2C_FUNC_SMBUS_EMUL so any bus driver using that, including any relying on i2c-algo-bit, will work with your client driver) for the first two. And you also need to check for I2C_FUNC_I2C for the third one.

Of course, any adapter with I2C_FUNC_I2C will be able to do SMBus byte data transfers, but since you do not use i2c_transfer to do them, you need to check the functionality separately (I think).

Also, I think that what you do with i2c_transfer is similar to I2C_FUNC_SMBUS_READ_I2C_BLOCK, which is supported by some SMBus (non-I2C) masters. If you convert your code to use i2c_smbus_read_i2c_block_data, then you don't rely on the full I2C capatbilities of the bus, which means that your chip driver will be useable on more plateforms. That said, note that this feature is unimplemented on most SMBus master drivers as of now, and broken on a number of others (but I guess we would start paying more attention to them if there were more users for this function).

As a side note... Did you know that we have a DS1307 driver for Linux 2.4 already?
http://www2.lm-sensors.nu/~lm78/cvs/browse.cgi/lm_sensors2/kernel/chips/ds1307.c
Looks like you started from scratch... Maybe you want to compare codes.

You are right that i2c-algo-bit should declare itself I2C_FUNC_I2C capable. I even think that every bus being I2C_FUNC_SMBUS_EMUL capable is very likely to be I2C_FUNC_I2C capable. This means that other algorithms (ite, pcf, maybe pca) could most probably be declared I2C_FUNC_I2C capable as well. Can anyone confirm?

I admit that the relationship between I2C and SMBus is somewhat tricky, it took me some time to get it and even then I am sometimes not sure to understand exactly what implies what ;) So we cannot blame you for not getting it at first. I hope I helped make things a little clearer. If not I welcome questions.

Whether or not you change your code to use SMBus only in your driver to make it more widely useable, your patch to i2c-algo-bit is valid, I am signing it too and will apply it to the 2.4 version of the driver as well.

Greg, please apply, thanks.

Index: 2.6/drivers/i2c/algos/i2c-algo-bit.c
=================================--- 2.6.orig/drivers/i2c/algos/i2c-algo-bit.c 2004-10-20 10:43:57.000000000 +0100
+++ 2.6/drivers/i2c/algos/i2c-algo-bit.c 2004-11-30 09:26:26.706069433 +0000
@@ -511,8 +511,8 @@

static u32 bit_func(struct i2c_adapter *adap)
{
- return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR |
- I2C_FUNC_PROTOCOL_MANGLING;
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+ I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
}

Jean Delvare
http://khali.linux-fr.org/

Hi Ian,

Doesn't look OK to me. Either this is an error and it has to be displayed regardless of DEBUG being enabled or not, or it is a debugging info and it should use KERN_DEBUG instead of KERN_ERR. In the latter case, I can already hear Greg suggest you should use pr_debug or even dev_dbg instead of printk.

(Otherwise I am fine with the I2C_FUNC_I2C additions, even if I did not look at the datasheets.)

Thanks,
--
Jean Delvare

Applied, thanks.

greg k-h

Applied, thanks.

greg k-h

The I2C_FUNC_SMBUS_{READ|WRITE}_I2C_BLOCK functionalities are "true" i2c block transfers, not smbus transfers; don't let the name fool you.

I2C_FUNC_I2C was defined at the very beginnning (and documented in doc/functionality in the i2c tree) but never used by anything in the i2c or sensors tree; it looks pretty much forgotten. So chip drivers looking for that functionality bit won't find it anywhere.

I think the best way to fix it is to add I2C_FUNC_I2C to the I2C_FUNC_SMBUS_EMUL bitmask in i2c.h. Then everyone should be happy.
comments?

Hi Ian,

Look at the I2C block reads and writes. Theses are similar to the SMBus writes but without the count byte.

For some reason these are called i2c_smbus_read/write_i2c_block_data but these are NOT SMBus transfers (as Mark Studebaker underlined elsewhere).

We can't because FUNC_SMBUS_EMUL only makes sense for bus drivers while FUNC_I2C makes sense for both bus and chip drivers. We could do it the other way around though as Mark Studebaker suggested. Or we can leave things the way they are. FUNC_SMBUS_EMUL is really a shortcut to help chip drivers verify if a bus driver supports one given SMBus command (and as such its definition is likely to change in the future). FUNC_I2C means that any arbitrary I2C transfer can be achieved. Although a master will support both or none (as far as I can see) they are separate things. I think things are clearer if we keep them separate. It shouldn't be hard to add I2C_FUNC_I2C to the busses which support it, they aren't that many, really.

Thanks,
--
Jean