[U-Boot] [PATCH v3 01/10] dm: i2c: Add a uclass for I2C

[Masahiro Yamada]

Hi Simon,


My review is still under way,
but I have some comments below:




On Mon, 24 Nov 2014 11:57:15 -0700
Simon Glass <sjg at chromium.org> wrote:

> +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset,
> +			     uint8_t offset_buf[], struct i2c_msg *msg)
> +{
> +	if (!chip->offset_len)
> +		return false;
> +	msg->addr = chip->chip_addr;
> +	msg->flags = chip->flags;
> +	msg->len = chip->offset_len;
> +	msg->buf = offset_buf;

You directly copy
from  (struct dm_i2c_chip *)->flags
to  (struct i2c_msg *)->flags.

But you define completely different flags for them:
  DM_I2C_CHIP_10BIT is defined as 0x1.
  I2C_M_TEN  is defined as 0x10.

It would not work.



> +
> +static int i2c_read_bytewise(struct udevice *dev, uint offset,
> +			     const uint8_t *buffer, int len)
> +{
> +	struct dm_i2c_chip *chip = dev_get_parentdata(dev);
> +	struct udevice *bus = dev_get_parent(dev);
> +	struct dm_i2c_ops *ops = i2c_get_ops(bus);
> +	struct i2c_msg msg[1];
> +	uint8_t buf[5];
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < len; i++) {
> +		i2c_setup_offset(chip, offset, buf, msg);
> +		msg->len++;
> +		buf[chip->offset_len] = buffer[i];
> +
> +		ret = ops->xfer(bus, msg, 1);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

I could not understand how this works.
It seems to send only write transactions.



> +
> +static int i2c_bind_driver(struct udevice *bus, uint chip_addr,
> +			   struct udevice **devp)
> +{
> +	struct dm_i2c_chip *chip;
> +	char name[30], *str;
> +	struct udevice *dev;
> +	int ret;
> +
> +	snprintf(name, sizeof(name), "generic_%x", chip_addr);
> +	str = strdup(name);
> +	ret = device_bind_driver(bus, "i2c_generic_drv", str, &dev);
> +	debug("%s:  device_bind_driver: ret=%d\n", __func__, ret);
> +	if (ret)
> +		goto err_bind;
> +
> +	/* Tell the device what we know about it */
> +	chip = calloc(1, sizeof(struct dm_i2c_chip));
> +	if (!chip) {
> +		ret = -ENOMEM;
> +		goto err_mem;
> +	}
> +	chip->chip_addr = chip_addr;
> +	chip->offset_len = 1;	/* we assume */
> +	ret = device_probe_child(dev, chip);
> +	debug("%s:  device_probe_child: ret=%d\n", __func__, ret);
> +	free(chip);


Why do you need calloc() & free() here?
I think you can use the stack area for "struct dm_i2c_chip chip;"








> +
> +UCLASS_DRIVER(i2c) = {
> +	.id		= UCLASS_I2C,
> +	.name		= "i2c",
> +	.per_device_auto_alloc_size = sizeof(struct dm_i2c_bus),
> +	.post_bind	= i2c_post_bind,
> +	.post_probe	= i2c_post_probe,
> +};
> +
> +UCLASS_DRIVER(i2c_generic) = {
> +	.id		= UCLASS_I2C_GENERIC,
> +	.name		= "i2c_generic",
> +};
> +
> +U_BOOT_DRIVER(i2c_generic_drv) = {

Perhaps isn't "i2c_generic_chip" clearer than "i2c_generic_drv"?



> +	.name		= "i2c_generic_drv",
> +	.id		= UCLASS_I2C_GENERIC,
> +};


Can we move "i2c_generic" to a different file?
maybe, drivers/i2c/i2c-generic.c or drivers/i2c/i2c-generic-chip.c ?

UCLASS_DRIVER(i2c) is a bus, whereas UCLASS_DRIVER(i2c_generic) is a chip.

Mixing up a bus and a chip-device together in the same file
looks confusing to me.




>  
>  /*
> + * For now there are essentially two parts to this file - driver model
> + * here at the top, and the older code below (with CONFIG_SYS_I2C being
> + * most recent). The plan is to migrate everything to driver model.
> + * The driver model structures and API are separate as they are different
> + * enough as to be incompatible for compilation purposes.
> + */
> +
> +#ifdef CONFIG_DM_I2C
> +
> +enum dm_i2c_chip_flags {
> +	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit addressing */
> +	DM_I2C_CHIP_RE_ADDRESS	= 1 << 1, /* Send address for every byte */
> +};


As I mentioned above, you define DM_I2C_CHIP_10BIT as 0x1
whereas you define I2C_M_TEN as 0x0010.

These flags should be shared with struct i2c_msg.



> +/*
> + * Not all of these flags are implemented in the U-Boot API
> + */
> +enum dm_i2c_msg_flags {
> +	I2C_M_TEN		= 0x0010, /* ten-bit chip address */
> +	I2C_M_RD		= 0x0001, /* read data, from slave to master */
> +	I2C_M_STOP		= 0x8000, /* send stop after this message */
> +	I2C_M_NOSTART		= 0x4000, /* no start before this message */
> +	I2C_M_REV_DIR_ADDR	= 0x2000, /* invert polarity of R/W bit */
> +	I2C_M_IGNORE_NAK	= 0x1000, /* continue after NAK */
> +	I2C_M_NO_RD_ACK		= 0x0800, /* skip the Ack bit on reads */
> +	I2C_M_RECV_LEN		= 0x0400, /* length is first received byte */
> +};

I think this enum usage is odd.

If you want to allocate specific values such as 0x8000, 0x4000, etc.
you should use #define instead of enum.

If you do not care which value is assigned, you can use enum.
arch/arm/include/asm/spl.h is a good example of usage of enum.






> +};
> +
> +/**
> + * struct dm_i2c_ops - driver operations for I2C uclass
> + *
> + * Drivers should support these operations unless otherwise noted. These
> + * operations are intended to be used by uclass code, not directly from
> + * other code.
> + */
> +struct dm_i2c_ops {
> +	/**
> +	 * xfer() - transfer a list of I2C messages
> +	 *
> +	 * @bus:	Bus to read from
> +	 * @chip_addr:	Chip address to read from
> +	 * @offset:	Offset within chip to start reading
> +	 * @olen:	Length of chip offset in bytes
> +	 * @buffer:	Place to put data
> +	 * @len:	Number of bytes to read
> +	 * @return 0 if OK, -EREMOTEIO if the slave did not ACK a byte,
> +	 *	other -ve value on some other error
> +	 */
> +	int (*xfer)(struct udevice *bus, struct i2c_msg *msg, int nmsgs);


This comment block does not reflect the actual prototype;
chip_addr, offset, ... etc. do not exist any more.





Best Regards
Masahiro Yamada