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

[Masahiro Yamada]

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.





> +	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