Hi Simon,
Here are some comments on v4.
On Thu, 4 Dec 2014 21:21:20 -0700 Simon Glass sjg@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.
- 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.
+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)
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?
+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 ??
+{
- 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..
+/*
- 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.
- /**
* 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/
Best Regards Masahiro Yamada