Hello Simon,
Am 02.12.2014 05:31, schrieb Simon Glass:
+Heiko - are you OK with the new msg-based approach?
Yes, you can add my acked-by to the hole series.
bye, Heiko
Hi Masahiro,
On 1 December 2014 at 04:47, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
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@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.
Thanks for these comments, I will work on another version soon.
Regards, Simon