Hi Simon,
On Tue, 11 Nov 2014 10:46:24 -0700 Simon Glass <[email protected]> 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 <[email protected]> > --- > > 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 _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
