Re: [U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C

Hi Simon,

On Tue, 11 Nov 2014 10:46:24 -0700 Simon Glass sjg@chromium.org wrote:

The uclass implements the same operations as the current I2C framework but makes some changes to make it fit driver model better:

• Remove the chip address from API calls
• Remove the address length from API calls
• Remove concept of 'current' I2C bus
• Drop all existing init functions

Signed-off-by: Simon Glass sjg@chromium.org

Changes in v2:

• Fix cihp typo
• Implement generic I2C devices to allow 'i2c probe' on unknown devices
• Return the probed device from i2c_probe()
• Set the bus speed after the bus is probed
• Add some debugging for generic I2C device binding
• Add a 'deblock' method to recover an I2C bus stuck in mid-transaction
• Add a helper function to find a chip on a particular bus number

[snip]

+int i2c_read(struct udevice *dev, uint addr, 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);
• if (!ops->read)

• return -ENOSYS;
  

• return ops->read(bus, chip->chip_addr, addr, chip->addr_len, buffer,

• 	 len);
  

+}

+int i2c_write(struct udevice *dev, uint addr, 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);
• if (!ops->write)

• return -ENOSYS;
  

• return ops->write(bus, chip->chip_addr, addr, chip->addr_len, buffer,

• 	  len);
  

+}

This seems inconsistent with "struct dm_i2c_ops". In this function, the address length(chip->addr_len) is passed to the forth argument of ops->write().

You should compare it with "struct dm_i2c_ops" in include/i2c.h It says that the third argument is alen.

BTW, address_offset within the chip and data are treated in the same way in I2C bus. Should we pass them separately to each driver?

I mean, can we put the offset address and data in the buffer?

+int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp) +{

• struct udevice *dev;
• debug("%s: Searching bus '%s' for address %02x: ", __func__,

•      bus->name, chip_addr);
  

• for (device_find_first_child(bus, &dev); dev;

• 	device_find_next_child(&dev)) {
  

• struct dm_i2c_chip store;
  

• struct dm_i2c_chip *chip = dev_get_parentdata(dev);
  

• int ret;
  

• if (!chip) {
  

• 	chip = &store;
  

• 	i2c_chip_ofdata_to_platdata(gd->fdt_blob,
  

• 				    dev->of_offset, chip);
  

• }
  

• if (chip->chip_addr == chip_addr) {
  

• 	ret = device_probe(dev);
  

• 	debug("found, ret=%d\n", ret);
  

• 	if (ret)
  

• 		return ret;
  

• 	*devp = dev;
  

• 	return 0;
  

• }
  

If "chip" is "NULL", i2c_chip_ofdata_to_platdata() is called to create struct dm_i2c_chip, but it is not thrown away soon. It is not efficient.

If we use device_probe_child() instead of device_probe(), I think we can re-use it.

I mean,

if (chip->chip_addr == chip_addr) { ret = device_probe_child(dev, chip);

Perhaps, we can remove sandbox_i2c_child_pre_probe().

• }
• debug("not found\n");
• return i2c_bind_driver(bus, chip_addr, devp);

+}

If no chip-device is found at the specified chip_addr, the last line calls i2c_bind_driver(). Why?

The i2c_bind_driver() tries to create a "generic" chip. What is this "generic" chip?

Besides, i2c_bind_driver() tries to probe the created generic chip, but it always fails in i2c_chip_ofdata_to_platdata() because the generic chip does not have "reg" property

I could not understand at all what this code wants to do.

+}

+int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp) +{

• struct dm_i2c_ops *ops = i2c_get_ops(bus);
• int ret;
• *devp = NULL;
• if (!ops->probe)

• return -ENODEV;
  

• /* First probe that chip */
• ret = ops->probe(bus, chip_addr);
• debug("%s: bus='%s', address %02x, ret=%d\n", __func__, bus->name,

•      chip_addr, ret);
  

• if (ret)

• return ret;
  

Is the "ops->probe" responsible to probe child devices? (At least, sandbox_i2c probes its children)

• /* The chip was found, see if we have a driver, and probe it */
• ret = i2c_get_chip(bus, chip_addr, devp);
• debug("%s: i2c_get_chip: ret=%d\n", __func__, ret);
• if (!ret || ret != -ENODEV)

• return ret;
  

• return i2c_bind_driver(bus, chip_addr, devp);

+}

If so, why do we need to call i2c_get_chip() and probe it again?

+int i2c_set_bus_speed(struct udevice *bus, unsigned int speed) +{

• struct dm_i2c_ops *ops = i2c_get_ops(bus);
• struct dm_i2c_bus *i2c = bus->uclass_priv;
• int ret;
• if (ops->set_bus_speed) {

• ret = ops->set_bus_speed(bus, speed);
  

• if (ret)
  

• 	return ret;
  

• }
• i2c->speed_hz = speed;
• return 0;

+}

+/*

• • 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);

+}

+int i2c_set_addr_len(struct udevice *dev, uint addr_len) +{

• struct udevice *bus = dev->parent;
• struct dm_i2c_chip *chip = dev_get_parentdata(dev);
• struct dm_i2c_ops *ops = i2c_get_ops(bus);
• int ret;
• if (addr_len > 3)

• return -EINVAL;
  

• if (ops->set_addr_len) {

• ret = ops->set_addr_len(dev, addr_len);
  

• if (ret)
  

• 	return ret;
  

• }
• chip->addr_len = addr_len;
• return 0;

+}

I am not 100% sure, but "addr_len" is a user-configurable parameter?

I think each device has its own fixed address size.

+UCLASS_DRIVER(i2c_generic) = {

• .id = UCLASS_I2C_GENERIC,
• .name = "i2c_generic",

+};

+U_BOOT_DRIVER(i2c_generic_drv) = {

• .name = "i2c_generic_drv",
• .id = UCLASS_I2C_GENERIC,

+};

Could you explain how "i2c_generic" is used.

+/**

• • 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 {

• /**

• * read() - read from a chip
  

• *
  

• * @bus:	Bus to read from
  

• * @chip_addr:	Chip address to read from
  

• * @alen:	Length of chip address in bytes
  

• * @offset:	Offset within chip to start reading
  

• * @buffer:	Place to put data
  

• * @len:	Number of bytes to read
  

• */
  

• int (*read)(struct udevice *bus, uint chip_addr, uint alen,

•     uint offset, uint8_t *buffer, int len);
  

• /**

• * write() - write bytes to a chip
  

• *
  

• * @dev:	Device to write to
  

• * @chip_addr:	Chip address to read from
  

• * @alen:	Length of chip address in bytes
  

• * @offset:	Offset within chip to start writing
  

• * @buffer:	Buffer containing data to write
  

• * @len:	Number of bytes to write
  

• *
  

• * @return 0 on success, -ve on failure
  

• */
  

• int (*write)(struct udevice *bus, uint chip_addr, uint alen,

•      uint offset, const uint8_t *buffer, int len);
  

See. The third argument is address length.

Best Regards Masahiro Yamada