lkml.org 
[lkml]   [2014]   [Jul]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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)
>
>



\
 
 \ /
  Last update: 2014-07-07 16:01    [W:3.324 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site