Hi Masahiro,
On 5 December 2014 at 06:11, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
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.
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