Re: [RFC PATCH] i2c: stub: Add support for SMBus block commands
From: Guenter Roeck
Date: Mon Jul 07 2014 - 09:32:10 EST
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@xxxxxxxxxxxx>
---
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)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/