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

Masahiro Yamada
2014-11-19T08:56:34Z

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