[i2c] [PATCH] i2c-stub: Use a single array for byte and word operations

Jean Delvare khali at linux-fr.org
Tue Nov 20 19:11:51 CET 2007


Hi Mark,

On Sun, 18 Nov 2007 14:53:57 -0500, Mark M. Hoffman wrote:
> Hi Jean:
> 
> * Jean Delvare <khali at linux-fr.org> [2007-11-05 17:54:16 +0100]:
> > This mimics the behavior of actual SMBus chips better.
> > 
> > Signed-off-by: Jean Delvare <khali at linux-fr.org>
> > Cc: Mark M. Hoffman <mhoffman at lightlink.com>
> > ---
> > Mark, any objection to this change?
> > 
> >  Documentation/i2c/i2c-stub    |    3 ---
> >  drivers/i2c/busses/i2c-stub.c |   15 ++++++++-------
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> > 
> > --- linux-2.6.24-rc1.orig/Documentation/i2c/i2c-stub	2007-11-05 09:52:39.000000000 +0100
> > +++ linux-2.6.24-rc1/Documentation/i2c/i2c-stub	2007-11-05 16:32:17.000000000 +0100
> > @@ -35,9 +35,6 @@ int chip_addr[10]:
> >  
> >  CAVEATS:
> >  
> > -There are independent arrays for byte/data and word/data commands.  Depending
> > -on if/how a target driver mixes them, you'll need to be careful.
> > -
> >  If your target driver polls some byte or word waiting for it to change, the
> >  stub could lock it up.  Use i2cset to unlock it.
> >  
> > --- linux-2.6.24-rc1.orig/drivers/i2c/busses/i2c-stub.c	2007-10-24 09:59:28.000000000 +0200
> > +++ linux-2.6.24-rc1/drivers/i2c/busses/i2c-stub.c	2007-11-05 16:41:06.000000000 +0100
> > @@ -1,8 +1,8 @@
> >  /*
> > -    i2c-stub.c - Part of lm_sensors, Linux kernel modules for hardware
> > -              monitoring
> > +    i2c-stub.c - I2C/SMBus chip emulator
> >  
> >      Copyright (c) 2004 Mark M. Hoffman <mhoffman at lightlink.com>
> > +    Copyright (C) 2007 Jean Delvare <khali at linux-fr.org>
> >  
> >      This program is free software; you can redistribute it and/or modify
> >      it under the terms of the GNU General Public License as published by
> > @@ -37,8 +37,8 @@ MODULE_PARM_DESC(chip_addr,
> >  
> >  struct stub_chip {
> >  	u8 pointer;
> > -	u8 bytes[256];
> > -	u16 words[256];
> > +	u16 words[256];		/* Byte operations use the LSB as per SMBus
> > +				   specification */
> >  };
> >  
> >  static struct stub_chip *stub_chips;
> > @@ -75,7 +75,7 @@ static s32 stub_xfer(struct i2c_adapter 
> >  					"wrote 0x%02x.\n",
> >  					addr, command);
> >  		} else {
> > -			data->byte = chip->bytes[chip->pointer++];
> > +			data->byte = chip->words[chip->pointer++] & 0xff;
> >  			dev_dbg(&adap->dev, "smbus byte - addr 0x%02x, "
> >  					"read  0x%02x.\n",
> >  					addr, data->byte);
> > @@ -86,12 +86,13 @@ static s32 stub_xfer(struct i2c_adapter 
> >  
> >  	case I2C_SMBUS_BYTE_DATA:
> >  		if (read_write == I2C_SMBUS_WRITE) {
> > -			chip->bytes[command] = data->byte;
> > +			chip->words[command] &= 0xff00;
> > +			chip->words[command] |= data->byte;
> >  			dev_dbg(&adap->dev, "smbus byte data - addr 0x%02x, "
> >  					"wrote 0x%02x at 0x%02x.\n",
> >  					addr, data->byte, command);
> >  		} else {
> > -			data->byte = chip->bytes[command];
> > +			data->byte = chip->words[command] & 0xff;
> >  			dev_dbg(&adap->dev, "smbus byte data - addr 0x%02x, "
> >  					"read  0x%02x at 0x%02x.\n",
> >  					addr, data->byte, command);
> > 
> 
> I'm not sure about this.  Aren't there at least some chips where...
> 
> 	i2c_smbus_read_word_data(addr) == 
> 		(i2c_smbus_read_word_byte(addr) << 8) + 
> 		i2c_smbus_read_byte_data(addr+1);

Assuming that you mean i2c_smbus_read_byte_data(addr) instead of
i2c_smbus_read_word_byte(addr), yes, in particular EEPROMs work like
this. Also some hardware monitoring chips (e.g. W83782D) but usually
only for a specified range of register addresses.

> The above patch does not allow it.  The original code does not enforce
> it either but at least it's possible to fake it by writing into both the
> word and read arrays for that address.

Only if the driver only reads from these addresses... I don't think
that my patch changes this. It is still possible to write word values
such that mixed byte reads and word reads will work. It is even easier
than before: you only have to write word values, instead of both byte
and word values. Or am I missing something?

> Perhaps what is really wanted is a module parameter which determines the
> relationship between word and byte data, where one choice represents
> your patch above, one represents the relation I wrote, and one maintains
> two independent arrays as in the original - maybe harder to use, but
> it's the most flexible.  What do you think?

I agree that a module parameter to select the mode would be nice to
have. However, I don't think that there is any value in the "independent
arrays" mode. An I2C chip cannot differentiate between a byte read and
a word read at first, so by definition, for every given address, the
first half of the word value has to match the byte value. The only
thing that chips can handle differently is the second half of the word
value. It is either the value of the next address (EEPROM style) or a
separate value (real 16-bit registers, LM75 style). I can't think of
any other (sane) possibility.

-- 
Jean Delvare



More information about the i2c mailing list