[U-Boot] [PATCH v3 01/10] dm: i2c: Add a uclass for I2C
Simon Glass
sjg at chromium.org
Tue Dec 2 05:31:29 CET 2014
+Heiko - are you OK with the new msg-based approach?
Hi Masahiro,
On 1 December 2014 at 04:47, Masahiro Yamada <yamada.m at 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 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.
Thanks for these comments, I will work on another version soon.
Regards,
Simon
More information about the U-Boot
mailing list