Re: [RFC PATCH] i2c: stub: Add support for SMBus block commands

[Jean Delvare]

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."

> +
>  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/