Hi Guenter, On Sun, 6 Jul 2014 20:55:12 -0700, Guenter Roeck wrote: > SMBus block commands are different to I2C block commands since > the returned data is not normally accessible with byte or word > commands on other command offsets. Add linked list of 'block' > commands to support those commands. > > Access mechanism is quite simple: Block commands must be written > before they can be read. The first write selects the block length. > Subsequent writes can be partial. Block read commands always return > the number of bytes selected with the first write.
Very smart, I like it. > Signed-off-by: Guenter Roeck <[email protected]> > --- > Documentation/i2c/i2c-stub | 7 +++- > drivers/i2c/i2c-stub.c | 96 > ++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 98 insertions(+), 5 deletions(-) > > diff --git a/Documentation/i2c/i2c-stub b/Documentation/i2c/i2c-stub > index fa4b669..8a112cc 100644 > --- a/Documentation/i2c/i2c-stub > +++ b/Documentation/i2c/i2c-stub > @@ -2,9 +2,9 @@ MODULE: i2c-stub > > DESCRIPTION: > > -This module is a very simple fake I2C/SMBus driver. It implements five > +This module is a very simple fake I2C/SMBus driver. It implements six > types of SMBus commands: write quick, (r/w) byte, (r/w) byte data, (r/w) > -word data, and (r/w) I2C block data. > +word data, (r/w) I2C block data, and (r/w) SMBus block data. > > You need to provide chip addresses as a module parameter when loading this > driver, which will then only react to SMBus commands to these addresses. > @@ -19,6 +19,9 @@ A pointer register with auto-increment is implemented for > all byte > operations. This allows for continuous byte reads like those supported by > EEPROMs, among others. > > +SMBus block commands must be written to configure an SMBus command for > +SMBus block operations. The first SMBus block write selects the block length. I think you should add valuable parts of the patch description here: "Subsequent writes can be partial. Block read commands always return the number of bytes selected with the first write." > + > The typical use-case is like this: > 1. load this module > 2. use i2cset (from the i2c-tools project) to pre-load some data > diff --git a/drivers/i2c/i2c-stub.c b/drivers/i2c/i2c-stub.c > index 77e4849..e08aa9d 100644 > --- a/drivers/i2c/i2c-stub.c > +++ b/drivers/i2c/i2c-stub.c > @@ -27,11 +27,12 @@ > #include <linux/slab.h> > #include <linux/errno.h> > #include <linux/i2c.h> > +#include <linux/list.h> > > #define MAX_CHIPS 10 > #define STUB_FUNC (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | \ > I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | \ > - I2C_FUNC_SMBUS_I2C_BLOCK) > + I2C_FUNC_SMBUS_I2C_BLOCK | I2C_FUNC_SMBUS_BLOCK_DATA) As discussed earlier, I don't think SMBus block support should be enabled by default, as it can confuse some device drivers. I think we want: #define STUB_FUNC_DEFAULT \ (I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | \ I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA | \ I2C_FUNC_SMBUS_I2C_BLOCK) #define STUB_FUNC_ALL \ (STUB_FUNC_DEFAULT | I2C_FUNC_SMBUS_BLOCK_DATA) static unsigned long functionality = STUB_FUNC_DEFAULT; static u32 stub_func(struct i2c_adapter *adapter) { return STUB_FUNC_ALL & functionality; } Would that be OK with you? You'd simply need to load the driver with functionality=0xffffffff to get the SMBus block support. > > static unsigned short chip_addr[MAX_CHIPS]; > module_param_array(chip_addr, ushort, NULL, S_IRUGO); > @@ -42,14 +43,44 @@ static unsigned long functionality = STUB_FUNC; > module_param(functionality, ulong, S_IRUGO | S_IWUSR); > MODULE_PARM_DESC(functionality, "Override functionality bitfield"); > > +struct smbus_block_data { > + struct list_head node; > + u8 command; > + u8 len; > + u8 block[I2C_SMBUS_BLOCK_MAX]; > +}; > + > struct stub_chip { > u8 pointer; > u16 words[256]; /* Byte operations use the LSB as per SMBus > specification */ > + struct list_head smbus_blocks; > }; > > static struct stub_chip *stub_chips; > > +static struct smbus_block_data *stub_find_block(struct device *dev, > + struct stub_chip *chip, > + u8 command, bool create) > +{ > + struct smbus_block_data *b, *rb = NULL; > + > + list_for_each_entry(b, &chip->smbus_blocks, node) { > + if (b->command == command) { > + rb = b; > + break; > + } > + } > + if (rb == NULL && create) { > + rb = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL); While this is exactly the same, sizeof(*rb) might be more intuitive and make static code analyzers happier too. > + if (rb == NULL) > + return rb; > + rb->command = command; > + list_add(&rb->node, &chip->smbus_blocks); > + } > + return rb; > +} > + > /* Return negative errno on error. */ > static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, unsigned short > flags, > char read_write, u8 command, int size, union i2c_smbus_data *data) > @@ -57,6 +88,7 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, > unsigned short flags, > s32 ret; > int i, len; > struct stub_chip *chip = NULL; > + struct smbus_block_data *b; > > /* Search for the right chip */ > for (i = 0; i < MAX_CHIPS && chip_addr[i]; i++) { > @@ -68,6 +100,8 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, > unsigned short flags, > if (!chip) > return -ENODEV; > > + b = stub_find_block(&adap->dev, chip, command, false); I'm not too happy to see this executed with every command. That's one function call plus one list search, this isn't cheap. I would prefer if this was only executed for actual SMBus block transfers. I think this is possible, see below. > + > switch (size) { > > case I2C_SMBUS_QUICK: > @@ -93,13 +127,20 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, > unsigned short flags, > > case I2C_SMBUS_BYTE_DATA: > if (read_write == I2C_SMBUS_WRITE) { > + if (b) { > + ret = -EINVAL; > + break; > + } Is this really necessary? I very much doubt a device driver would do that in the first place. And if it did, would a real device actually fail? > 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->words[command] & 0xff; > + if (b) > + data->byte = b->len; > + else > + data->byte = chip->words[command] & 0xff; You could avoid this conditional (and the same below in case I2C_SMBUS_WORD_DATA) by writing to chip->words at the same time you write to b->block. Block transfers being rare and reads occurring more frequently than writes, I think the performance benefit is clear. > dev_dbg(&adap->dev, > "smbus byte data - addr 0x%02x, read 0x%02x at > 0x%02x.\n", > addr, data->byte, command); > @@ -111,12 +152,19 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 > addr, unsigned short flags, > > case I2C_SMBUS_WORD_DATA: > if (read_write == I2C_SMBUS_WRITE) { > + if (b) { > + ret = -EINVAL; > + break; > + } > chip->words[command] = data->word; > dev_dbg(&adap->dev, > "smbus word data - addr 0x%02x, wrote 0x%04x at > 0x%02x.\n", > addr, data->word, command); > } else { > - data->word = chip->words[command]; > + if (b) > + data->word = (b->block[0] << 8) | b->len; > + else > + data->word = chip->words[command]; > dev_dbg(&adap->dev, > "smbus word data - addr 0x%02x, read 0x%04x at > 0x%02x.\n", > addr, data->word, command); > @@ -148,6 +196,46 @@ static s32 stub_xfer(struct i2c_adapter *adap, u16 addr, > unsigned short flags, > ret = 0; > break; > > + case I2C_SMBUS_BLOCK_DATA: > + if (read_write == I2C_SMBUS_WRITE) { > + len = data->block[0]; > + if (len == 0 || len > I2C_SMBUS_BLOCK_MAX || > + (b && len > b->len)) { A useful debug message in the latter case might be good to have. > + ret = -EINVAL; > + break; > + } > + if (b == NULL) { > + b = stub_find_block(&adap->dev, chip, command, > + true); > + if (b == NULL) { > + ret = -ENOMEM; > + break; > + } > + /* First write sets block length */ > + b->len = len; > + } > + for (i = 0; i < len; i++) > + b->block[i] = data->block[i + 1]; > + dev_dbg(&adap->dev, > + "smbus block data - addr 0x%02x, wrote %d bytes > at 0x%02x.\n", > + addr, len, command); > + } else { > + if (b == NULL) { > + ret = -EINVAL; I would suggest -EOPNOTSUPP with a useful debug message. > + break; > + } > + len = b->len; > + data->block[0] = len; > + for (i = 0; i < len; i++) > + data->block[i + 1] = b->block[i]; > + dev_dbg(&adap->dev, > + "smbus block data - addr 0x%02x, read %d bytes > at 0x%02x.\n", > + addr, len, command); > + } > + > + ret = 0; > + break; > + > default: > dev_dbg(&adap->dev, "Unsupported I2C/SMBus command\n"); > ret = -EOPNOTSUPP; > @@ -199,6 +287,8 @@ static int __init i2c_stub_init(void) > pr_err("i2c-stub: Out of memory\n"); > return -ENOMEM; > } > + for (i--; i >= 0; i--) > + INIT_LIST_HEAD(&stub_chips[i].smbus_blocks); That works but I have to admit it confused me at first, because traditionally reverse iterations like that are for cleanups on failure paths. I think it might make sense to introduce an additional variable to store the actual number of instantiated chips, so that we can use the regular iteration direction (which I think modern memory controllers can anticipate and optimize.) This would also let us optimize the first test in stub_xfer. But well this can be left as a separate cleanup patch, I'll take care of that (unless you object.) > > ret = i2c_add_adapter(&stub_adapter); > if (ret) -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
