[i2c] [RFC/PATCH] Add I2C_M_STOP flag

[Jean Delvare]

Hi Trent,

On Mon, 21 May 2007 17:45:32 -0700 (PDT), Trent Piepho wrote:
> A problem came up with a certain i2c client used in the DVB card.  A typical i2c
> transaction to read a register with most device looks like this:
> 
> S Addr Wr [A] Comm [A] S Addr Rd [A] [Data] NA P
> 
> That would read one byte from the register Comm.  It's the same as the smbus
> read byte data command.  The stv0297 doesn't want a command like this, it needs
> something slightly different:
> 
> S Addr Wr [A] Comm [A] P S Addr Rd [A] [Data] NA P
> 
> It doesn't want a repeated start, instead it wants a stop and then a start.
> 
> Now, one can generate a sequence like this already, with code like this:
> char comm = 0x42, data;
> struct i2c_msg msgs[2] = {
> 	{ .addr = 0x61, .buf = &comm, .len = 1, .flags = 0 },
> 	{ .addr = 0x61, .buf = &data, .len = 1, .flags = I2C_M_RD } };
> i2c_transfer(adap, msgs, 1);
> i2c_transfer(adap, msgs+1, 1);

Or, easier and more portable:

	i2c_smbus_write_byte(client, comm);
	i2c_smbus_read_byte(client, data);

> The problem with this is that it's now two transactions, and not one single
> atomic transaction.  It's possible for another message by another process
> (different driver, i2c-dev, etc.) to slip in between the write message and
> read message.

Two clients cannot be created at the same address on a given i2c
adapter, so you should be safe. The only exception is indeed i2c-dev,
because the address can be forcibly used even if already busy, and
because i2c-dev itself doesn't register its client (which I consider a
bug). But people using i2c-dev in force mode are supposed to know what
they are doing.

> To solve this, my patch introduces a new flag I2C_M_STOP which tells the
> i2c bus driver to send a stop after the flagged message.  This way one can
> send a single transaction with i2c_transfer() that contains multiple stop
> conditions.  None of the i2c bus drivers need to care about I2C_M_STOP, it
> is handled by the code in i2c_transfer(), which splits the message on STOP
> boundaries into multiple calls to master_xfer(), while the
> i2c_adapter->bus_lock mutex ensures the entire transaction is atomic.
> 
> This allows the follow code to send the proper command in one transaction:
> char comm = 0x42, data;
> struct i2c_msg msgs[2] = {
>         { .addr = 0x61, .buf = &comm, .len = 1, .flags = I2C_M_STOP },
>         { .addr = 0x61, .buf = &data, .len = 1, .flags = I2C_M_RD } };
> i2c_transfer(adap, msgs, 2);
> 
> 
> -----------------------------------------------------------------------------
> diff -r 56b4c3e8f350 drivers/i2c/i2c-core.c
> --- a/drivers/i2c/i2c-core.c	Sat May 19 05:00:32 2007 +0000
> +++ b/drivers/i2c/i2c-core.c	Sat May 19 17:35:19 2007 -0700
> @@ -862,6 +862,7 @@ int i2c_transfer(struct i2c_adapter * ad
>  	int ret;
> 
>  	if (adap->algo->master_xfer) {
> +		int n, total = 0;
>  #ifdef DEBUG
>  		for (ret = 0; ret < num; ret++) {
>  			dev_dbg(&adap->dev, "master_xfer[%d] %c, addr=0x%02x, "
> @@ -872,10 +873,28 @@ int i2c_transfer(struct i2c_adapter * ad
>  #endif
> 
>  		mutex_lock_nested(&adap->bus_lock, adap->level);
> -		ret = adap->algo->master_xfer(adap,msgs,num);
> +		for (n=1; n<=num; n++) {
> +			/* xfer the message(s) if we want a stop or if
> +			   it's the last message. */
> +			if (n == num || (msgs[n-1].flags & I2C_M_STOP)) {
> +				ret = adap->algo->master_xfer(adap,msgs,n);
> +
> +				total += ret;
> +				if (ret < n) {
> +					/* Return error code, if any */
> +					if (ret < 0)
> +						total = ret;
> +					break;
> +				}
> +
> +				/* Process any remaining messages */
> +				msgs += n;
> +				num -= n;
> +				n = 0;
> +			}
> +		}
>  		mutex_unlock(&adap->bus_lock);
> -
> -		return ret;
> +		return total;
>  	} else {
>  		dev_dbg(&adap->dev, "I2C level transfers not supported\n");
>  		return -ENOSYS;
> diff -r 56b4c3e8f350 include/linux/i2c.h
> --- a/include/linux/i2c.h	Sat May 19 05:00:32 2007 +0000
> +++ b/include/linux/i2c.h	Sat May 19 17:35:03 2007 -0700
> @@ -448,6 +448,7 @@ struct i2c_msg {
>  #define I2C_M_IGNORE_NAK	0x1000
>  #define I2C_M_NO_RD_ACK		0x0800
>  #define I2C_M_RECV_LEN		0x0400 /* length will be first received byte */
> +#define I2C_M_STOP		0x0200 /* send STOP condition after this msg */
>  	__u16 len;		/* msg length				*/
>  	__u8 *buf;		/* pointer to msg data			*/
>  };

I don't see how this is necessary. Once you have a client at a given
address, your driver has full control on it, so you can define and use
your own mutex to guarantee that device accesses are serialized. All
I2C-based hardware monitoring drivers do that for example.

-- 
Jean Delvare