Hi Masahiro, On 1 December 2014 at 04:47, Masahiro Yamada <yamad...@jp.panasonic.com> wrote: > Hi Simon, > > > My review is still under way, > but I have some comments below: > >
Thanks again for the comments, will tidy up a few other things too before sending v4. > > > On Mon, 24 Nov 2014 11:57:15 -0700 > Simon Glass <s...@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. Yes this is the write function, I need to add a read function and rename... > > > >> + >> +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. I prefer enum in most cases - it comes through nicely in the debugger and I don't need brackets around everything. > > > > > > >> +}; >> + >> +/** >> + * 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 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot