Messages in this thread Patch in this message | | | Date | Mon, 07 Jul 2014 06:32:02 -0700 | From | Guenter Roeck <> | Subject | Re: [RFC PATCH] i2c: stub: Add support for SMBus block commands |
| |
On 07/07/2014 01:27 AM, Jean Delvare wrote: > 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 <linux@roeck-us.net> >> --- >> 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." > Ok, done.
>> + >> 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. > Yes; it is what I actually had in an earlier version of the document, except for the 0xffffffff part in my test script which is an excellent idea.
>> >> 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. > Makes sense.
>> + 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? > No, it wouldn't fail unless the written length byte is invalid. I don't know if drivers try to do it; it doesn't make much sense, so most likely not.
>> 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. > Makes sense, I'll do that. Great idea.
Question is if I should cover attempts to write a byte or word into block data. I don't think it is worth the effort, as drivers won't usually do that. My take is that we could add it later if really needed. What do you think ?
>> 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. > Ok.
>> + 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. > Ok.
>> + 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.) > Ok with me. I'll leave it alone.
Thanks, Guenter
>> >> ret = i2c_add_adapter(&stub_adapter); >> if (ret) > >
| |