[U-Boot] [PATCH v2 08/17] dm: i2c: Add a uclass for I2C
Simon Glass
sjg at chromium.org
Wed Nov 19 11:24:47 CET 2014
Hi Masahiro,
On 19 November 2014 08:56, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Tue, 11 Nov 2014 10:46:24 -0700
> Simon Glass <sjg at 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 at 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.
Oh dear that's going to be very very confusing. I'll tidy this up.
>
>
>
> 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?
>
>
I'm not sure what you mean by this sorry.
>
>
>
>> +
>> +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().
>
>
Yes that sounds good but the only catch is that i2c drivers may want
to have extra information over and above struct dm_i2c_chip.
I'm not terribly happy with how this works (SPI is the same). We are
peeking to see the address of a device, then throwing that information
away.
For PCI (which I was looking at on a recent flight) it really can't be
made to work - in that case devices are configured before they are
probed, and it's really helpful to figure out the bus addresses at
'bind' time. So I'm looking at introducing a per-child platdata
structure. In that case we wouldn't need to throw the information
away, and better, it puts the driver back in control of this decoding.
>
>
>
>> + }
>> + 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.
This actually creates the device. A generic I2C device is something
that has no specific driver, but you can use read()/write() on it.
As an example, if we have an EEPROM we might add a special driver for
it with functions like 'erase' and 'lock'. In that case we would bind
the EEPROM driver to this address on the I2C bus. But for many cases
we don't have/need a special driver, and can just fall back to one
that supports simple read() and write() only.
>
>
>
>
>
>
>> +}
>> +
>> +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)
Yes this is the probe operation, where we ping that address on the bus
and see if there is anything there on the bus.
>
>
>
>> + /* 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?
>
>
The bus drivers must bind their children. So this function checks if
there is a chip at that address and then binds it to the correct
driver.
But you are right there is an extra call to i2c_bind_driver(). It
won't do anything useful - just return the same error as the first
try. I'll remove it.
>
>
>
>
>> +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.
>
At the moment only 1 byte (i.e. 7 bits) is supported by the
configuration - you need to call i2c_set_addr_len() to change it.
>
>
>
>
>
>
>
>
>
>
>
>> +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.
>
Explained above.
>
>
>> +
>> +/**
>> + * 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.
Yes, very confusing! Will fix the order.
Thanks for the close review.
Regards,
Simon
More information about the U-Boot
mailing list