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

[Simon Glass]

Hi Masahiro,

On 5 December 2014 at 06:11, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
> Here are some comments on v4.
>
>
>
> On Thu,  4 Dec 2014 21:21:20 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
>> +#define I2C_MAX_OFFSET_LEN   4
>> +
>> +/**
>> + * i2c_setup_offset() - Set up a new message with a chip offset
>> + *
>> + * @chip:    Chip to use
>> + * @offset:  Byte offset within chip
>> + * @offset_buf:      Place to put byte offset
>> + * @msg:     Message buffer
>> + * @return 0 if OK, -EADDRNOTAVAIL if the offset length is 0. In that case the
>> + * message is still set up but will not contain an offset.
>> + */
>> +static int i2c_setup_offset(struct dm_i2c_chip *chip, uint offset,
>> +                         uint8_t offset_buf[], struct i2c_msg *msg)
>> +{
>> +     int offset_len;
>> +
>> +     msg->addr = chip->chip_addr;
>> +     msg->flags = chip->flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
>> +     msg->len = chip->offset_len;
>> +     msg->buf = offset_buf;
>> +     if (!chip->offset_len)
>> +             return -EADDRNOTAVAIL;
>
> I notice i2c_{read|write}_bytewise checks the return code of this
> function, but the normal one does not.
>
> I think it seems a little bit strange.
>
>
> Instead of the code above, can we put this here?
>
>        if (chip->offset_len > I2C_MAX_OFFSET_LEN)
>                 return -EADDRNOTAVAIL;
>
> And then, the normal i2c_{read|write} should also check the return code of this.
>
>
>
>         if (!chip->offset_len)
>                 return -EADDRNOTAVAIL;
>
> is specific to  *_bytewise ones, so it can be moved to there.
>
>

We are not trying to check that the offset length is too large - that
has already been done in ic_set_chip_offset_len(). I will add an
assert and a comment. The purpose of the return value is to indicate
whether an offset was added at all. In the case of a chip with a zero
offset length, when doing a read, we want to omit the 'write' cycle
entirely since there is no offset to write. The return value makes
that possible.

>
>
>
>> +     offset_len = chip->offset_len;
>> +     while (offset_len--)
>> +             *offset_buf++ = offset >> (8 * offset_len);
>
> We should be very careful about this code
> because buffer overrun happens if too big offset_len is given.
>
> So, we should make sure that offset_len is no larger than I2C_MAX_OFFSET_LEN.
> (Or, you can pass the length of offset_buf[] to this function.)
>
> I know you implemented a similar check check in i2c_set_chip_offset_len() function.
>
> But users can skip i2c_set_chip_offset_len() and
> directly invoke i2c_read() or i2c_write().
> This is very dangerous.
>

If they skip it, then the value will be 1, the default. I've added an
assert() but I don't really want to bloat the code which duplicate
checks. Of course assert() is only useful for debugging.

>
>
>
>
>
>
>
>
>
>> +int i2c_write(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];
>> +
>> +     if (!ops->xfer)
>> +             return -ENOSYS;
>> +
>> +     if (chip->flags & DM_I2C_CHIP_RE_ADDRESS)
>> +             return i2c_write_bytewise(dev, offset, buffer, len);
>
>
>
> Have you noticed that  do_i2c_write() function in common/cmd_i2c.c
> always does the bytewise-equivalent behavior?
>
> (It uses a while() loop and in each loop it calls i2c_write with length=1)

Yes, in fact someone submitted a patch for this recently.

http://patchwork.ozlabs.org/patch/415117/

I'd like to use the flags to support this at some point.

>
> On the other hand, do_i2c_read() does not split it into small chunks.
>
> At first I was wondering why only do_i2c_write() must split into many
> small transactions,  but I got an answer when I was testing it on my board.
>
> My on-board EEPROM chip does not work with long-burst write,
> but it works with any long-burst read.
>
> It turned out some chips (at least mine) require DM_I2C_CHIP_RE_ADDRESS
> only for write.
>
> Maybe, do we need to have a _RE_ADDRESS flag for each of write/read?

I was heading that way originally but then stopped when I wasn't sure
of the use case. I think I will do this.

>
>
>
>
>
>
>
>> +static int i2c_probe_chip(struct udevice *bus, uint chip_addr,
>> +                       enum dm_i2c_chip_flags flags)
>
> I notice you added "flags" argument in v4.
>
> When and how do we use this flag ?
>
> for DM_I2C_CHIP_10BIT ??

I commented the exported function i2c_probe() but not this, I'll add a comment.
>
>
>> +{
>> +     struct dm_i2c_ops *ops = i2c_get_ops(bus);
>> +     struct i2c_msg msg[1];
>> +     uint8_t ch = 0;
>> +     int ret;
>> +
>> +     if (ops->probe_chip) {
>> +             ret = ops->probe_chip(bus, chip_addr, flags);
>> +             if (!ret || ret != -ENOSYS)
>> +                     return ret;
>> +     }
>> +
>> +     if (!ops->xfer)
>> +             return -ENOSYS;
>> +
>> +     /* Probe with a zero-length message */
>> +     msg->addr = chip_addr;
>> +     msg->flags = 0;
>
>
> If my guess is correct, this line should be like this?
>
>    ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
>
>
> I am not sure..
>

Yes we should do that, I'll add it.

>
>
>
>> +
>> +/*
>> + * i2c_get_bus_speed:
>> + *
>> + *  Returns speed of selected I2C bus in Hz
>> + */
>> +int i2c_get_bus_speed(struct udevice *bus)
>> +{
>> +     struct dm_i2c_ops *ops = i2c_get_ops(bus);
>> +     struct dm_i2c_bus *i2c = bus->uclass_priv;
>> +
>> +     if (!ops->set_bus_speed)
>> +             return i2c->speed_hz;
>> +
>> +     return ops->get_bus_speed(bus);
>> +}
>
>
> If ops->set_bus_speed is missing,  ops->get_bus_speed is never called even if it exists.
> Isn't it a bit strange?
>
> It is not clear to me why set_bus_speed must be checked.
>
>
> Why isn't it like this?
>
>
> int i2c_get_bus_speed(struct udevice *bus)
> {
>         struct dm_i2c_ops *ops = i2c_get_ops(bus);
>         struct dm_i2c_bus *i2c = bus->uclass_priv;
>
>         if (ops->get_bus_speed(bus)
>                 return ops->get_bus_speed(bus);
>
>         return i2c->speed_hz;
> }
>
>
> When opt->set_bus_speed is missing, I think there are two possibilities:
>  [1] Changing the bus speed is not supported
>  [2] Hardware registers are set in .xfer handler
>
> In case [2], somebody still might want to read the bus speed from the hardware
> register.
>
>

Well that's a strange case. But I think the simplest thing is to make
these two completely independent as you suggest. I'll also document
the return value for xfer() when the speed is wrong.

I'm not 100% sure of the use case for get_bus_speed() but I have seen
hardware where 100KHz cannot be honoured exactly, or where clock
change problems may cause the rate to change under your feet. So I'd
like to keep it here.

>
>
>
>
>> +     /**
>> +      * set_bus_speed() - set the speed of a bus (optional)
>> +      *
>> +      * The bus speed value will be updated by the uclass if this function
>> +      * does not return an error. This method is optional - if it is not
>> +      * provided then the driver can read the speed from
>> +      * bus->uclass_priv->speed_hz.
>> +      *
>> +      * @bus:        Bus to adjust
>> +      * @speed:      Requested speed in Hz
>> +      * @return 0 if OK, -INVAL for invalid values
>> +      */
>
> I am afraid you missed what I pointed out in v3.
>
>  s/-INVAL/-EINVAL/
>
>

Fixed thanks.

Regards,
Simon