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

[Heiko Schocher <hs@denx.de>]

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
>

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany