| Message ID | 1416855444-32016-2-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
+Masahiro On Nov 24, 2014 11:58 AM, "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 v3: > - Change uclass <=> driver API to use a message list > - Correct bogus address len code (was confused with chip address length) > - Drop extra call to i2c_bind_driver() in i2c_probe() > - Add a helper to query chip flags > - Add support for reading a byte at a time with an address for each byte > > 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 > > drivers/i2c/Makefile | 1 + > drivers/i2c/i2c-uclass.c | 411 > +++++++++++++++++++++++++++++++++++++++++++++ > include/config_fallbacks.h | 6 + > include/dm/uclass-id.h | 2 + > include/i2c.h | 348 ++++++++++++++++++++++++++++++++++++++ > 5 files changed, 768 insertions(+) > create mode 100644 drivers/i2c/i2c-uclass.c > > diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile > index dae3d71..063e097 100644 > --- a/drivers/i2c/Makefile > +++ b/drivers/i2c/Makefile > @@ -4,6 +4,7 @@ > # > # SPDX-License-Identifier: GPL-2.0+ > # > +obj-$(CONFIG_DM_I2C) += i2c-uclass.o > > obj-$(CONFIG_SYS_I2C_ADI) += adi_i2c.o > obj-$(CONFIG_I2C_MV) += mv_i2c.o > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c > new file mode 100644 > index 0000000..4063a80 > --- /dev/null > +++ b/drivers/i2c/i2c-uclass.c > @@ -0,0 +1,411 @@ > +/* > + * Copyright (c) 2014 Google, Inc > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <dm.h> > +#include <errno.h> > +#include <fdtdec.h> > +#include <i2c.h> > +#include <malloc.h> > +#include <dm/device-internal.h> > +#include <dm/lists.h> > +#include <dm/root.h> > + > +DECLARE_GLOBAL_DATA_PTR; > + > +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, > + uint8_t offset_buf[], struct i2c_msg *msg) > +{ > + if (!chip->offset_len) > + return false; > + msg->addr = chip->chip_addr; > + msg->flags = chip->flags; > + msg->len = chip->offset_len; > + msg->buf = offset_buf; > + offset_buf[0] = offset; > + offset_buf[1] = offset >> 8; > + offset_buf[2] = offset >> 16; > + offset_buf[3] = offset >> 24; > + > + return true; > +} > + > +static int i2c_read_bytewise(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]; > + uint8_t buf[5]; > + int ret; > + int i; > + > + for (i = 0; i < len; i++) { > + i2c_setup_offset(chip, offset, buf, msg); > + msg->len++; > + buf[chip->offset_len] = buffer[i]; > + > + ret = ops->xfer(bus, msg, 1); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +int i2c_read(struct udevice *dev, uint offset, 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[2], *ptr; > + uint8_t offset_buf[4]; > + int msg_count; > + > + if (!ops->xfer) > + return -ENOSYS; > + if (chip->flags & DM_I2C_CHIP_RE_ADDRESS) > + return i2c_read_bytewise(dev, offset, buffer, len); > + ptr = msg; > + if (i2c_setup_offset(chip, offset, offset_buf, ptr)) > + ptr++; > + > + if (len) { > + ptr->addr = chip->chip_addr; > + ptr->flags = chip->flags | I2C_M_RD; > + ptr->len = len; > + ptr->buf = buffer; > + ptr++; > + } > + msg_count = ptr - msg; > + > + return ops->xfer(bus, msg, msg_count); > +} > + > +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; > + > + /* > + * The simple approach would be to send two messages here: one to > + * set the offset and one to write the bytes. However some drivers > + * will not be expecting this, and some chips won't like how the > + * driver presents this on the I2C bus. > + * > + * The API does not support separate offset and data. We could > extend > + * it with a flag indicating that there is data in the next message > + * that needs to be processed in the same transaction. We could > + * instead add an additional buffer to each message. For now, > handle > + * this in the uclass since it isn't clear what the impact on > drivers > + * would be with this extra complication. Unfortunately this means > + * copying the message. > + * > + * Use the stack for small messages, malloc() for larger ones. We > + * need to allow space for the offset (up to 4 bytes) and the > message > + * itself. > + */ > + if (len < 64) { > + uint8_t buf[4 + len]; > + > + i2c_setup_offset(chip, offset, buf, msg); > + msg->len += len; > + memcpy(buf + chip->offset_len, buffer, len); > + > + return ops->xfer(bus, msg, 1); > + } else { > + uint8_t *buf; > + int ret; > + > + buf = malloc(4 + len); > + if (!buf) > + return -ENOMEM; > + i2c_setup_offset(chip, offset, buf, msg); > + msg->len += len; > + memcpy(buf + chip->offset_len, buffer, len); > + > + ret = ops->xfer(bus, msg, 1); > + free(buf); > + return ret; > + } > +} > + > +static int i2c_probe_chip(struct udevice *bus, uint chip_addr) > +{ > + struct dm_i2c_ops *ops = i2c_get_ops(bus); > + struct i2c_msg msg[1]; > + uint8_t ch = 0; > + > + if (!ops->xfer) > + return -ENOSYS; > + > + msg->addr = chip_addr; > + msg->flags = 0; > + msg->len = 1; > + msg->buf = &ch; > + > + return ops->xfer(bus, msg, 1); > +} > + > +static int i2c_bind_driver(struct udevice *bus, uint chip_addr, > + struct udevice **devp) > +{ > + struct dm_i2c_chip *chip; > + char name[30], *str; > + struct udevice *dev; > + int ret; > + > + snprintf(name, sizeof(name), "generic_%x", chip_addr); > + str = strdup(name); > + ret = device_bind_driver(bus, "i2c_generic_drv", str, &dev); > + debug("%s: device_bind_driver: ret=%d\n", __func__, ret); > + if (ret) > + goto err_bind; > + > + /* Tell the device what we know about it */ > + chip = calloc(1, sizeof(struct dm_i2c_chip)); > + if (!chip) { > + ret = -ENOMEM; > + goto err_mem; > + } > + chip->chip_addr = chip_addr; > + chip->offset_len = 1; /* we assume */ > + ret = device_probe_child(dev, chip); > + debug("%s: device_probe_child: ret=%d\n", __func__, ret); > + free(chip); > + if (ret) > + goto err_probe; > + > + *devp = dev; > + return 0; > + > +err_probe: > +err_mem: > + device_unbind(dev); > +err_bind: > + free(str); > + return ret; > +} > + > +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; > + } > + } > + debug("not found\n"); > + return i2c_bind_driver(bus, chip_addr, devp); > +} > + > +int i2c_get_chip_for_busnum(int busnum, int chip_addr, struct udevice > **devp) > +{ > + struct udevice *bus; > + int ret; > + > + ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus); > + if (ret) { > + debug("Cannot find I2C bus %d\n", busnum); > + return ret; > + } > + ret = i2c_get_chip(bus, chip_addr, devp); > + if (ret) { > + debug("Cannot find I2C chip %02x on bus %d\n", chip_addr, > + busnum); > + return ret; > + } > + > + return 0; > +} > + > +int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp) > +{ > + int ret; > + > + *devp = NULL; > + > + /* First probe that chip */ > + ret = i2c_probe_chip(bus, chip_addr); > + debug("%s: bus='%s', address %02x, ret=%d\n", __func__, bus->name, > + chip_addr, ret); > + if (ret) > + return ret; > + > + /* 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); > + > + return ret; > +} > + > +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_chip_flags(struct udevice *dev, uint flags) > +{ > + 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 (ops->set_flags) { > + ret = ops->set_flags(dev, flags); > + if (ret) > + return ret; > + } > + chip->flags = flags; > + > + return 0; > +} > + > +int i2c_get_chip_flags(struct udevice *dev, uint *flagsp) > +{ > + struct dm_i2c_chip *chip = dev_get_parentdata(dev); > + > + *flagsp = chip->flags; > + > + return 0; > +} > + > +int i2c_set_chip_offset_len(struct udevice *dev, uint offset_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 (offset_len > 3) > + return -EINVAL; > + if (ops->set_offset_len) { > + ret = ops->set_offset_len(dev, offset_len); > + if (ret) > + return ret; > + } > + chip->offset_len = offset_len; > + > + return 0; > +} > + > +int i2c_deblock(struct udevice *bus) > +{ > + struct dm_i2c_ops *ops = i2c_get_ops(bus); > + > + /* > + * We could implement a software deblocking here if we could get > + * access to the GPIOs used by I2C, and switch them to GPIO mode > + * and then back to I2C. This is somewhat beyond our powers in > + * driver model at present, so for now just fail. > + * > + * See https://patchwork.ozlabs.org/patch/399040/ > + */ > + if (!ops->deblock) > + return -ENOSYS; > + > + return ops->deblock(bus); > +} > + > +int i2c_chip_ofdata_to_platdata(const void *blob, int node, > + struct dm_i2c_chip *chip) > +{ > + chip->offset_len = 1; /* default */ > + chip->flags = 0; > + chip->chip_addr = fdtdec_get_int(gd->fdt_blob, node, "reg", -1); > + if (chip->chip_addr == -1) { > + debug("%s: I2C Node '%s' has no 'reg' property\n", > __func__, > + fdt_get_name(blob, node, NULL)); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int i2c_post_probe(struct udevice *dev) > +{ > + struct dm_i2c_bus *i2c = dev->uclass_priv; > + > + i2c->speed_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset, > + "clock-frequency", 100000); > + > + return i2c_set_bus_speed(dev, i2c->speed_hz); > +} > + > +int i2c_post_bind(struct udevice *dev) > +{ > + /* Scan the bus for devices */ > + return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false); > +} > + > +UCLASS_DRIVER(i2c) = { > + .id = UCLASS_I2C, > + .name = "i2c", > + .per_device_auto_alloc_size = sizeof(struct dm_i2c_bus), > + .post_bind = i2c_post_bind, > + .post_probe = i2c_post_probe, > +}; > + > +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, > +}; > diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h > index 7d8daa2..91a0a43 100644 > --- a/include/config_fallbacks.h > +++ b/include/config_fallbacks.h > @@ -87,4 +87,10 @@ > #undef CONFIG_IMAGE_FORMAT_LEGACY > #endif > > +#ifdef CONFIG_DM_I2C > +# ifdef CONFIG_SYS_I2C > +# error "Cannot define CONFIG_SYS_I2C when CONFIG_DM_I2C is used" > +# endif > +#endif > + > #endif /* __CONFIG_FALLBACKS_H */ > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h > index a8944c9..43514bc 100644 > --- a/include/dm/uclass-id.h > +++ b/include/dm/uclass-id.h > @@ -28,6 +28,8 @@ enum uclass_id { > UCLASS_SPI_GENERIC, /* Generic SPI flash target */ > UCLASS_SPI_FLASH, /* SPI flash */ > UCLASS_CROS_EC, /* Chrome OS EC */ > + UCLASS_I2C, /* I2C bus */ > + UCLASS_I2C_GENERIC, /* Generic I2C device */ > > UCLASS_COUNT, > UCLASS_INVALID = -1, > diff --git a/include/i2c.h b/include/i2c.h > index 1b4078e..dd8fd2e 100644 > --- a/include/i2c.h > +++ b/include/i2c.h > @@ -18,6 +18,351 @@ > #define _I2C_H_ > > /* > + * For now there are essentially two parts to this file - driver model > + * here at the top, and the older code below (with CONFIG_SYS_I2C being > + * most recent). The plan is to migrate everything to driver model. > + * The driver model structures and API are separate as they are different > + * enough as to be incompatible for compilation purposes. > + */ > + > +#ifdef CONFIG_DM_I2C > + > +enum dm_i2c_chip_flags { > + DM_I2C_CHIP_10BIT = 1 << 0, /* Use 10-bit addressing */ > + DM_I2C_CHIP_RE_ADDRESS = 1 << 1, /* Send address for every byte */ > +}; > + > +/** > + * struct dm_i2c_chip - information about an i2c chip > + * > + * An I2C chip is a device on the I2C bus. It sits at a particular address > + * and normally supports 7-bit or 10-bit addressing. > + * > + * To obtain this structure, use dev_get_parentdata(dev) where dev is the > + * chip to examine. > + * > + * @chip_addr: Chip address on bus > + * @offset_len: Length of offset in bytes. A single byte offset can > + * represent up to 256 bytes. A value larger than 1 may be > + * needed for larger devices. > + * @flags: Flags for this chip (dm_i2c_chip_flags) > + * @emul: Emulator for this chip address (only used for emulation) > + */ > +struct dm_i2c_chip { > + uint chip_addr; > + uint offset_len; > + uint flags; > +#ifdef CONFIG_SANDBOX > + struct udevice *emul; > +#endif > +}; > + > +/** > + * struct dm_i2c_bus- information about an i2c bus > + * > + * An I2C bus contains 0 or more chips on it, each at its own address. The > + * bus can operate at different speeds (measured in Hz, typically 100KHz > + * or 400KHz). > + * > + * To obtain this structure, use bus->uclass_priv where bus is the I2C > + * bus udevice. > + * > + * @speed_hz: Bus speed in hertz (typically 100000) > + */ > +struct dm_i2c_bus { > + int speed_hz; > +}; > + > +/** > + * i2c_read() - read bytes from an I2C chip > + * > + * To obtain an I2C device (called a 'chip') given the I2C bus address you > + * can use i2c_get_chip(). To obtain a bus by bus number use > + * uclass_get_device_by_seq(UCLASS_I2C, <bus number>). > + * > + * To set the address length of a devce use i2c_set_addr_len(). It > + * defaults to 1. > + * > + * @dev: Chip to read from > + * @offset: Offset within chip to start reading > + * @buffer: Place to put data > + * @len: Number of bytes to read > + * > + * @return 0 on success, -ve on failure > + */ > +int i2c_read(struct udevice *dev, uint offset, uint8_t *buffer, > + int len); > + > +/** > + * i2c_write() - write bytes to an I2C chip > + * > + * See notes for i2c_read() above. > + * > + * @dev: Chip to write to > + * @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 i2c_write(struct udevice *dev, uint offset, const uint8_t *buffer, > + int len); > + > +/** > + * i2c_probe() - probe a particular chip address > + * > + * This can be useful to check for the existence of a chip on the bus. > + * It is typically implemented by writing the chip address to the bus > + * and checking that the chip replies with an ACK. > + * > + * @bus: Bus to probe > + * @chip_addr: 7-bit address to probe (10-bit and others are not > supported) > + * @devp: Returns the device found, or NULL if none > + * @return 0 if a chip was found at that address, -ve if not > + */ > +int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp); > + > +/** > + * i2c_set_bus_speed() - set the speed of a bus > + * > + * @bus: Bus to adjust > + * @speed: Requested speed in Hz > + * @return 0 if OK, -EINVAL for invalid values > + */ > +int i2c_set_bus_speed(struct udevice *bus, unsigned int speed); > + > +/** > + * i2c_get_bus_speed() - get the speed of a bus > + * > + * @bus: Bus to check > + * @return speed of selected I2C bus in Hz, -ve on error > + */ > +int i2c_get_bus_speed(struct udevice *bus); > + > +/** > + * i2c_set_chip_flags() - set flags for a chip > + * > + * Typically addresses are 7 bits, but for 10-bit addresses you should set > + * flags to DM_I2C_CHIP_10BIT. All accesses will then use 10-bit > addressing. > + * > + * @dev: Chip to adjust > + * @flags: New flags > + * @return 0 if OK, -EINVAL if value is unsupported, other -ve value on > error > + */ > +int i2c_set_chip_flags(struct udevice *dev, uint flags); > + > +/** > + * i2c_get_chip_flags() - get flags for a chip > + * > + * @dev: Chip to check > + * @flagsp: Place to put flags > + * @return 0 if OK, other -ve value on error > + */ > +int i2c_get_chip_flags(struct udevice *dev, uint *flagsp); > + > +/** > + * i2c_set_offset_len() - set the offset length for a chip > + * > + * The offset used to access a chip may be up to 4 bytes long. Typically > it > + * is only 1 byte, which is enough for chips with 256 bytes of memory or > + * registers. The default value is 1, but you can call this function to > + * change it. > + * > + * @offset_len: New offset length value (typically 1 or 2) > + */ > + > +int i2c_set_chip_offset_len(struct udevice *dev, uint offset_len); > +/** > + * i2c_deblock() - recover a bus that is in an unknown state > + * > + * See the deblock() method in 'struct dm_i2c_ops' for full information > + * > + * @bus: Bus to recover > + * @return 0 if OK, -ve on error > + */ > +int i2c_deblock(struct udevice *bus); > + > +/* > + * Not all of these flags are implemented in the U-Boot API > + */ > +enum dm_i2c_msg_flags { > + I2C_M_TEN = 0x0010, /* ten-bit chip address */ > + I2C_M_RD = 0x0001, /* read data, from slave to > master */ > + I2C_M_STOP = 0x8000, /* send stop after this message > */ > + I2C_M_NOSTART = 0x4000, /* no start before this message > */ > + I2C_M_REV_DIR_ADDR = 0x2000, /* invert polarity of R/W bit */ > + I2C_M_IGNORE_NAK = 0x1000, /* continue after NAK */ > + I2C_M_NO_RD_ACK = 0x0800, /* skip the Ack bit on reads */ > + I2C_M_RECV_LEN = 0x0400, /* length is first received byte > */ > +}; > + > +/** > + * struct i2c_msg - an I2C message > + * > + * @addr: Slave address > + * @flags: Flags (see enum dm_i2c_msg_flags) > + * @len: Length of buffer in bytes > + * @buf: Buffer to send/receive > + */ > +struct i2c_msg { > + uint addr; > + uint flags; > + uint len; > + u8 *buf; > +}; > + > +/** > + * struct i2c_msg_list - a list of I2C messages > + * > + * This is called i2c_rdwr_ioctl_data in Linux but the name does not seem > + * appropriate in U-Boot. > + * > + * @msg: Pointer to i2c_msg array > + * @nmsgs: Number of elements in the array > + */ > +struct i2c_msg_list { > + struct i2c_msg *msgs; > + uint nmsgs; > +}; > + > +/** > + * 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 { > + /** > + * xfer() - transfer a list of I2C messages > + * > + * @bus: Bus to read from > + * @chip_addr: Chip address to read from > + * @offset: Offset within chip to start reading > + * @olen: Length of chip offset in bytes > + * @buffer: Place to put data > + * @len: Number of bytes to read > + * @return 0 if OK, -EREMOTEIO if the slave did not ACK a byte, > + * other -ve value on some other error > + */ > + int (*xfer)(struct udevice *bus, struct i2c_msg *msg, int nmsgs); > + > + /** > + * 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. > + * > + * @bus: Bus to adjust > + * @speed: Requested speed in Hz > + * @return 0 if OK, -INVAL for invalid values > + */ > + int (*set_bus_speed)(struct udevice *bus, unsigned int speed); > + > + /** > + * get_bus_speed() - get the speed of a bus (optional) > + * > + * Normally this can be provided by the uclass, but if you want > your > + * driver to check the bus speed by looking at the hardware, you > can > + * implement that here. > + * > + * @bus: Bus to check > + * @return speed of selected I2C bus in Hz, -ve on error > + */ > + int (*get_bus_speed)(struct udevice *bus); > + > + /** > + * set_flags() - set the flags for a chip (optional) > + * > + * This is generally implemented by the uclass, but drivers can > + * check the value to ensure that unsupported options are not used. > + * If provided, this method will always be called when the flags > + * change. > + * > + * @dev: Chip to adjust > + * @flags: New flags value > + * @return 0 if OK, -EINVAL if value is unsupported > + */ > + int (*set_flags)(struct udevice *dev, uint flags); > + > + /** > + * set_offset_len() - set the offset length of a chip (optional) > + * > + * This is generally implemented by the uclass, but drivers can > + * check the value to ensure that unsupported options are not used. > + * If provided, this method will always be called when the offset > + * length changes from the default of 1. > + * > + * @dev: Chip to adjust > + * @addr_len: New offset length value (typically 1 or 2) > + * @return 0 if OK, -INVAL if value is unsupported > + */ > + int (*set_offset_len)(struct udevice *dev, uint offset_len); > + > + /** > + * deblock() - recover a bus that is in an unknown state > + * > + * I2C is a synchronous protocol and resets of the processor in the > + * middle of an access can block the I2C Bus until a powerdown of > + * the full unit is done. This is because slaves can be stuck > + * waiting for addition bus transitions for a transaction that will > + * never complete. Resetting the I2C master does not help. The only > + * way is to force the bus through a series of transitions to make > + * sure that all slaves are done with the transaction. This method > + * performs this 'deblocking' if support by the driver. > + * > + * This method is optional. > + */ > + int (*deblock)(struct udevice *bus); > +}; > + > +#define i2c_get_ops(dev) ((struct dm_i2c_ops *)(dev)->driver->ops) > + > +/** > + * i2c_get_chip() - get a device to use to access a chip on a bus > + * > + * This returns the device for the given chip address. The device can then > + * be used with calls to i2c_read(), i2c_write(), i2c_probe(), etc. > + * > + * @bus: Bus to examine > + * @chip_addr: Chip address for the new device > + * @devp: Returns pointer to new device if found or -ENODEV if not > + * found > + */ > +int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice > **devp); > + > +/** > + * i2c_get_chip() - get a device to use to access a chip on a bus number > + * > + * This returns the device for the given chip address on a particular bus > + * number. > + * > + * @busnum: Bus number to examine > + * @chip_addr: Chip address for the new device > + * @devp: Returns pointer to new device if found or -ENODEV if not > + * found > + */ > +int i2c_get_chip_for_busnum(int busnum, int chip_addr, struct udevice > **devp); > + > +/** > + * i2c_chip_ofdata_to_platdata() - Decode standard I2C platform data > + * > + * This decodes the chip address from a device tree node and puts it into > + * its dm_i2c_chip structure. This should be called in your driver's > + * ofdata_to_platdata() method. > + * > + * @blob: Device tree blob > + * @node: Node offset to read from > + * @spi: Place to put the decoded information > + */ > +int i2c_chip_ofdata_to_platdata(const void *blob, int node, > + struct dm_i2c_chip *chip); > + > +#endif > + > +#ifndef CONFIG_DM_I2C > + > +/* > * WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING > * > * The implementation MUST NOT use static or global variables if the > @@ -451,4 +796,7 @@ int i2c_get_bus_num_fdt(int node); > * @return 0 if port was reset, -1 if not found > */ > int i2c_reset_port_fdt(const void *blob, int node); > + > +#endif /* !CONFIG_DM_I2C */ > + > #endif /* _I2C_H_ */ > -- > 2.1.0.rc2.206.gedb03e5 > >
Hi Simon, On Thu, 27 Nov 2014 21:47:07 -0700 Simon Glass <sjg@chromium.org> wrote: > +Masahiro > On Nov 24, 2014 11:58 AM, "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> I am reviewing v3 now. Hopefully, I will send some comments in a couple of days. Best Regards Masahiro Yamada
Hi Simon, My review is still under way, but I have some comments below: On Mon, 24 Nov 2014 11:57:15 -0700 Simon Glass <sjg@chromium.org> wrote: > +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, > + uint8_t offset_buf[], struct i2c_msg *msg) > +{ > + if (!chip->offset_len) > + return false; > + msg->addr = chip->chip_addr; > + msg->flags = chip->flags; > + msg->len = chip->offset_len; > + msg->buf = offset_buf; You directly copy from (struct dm_i2c_chip *)->flags to (struct i2c_msg *)->flags. But you define completely different flags for them: DM_I2C_CHIP_10BIT is defined as 0x1. I2C_M_TEN is defined as 0x10. It would not work. > + > +static int i2c_read_bytewise(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]; > + uint8_t buf[5]; > + int ret; > + int i; > + > + for (i = 0; i < len; i++) { > + i2c_setup_offset(chip, offset, buf, msg); > + msg->len++; > + buf[chip->offset_len] = buffer[i]; > + > + ret = ops->xfer(bus, msg, 1); > + if (ret) > + return ret; > + } > + > + return 0; > +} I could not understand how this works. It seems to send only write transactions. > + > +static int i2c_bind_driver(struct udevice *bus, uint chip_addr, > + struct udevice **devp) > +{ > + struct dm_i2c_chip *chip; > + char name[30], *str; > + struct udevice *dev; > + int ret; > + > + snprintf(name, sizeof(name), "generic_%x", chip_addr); > + str = strdup(name); > + ret = device_bind_driver(bus, "i2c_generic_drv", str, &dev); > + debug("%s: device_bind_driver: ret=%d\n", __func__, ret); > + if (ret) > + goto err_bind; > + > + /* Tell the device what we know about it */ > + chip = calloc(1, sizeof(struct dm_i2c_chip)); > + if (!chip) { > + ret = -ENOMEM; > + goto err_mem; > + } > + chip->chip_addr = chip_addr; > + chip->offset_len = 1; /* we assume */ > + ret = device_probe_child(dev, chip); > + debug("%s: device_probe_child: ret=%d\n", __func__, ret); > + free(chip); Why do you need calloc() & free() here? I think you can use the stack area for "struct dm_i2c_chip chip;" > + > +UCLASS_DRIVER(i2c) = { > + .id = UCLASS_I2C, > + .name = "i2c", > + .per_device_auto_alloc_size = sizeof(struct dm_i2c_bus), > + .post_bind = i2c_post_bind, > + .post_probe = i2c_post_probe, > +}; > + > +UCLASS_DRIVER(i2c_generic) = { > + .id = UCLASS_I2C_GENERIC, > + .name = "i2c_generic", > +}; > + > +U_BOOT_DRIVER(i2c_generic_drv) = { Perhaps isn't "i2c_generic_chip" clearer than "i2c_generic_drv"? > + .name = "i2c_generic_drv", > + .id = UCLASS_I2C_GENERIC, > +}; Can we move "i2c_generic" to a different file? maybe, drivers/i2c/i2c-generic.c or drivers/i2c/i2c-generic-chip.c ? UCLASS_DRIVER(i2c) is a bus, whereas UCLASS_DRIVER(i2c_generic) is a chip. Mixing up a bus and a chip-device together in the same file looks confusing to me. > > /* > + * For now there are essentially two parts to this file - driver model > + * here at the top, and the older code below (with CONFIG_SYS_I2C being > + * most recent). The plan is to migrate everything to driver model. > + * The driver model structures and API are separate as they are different > + * enough as to be incompatible for compilation purposes. > + */ > + > +#ifdef CONFIG_DM_I2C > + > +enum dm_i2c_chip_flags { > + DM_I2C_CHIP_10BIT = 1 << 0, /* Use 10-bit addressing */ > + DM_I2C_CHIP_RE_ADDRESS = 1 << 1, /* Send address for every byte */ > +}; As I mentioned above, you define DM_I2C_CHIP_10BIT as 0x1 whereas you define I2C_M_TEN as 0x0010. These flags should be shared with struct i2c_msg. > +/* > + * Not all of these flags are implemented in the U-Boot API > + */ > +enum dm_i2c_msg_flags { > + I2C_M_TEN = 0x0010, /* ten-bit chip address */ > + I2C_M_RD = 0x0001, /* read data, from slave to master */ > + I2C_M_STOP = 0x8000, /* send stop after this message */ > + I2C_M_NOSTART = 0x4000, /* no start before this message */ > + I2C_M_REV_DIR_ADDR = 0x2000, /* invert polarity of R/W bit */ > + I2C_M_IGNORE_NAK = 0x1000, /* continue after NAK */ > + I2C_M_NO_RD_ACK = 0x0800, /* skip the Ack bit on reads */ > + I2C_M_RECV_LEN = 0x0400, /* length is first received byte */ > +}; I think this enum usage is odd. If you want to allocate specific values such as 0x8000, 0x4000, etc. you should use #define instead of enum. If you do not care which value is assigned, you can use enum. arch/arm/include/asm/spl.h is a good example of usage of enum. > +}; > + > +/** > + * 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 { > + /** > + * xfer() - transfer a list of I2C messages > + * > + * @bus: Bus to read from > + * @chip_addr: Chip address to read from > + * @offset: Offset within chip to start reading > + * @olen: Length of chip offset in bytes > + * @buffer: Place to put data > + * @len: Number of bytes to read > + * @return 0 if OK, -EREMOTEIO if the slave did not ACK a byte, > + * other -ve value on some other error > + */ > + int (*xfer)(struct udevice *bus, struct i2c_msg *msg, int nmsgs); This comment block does not reflect the actual prototype; chip_addr, offset, ... etc. do not exist any more. Best Regards Masahiro Yamada
+Heiko - are you OK with the new msg-based approach? Hi Masahiro, On 1 December 2014 at 04:47, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > My review is still under way, > but I have some comments below: > > > > > On Mon, 24 Nov 2014 11:57:15 -0700 > Simon Glass <sjg@chromium.org> wrote: > >> +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, >> + uint8_t offset_buf[], struct i2c_msg *msg) >> +{ >> + if (!chip->offset_len) >> + return false; >> + msg->addr = chip->chip_addr; >> + msg->flags = chip->flags; >> + msg->len = chip->offset_len; >> + msg->buf = offset_buf; > > You directly copy > from (struct dm_i2c_chip *)->flags > to (struct i2c_msg *)->flags. > > But you define completely different flags for them: > DM_I2C_CHIP_10BIT is defined as 0x1. > I2C_M_TEN is defined as 0x10. > > It would not work. > > > >> + >> +static int i2c_read_bytewise(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]; >> + uint8_t buf[5]; >> + int ret; >> + int i; >> + >> + for (i = 0; i < len; i++) { >> + i2c_setup_offset(chip, offset, buf, msg); >> + msg->len++; >> + buf[chip->offset_len] = buffer[i]; >> + >> + ret = ops->xfer(bus, msg, 1); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} > > I could not understand how this works. > It seems to send only write transactions. > > > >> + >> +static int i2c_bind_driver(struct udevice *bus, uint chip_addr, >> + struct udevice **devp) >> +{ >> + struct dm_i2c_chip *chip; >> + char name[30], *str; >> + struct udevice *dev; >> + int ret; >> + >> + snprintf(name, sizeof(name), "generic_%x", chip_addr); >> + str = strdup(name); >> + ret = device_bind_driver(bus, "i2c_generic_drv", str, &dev); >> + debug("%s: device_bind_driver: ret=%d\n", __func__, ret); >> + if (ret) >> + goto err_bind; >> + >> + /* Tell the device what we know about it */ >> + chip = calloc(1, sizeof(struct dm_i2c_chip)); >> + if (!chip) { >> + ret = -ENOMEM; >> + goto err_mem; >> + } >> + chip->chip_addr = chip_addr; >> + chip->offset_len = 1; /* we assume */ >> + ret = device_probe_child(dev, chip); >> + debug("%s: device_probe_child: ret=%d\n", __func__, ret); >> + free(chip); > > > Why do you need calloc() & free() here? > I think you can use the stack area for "struct dm_i2c_chip chip;" > > > > > > > > >> + >> +UCLASS_DRIVER(i2c) = { >> + .id = UCLASS_I2C, >> + .name = "i2c", >> + .per_device_auto_alloc_size = sizeof(struct dm_i2c_bus), >> + .post_bind = i2c_post_bind, >> + .post_probe = i2c_post_probe, >> +}; >> + >> +UCLASS_DRIVER(i2c_generic) = { >> + .id = UCLASS_I2C_GENERIC, >> + .name = "i2c_generic", >> +}; >> + >> +U_BOOT_DRIVER(i2c_generic_drv) = { > > Perhaps isn't "i2c_generic_chip" clearer than "i2c_generic_drv"? > > > >> + .name = "i2c_generic_drv", >> + .id = UCLASS_I2C_GENERIC, >> +}; > > > Can we move "i2c_generic" to a different file? > maybe, drivers/i2c/i2c-generic.c or drivers/i2c/i2c-generic-chip.c ? > > UCLASS_DRIVER(i2c) is a bus, whereas UCLASS_DRIVER(i2c_generic) is a chip. > > Mixing up a bus and a chip-device together in the same file > looks confusing to me. > > > > >> >> /* >> + * For now there are essentially two parts to this file - driver model >> + * here at the top, and the older code below (with CONFIG_SYS_I2C being >> + * most recent). The plan is to migrate everything to driver model. >> + * The driver model structures and API are separate as they are different >> + * enough as to be incompatible for compilation purposes. >> + */ >> + >> +#ifdef CONFIG_DM_I2C >> + >> +enum dm_i2c_chip_flags { >> + DM_I2C_CHIP_10BIT = 1 << 0, /* Use 10-bit addressing */ >> + DM_I2C_CHIP_RE_ADDRESS = 1 << 1, /* Send address for every byte */ >> +}; > > > As I mentioned above, you define DM_I2C_CHIP_10BIT as 0x1 > whereas you define I2C_M_TEN as 0x0010. > > These flags should be shared with struct i2c_msg. > > > >> +/* >> + * Not all of these flags are implemented in the U-Boot API >> + */ >> +enum dm_i2c_msg_flags { >> + I2C_M_TEN = 0x0010, /* ten-bit chip address */ >> + I2C_M_RD = 0x0001, /* read data, from slave to master */ >> + I2C_M_STOP = 0x8000, /* send stop after this message */ >> + I2C_M_NOSTART = 0x4000, /* no start before this message */ >> + I2C_M_REV_DIR_ADDR = 0x2000, /* invert polarity of R/W bit */ >> + I2C_M_IGNORE_NAK = 0x1000, /* continue after NAK */ >> + I2C_M_NO_RD_ACK = 0x0800, /* skip the Ack bit on reads */ >> + I2C_M_RECV_LEN = 0x0400, /* length is first received byte */ >> +}; > > I think this enum usage is odd. > > If you want to allocate specific values such as 0x8000, 0x4000, etc. > you should use #define instead of enum. > > If you do not care which value is assigned, you can use enum. > arch/arm/include/asm/spl.h is a good example of usage of enum. > > > > > > >> +}; >> + >> +/** >> + * 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 { >> + /** >> + * xfer() - transfer a list of I2C messages >> + * >> + * @bus: Bus to read from >> + * @chip_addr: Chip address to read from >> + * @offset: Offset within chip to start reading >> + * @olen: Length of chip offset in bytes >> + * @buffer: Place to put data >> + * @len: Number of bytes to read >> + * @return 0 if OK, -EREMOTEIO if the slave did not ACK a byte, >> + * other -ve value on some other error >> + */ >> + int (*xfer)(struct udevice *bus, struct i2c_msg *msg, int nmsgs); > > > This comment block does not reflect the actual prototype; > chip_addr, offset, ... etc. do not exist any more. Thanks for these comments, I will work on another version soon. Regards, Simon
Hi Simon, On Mon, 1 Dec 2014 21:31:29 -0700 Simon Glass <sjg@chromium.org> wrote: > +Heiko - are you OK with the new msg-based approach? This approach looks much better to me. > >> + .name = "i2c_generic_drv", > >> + .id = UCLASS_I2C_GENERIC, > >> +}; > > > > > > Can we move "i2c_generic" to a different file? > > maybe, drivers/i2c/i2c-generic.c or drivers/i2c/i2c-generic-chip.c ? > > > > UCLASS_DRIVER(i2c) is a bus, whereas UCLASS_DRIVER(i2c_generic) is a chip. > > > > Mixing up a bus and a chip-device together in the same file > > looks confusing to me. I take back this comment. I thought it again, and "i2c_generic" is short enough to be included in i2c-uclass.c. I am fine with it as is. Best Regards Masahiro Yamada
Hello Simon, Am 02.12.2014 05:31, schrieb Simon Glass: > +Heiko - are you OK with the new msg-based approach? Yes, you can add my acked-by to the hole series. bye, Heiko > > > Hi Masahiro, > > On 1 December 2014 at 04:47, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: >> Hi Simon, >> >> >> My review is still under way, >> but I have some comments below: >> >> >> >> >> On Mon, 24 Nov 2014 11:57:15 -0700 >> Simon Glass <sjg@chromium.org> wrote: >> >>> +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, >>> + uint8_t offset_buf[], struct i2c_msg *msg) >>> +{ >>> + if (!chip->offset_len) >>> + return false; >>> + msg->addr = chip->chip_addr; >>> + msg->flags = chip->flags; >>> + msg->len = chip->offset_len; >>> + msg->buf = offset_buf; >> >> You directly copy >> from (struct dm_i2c_chip *)->flags >> to (struct i2c_msg *)->flags. >> >> But you define completely different flags for them: >> DM_I2C_CHIP_10BIT is defined as 0x1. >> I2C_M_TEN is defined as 0x10. >> >> It would not work. >> >> >> >>> + >>> +static int i2c_read_bytewise(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]; >>> + uint8_t buf[5]; >>> + int ret; >>> + int i; >>> + >>> + for (i = 0; i < len; i++) { >>> + i2c_setup_offset(chip, offset, buf, msg); >>> + msg->len++; >>> + buf[chip->offset_len] = buffer[i]; >>> + >>> + ret = ops->xfer(bus, msg, 1); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >> >> I could not understand how this works. >> It seems to send only write transactions. >> >> >> >>> + >>> +static int i2c_bind_driver(struct udevice *bus, uint chip_addr, >>> + struct udevice **devp) >>> +{ >>> + struct dm_i2c_chip *chip; >>> + char name[30], *str; >>> + struct udevice *dev; >>> + int ret; >>> + >>> + snprintf(name, sizeof(name), "generic_%x", chip_addr); >>> + str = strdup(name); >>> + ret = device_bind_driver(bus, "i2c_generic_drv", str, &dev); >>> + debug("%s: device_bind_driver: ret=%d\n", __func__, ret); >>> + if (ret) >>> + goto err_bind; >>> + >>> + /* Tell the device what we know about it */ >>> + chip = calloc(1, sizeof(struct dm_i2c_chip)); >>> + if (!chip) { >>> + ret = -ENOMEM; >>> + goto err_mem; >>> + } >>> + chip->chip_addr = chip_addr; >>> + chip->offset_len = 1; /* we assume */ >>> + ret = device_probe_child(dev, chip); >>> + debug("%s: device_probe_child: ret=%d\n", __func__, ret); >>> + free(chip); >> >> >> Why do you need calloc() & free() here? >> I think you can use the stack area for "struct dm_i2c_chip chip;" >> >> >> >> >> >> >> >> >>> + >>> +UCLASS_DRIVER(i2c) = { >>> + .id = UCLASS_I2C, >>> + .name = "i2c", >>> + .per_device_auto_alloc_size = sizeof(struct dm_i2c_bus), >>> + .post_bind = i2c_post_bind, >>> + .post_probe = i2c_post_probe, >>> +}; >>> + >>> +UCLASS_DRIVER(i2c_generic) = { >>> + .id = UCLASS_I2C_GENERIC, >>> + .name = "i2c_generic", >>> +}; >>> + >>> +U_BOOT_DRIVER(i2c_generic_drv) = { >> >> Perhaps isn't "i2c_generic_chip" clearer than "i2c_generic_drv"? >> >> >> >>> + .name = "i2c_generic_drv", >>> + .id = UCLASS_I2C_GENERIC, >>> +}; >> >> >> Can we move "i2c_generic" to a different file? >> maybe, drivers/i2c/i2c-generic.c or drivers/i2c/i2c-generic-chip.c ? >> >> UCLASS_DRIVER(i2c) is a bus, whereas UCLASS_DRIVER(i2c_generic) is a chip. >> >> Mixing up a bus and a chip-device together in the same file >> looks confusing to me. >> >> >> >> >>> >>> /* >>> + * For now there are essentially two parts to this file - driver model >>> + * here at the top, and the older code below (with CONFIG_SYS_I2C being >>> + * most recent). The plan is to migrate everything to driver model. >>> + * The driver model structures and API are separate as they are different >>> + * enough as to be incompatible for compilation purposes. >>> + */ >>> + >>> +#ifdef CONFIG_DM_I2C >>> + >>> +enum dm_i2c_chip_flags { >>> + DM_I2C_CHIP_10BIT = 1 << 0, /* Use 10-bit addressing */ >>> + DM_I2C_CHIP_RE_ADDRESS = 1 << 1, /* Send address for every byte */ >>> +}; >> >> >> As I mentioned above, you define DM_I2C_CHIP_10BIT as 0x1 >> whereas you define I2C_M_TEN as 0x0010. >> >> These flags should be shared with struct i2c_msg. >> >> >> >>> +/* >>> + * Not all of these flags are implemented in the U-Boot API >>> + */ >>> +enum dm_i2c_msg_flags { >>> + I2C_M_TEN = 0x0010, /* ten-bit chip address */ >>> + I2C_M_RD = 0x0001, /* read data, from slave to master */ >>> + I2C_M_STOP = 0x8000, /* send stop after this message */ >>> + I2C_M_NOSTART = 0x4000, /* no start before this message */ >>> + I2C_M_REV_DIR_ADDR = 0x2000, /* invert polarity of R/W bit */ >>> + I2C_M_IGNORE_NAK = 0x1000, /* continue after NAK */ >>> + I2C_M_NO_RD_ACK = 0x0800, /* skip the Ack bit on reads */ >>> + I2C_M_RECV_LEN = 0x0400, /* length is first received byte */ >>> +}; >> >> I think this enum usage is odd. >> >> If you want to allocate specific values such as 0x8000, 0x4000, etc. >> you should use #define instead of enum. >> >> If you do not care which value is assigned, you can use enum. >> arch/arm/include/asm/spl.h is a good example of usage of enum. >> >> >> >> >> >> >>> +}; >>> + >>> +/** >>> + * 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 { >>> + /** >>> + * xfer() - transfer a list of I2C messages >>> + * >>> + * @bus: Bus to read from >>> + * @chip_addr: Chip address to read from >>> + * @offset: Offset within chip to start reading >>> + * @olen: Length of chip offset in bytes >>> + * @buffer: Place to put data >>> + * @len: Number of bytes to read >>> + * @return 0 if OK, -EREMOTEIO if the slave did not ACK a byte, >>> + * other -ve value on some other error >>> + */ >>> + int (*xfer)(struct udevice *bus, struct i2c_msg *msg, int nmsgs); >> >> >> This comment block does not reflect the actual prototype; >> chip_addr, offset, ... etc. do not exist any more. > > Thanks for these comments, I will work on another version soon. > > Regards, > Simon >
Hi, On 1 December 2014 at 23:29, Heiko Schocher <hs@denx.de> wrote: > Hello Simon, > > Am 02.12.2014 05:31, schrieb Simon Glass: >> >> +Heiko - are you OK with the new msg-based approach? > > > Yes, you can add my acked-by to the hole series. OK good, I'm going to continue on this line, and work through Masahiro's comments (and add a few more tests) over the next few days. Regards, Simon
Hi Simon, A little more comments from me. On Mon, 24 Nov 2014 11:57:15 -0700 Simon Glass <sjg@chromium.org> wrote: > +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; > +} This looks odd. If each driver does not have .set_bus_speed handler, we cannot change the bus speed because changing the bus speed involves some hardware register(s) setting. We should not change i2c->speed_hz without changing the actual speed. I think the code should be: if (ops->set_bus_speed) { ret = ops->set_bus_speed(bus, speed); if (ret) return ret; i2c->speed_hz = speed; } + /** + * 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. + * + * @bus: Bus to adjust + * @speed: Requested speed in Hz + * @return 0 if OK, -INVAL for invalid values s/-INVAL/-EINVAL/ Best Regards Masahiro Yamada
Hi Masahiro, On 1 December 2014 at 04:47, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > My review is still under way, > but I have some comments below: > > Thanks again for the comments, will tidy up a few other things too before sending v4. > > > On Mon, 24 Nov 2014 11:57:15 -0700 > Simon Glass <sjg@chromium.org> wrote: > >> +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, >> + uint8_t offset_buf[], struct i2c_msg *msg) >> +{ >> + if (!chip->offset_len) >> + return false; >> + msg->addr = chip->chip_addr; >> + msg->flags = chip->flags; >> + msg->len = chip->offset_len; >> + msg->buf = offset_buf; > > You directly copy > from (struct dm_i2c_chip *)->flags > to (struct i2c_msg *)->flags. > > But you define completely different flags for them: > DM_I2C_CHIP_10BIT is defined as 0x1. > I2C_M_TEN is defined as 0x10. > > It would not work. > > > >> + >> +static int i2c_read_bytewise(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]; >> + uint8_t buf[5]; >> + int ret; >> + int i; >> + >> + for (i = 0; i < len; i++) { >> + i2c_setup_offset(chip, offset, buf, msg); >> + msg->len++; >> + buf[chip->offset_len] = buffer[i]; >> + >> + ret = ops->xfer(bus, msg, 1); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} > > I could not understand how this works. > It seems to send only write transactions. Yes this is the write function, I need to add a read function and rename... > > > >> + >> +static int i2c_bind_driver(struct udevice *bus, uint chip_addr, >> + struct udevice **devp) >> +{ >> + struct dm_i2c_chip *chip; >> + char name[30], *str; >> + struct udevice *dev; >> + int ret; >> + >> + snprintf(name, sizeof(name), "generic_%x", chip_addr); >> + str = strdup(name); >> + ret = device_bind_driver(bus, "i2c_generic_drv", str, &dev); >> + debug("%s: device_bind_driver: ret=%d\n", __func__, ret); >> + if (ret) >> + goto err_bind; >> + >> + /* Tell the device what we know about it */ >> + chip = calloc(1, sizeof(struct dm_i2c_chip)); >> + if (!chip) { >> + ret = -ENOMEM; >> + goto err_mem; >> + } >> + chip->chip_addr = chip_addr; >> + chip->offset_len = 1; /* we assume */ >> + ret = device_probe_child(dev, chip); >> + debug("%s: device_probe_child: ret=%d\n", __func__, ret); >> + free(chip); > > > Why do you need calloc() & free() here? > I think you can use the stack area for "struct dm_i2c_chip chip;" > > > > > > > > >> + >> +UCLASS_DRIVER(i2c) = { >> + .id = UCLASS_I2C, >> + .name = "i2c", >> + .per_device_auto_alloc_size = sizeof(struct dm_i2c_bus), >> + .post_bind = i2c_post_bind, >> + .post_probe = i2c_post_probe, >> +}; >> + >> +UCLASS_DRIVER(i2c_generic) = { >> + .id = UCLASS_I2C_GENERIC, >> + .name = "i2c_generic", >> +}; >> + >> +U_BOOT_DRIVER(i2c_generic_drv) = { > > Perhaps isn't "i2c_generic_chip" clearer than "i2c_generic_drv"? > > > >> + .name = "i2c_generic_drv", >> + .id = UCLASS_I2C_GENERIC, >> +}; > > > Can we move "i2c_generic" to a different file? > maybe, drivers/i2c/i2c-generic.c or drivers/i2c/i2c-generic-chip.c ? > > UCLASS_DRIVER(i2c) is a bus, whereas UCLASS_DRIVER(i2c_generic) is a chip. > > Mixing up a bus and a chip-device together in the same file > looks confusing to me. > > > > >> >> /* >> + * For now there are essentially two parts to this file - driver model >> + * here at the top, and the older code below (with CONFIG_SYS_I2C being >> + * most recent). The plan is to migrate everything to driver model. >> + * The driver model structures and API are separate as they are different >> + * enough as to be incompatible for compilation purposes. >> + */ >> + >> +#ifdef CONFIG_DM_I2C >> + >> +enum dm_i2c_chip_flags { >> + DM_I2C_CHIP_10BIT = 1 << 0, /* Use 10-bit addressing */ >> + DM_I2C_CHIP_RE_ADDRESS = 1 << 1, /* Send address for every byte */ >> +}; > > > As I mentioned above, you define DM_I2C_CHIP_10BIT as 0x1 > whereas you define I2C_M_TEN as 0x0010. > > These flags should be shared with struct i2c_msg. > > > >> +/* >> + * Not all of these flags are implemented in the U-Boot API >> + */ >> +enum dm_i2c_msg_flags { >> + I2C_M_TEN = 0x0010, /* ten-bit chip address */ >> + I2C_M_RD = 0x0001, /* read data, from slave to master */ >> + I2C_M_STOP = 0x8000, /* send stop after this message */ >> + I2C_M_NOSTART = 0x4000, /* no start before this message */ >> + I2C_M_REV_DIR_ADDR = 0x2000, /* invert polarity of R/W bit */ >> + I2C_M_IGNORE_NAK = 0x1000, /* continue after NAK */ >> + I2C_M_NO_RD_ACK = 0x0800, /* skip the Ack bit on reads */ >> + I2C_M_RECV_LEN = 0x0400, /* length is first received byte */ >> +}; > > I think this enum usage is odd. > > If you want to allocate specific values such as 0x8000, 0x4000, etc. > you should use #define instead of enum. > > If you do not care which value is assigned, you can use enum. > arch/arm/include/asm/spl.h is a good example of usage of enum. I prefer enum in most cases - it comes through nicely in the debugger and I don't need brackets around everything. > > > > > > >> +}; >> + >> +/** >> + * 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 { >> + /** >> + * xfer() - transfer a list of I2C messages >> + * >> + * @bus: Bus to read from >> + * @chip_addr: Chip address to read from >> + * @offset: Offset within chip to start reading >> + * @olen: Length of chip offset in bytes >> + * @buffer: Place to put data >> + * @len: Number of bytes to read >> + * @return 0 if OK, -EREMOTEIO if the slave did not ACK a byte, >> + * other -ve value on some other error >> + */ >> + int (*xfer)(struct udevice *bus, struct i2c_msg *msg, int nmsgs); > > > This comment block does not reflect the actual prototype; > chip_addr, offset, ... etc. do not exist any more. > > > > > > Best Regards > Masahiro Yamada > Regards, Simon
Hi Masahiro, On 3 December 2014 at 06:24, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > A little more comments from me. > > > > > On Mon, 24 Nov 2014 11:57:15 -0700 > Simon Glass <sjg@chromium.org> wrote: > >> +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; >> +} > > > This looks odd. > > > If each driver does not have .set_bus_speed handler, > we cannot change the bus speed > because changing the bus speed involves some hardware > register(s) setting. > > We should not change i2c->speed_hz without changing the > actual speed. > > I think the code should be: > > > if (ops->set_bus_speed) { > ret = ops->set_bus_speed(bus, speed); > if (ret) > return ret; > i2c->speed_hz = speed; > } > > I'll add a comment. The idea is that the driver can check the speed and give an error here rather than on the next xfer(). Also if it wants to change the clocks here then it can do so. But otherwise it is OK to deal with the speed change on the next xfer. [snip] Regards, Simon
Hi Simon, 2014-12-04 0:13 GMT+09:00 Simon Glass <sjg@chromium.org>: >> If each driver does not have .set_bus_speed handler, >> we cannot change the bus speed >> because changing the bus speed involves some hardware >> register(s) setting. >> >> We should not change i2c->speed_hz without changing the >> actual speed. >> >> I think the code should be: >> >> >> if (ops->set_bus_speed) { >> ret = ops->set_bus_speed(bus, speed); >> if (ret) >> return ret; >> i2c->speed_hz = speed; >> } >> >> > > I'll add a comment. The idea is that the driver can check the speed > and give an error here rather than on the next xfer(). Also if it > wants to change the clocks here then it can do so. But otherwise it is > OK to deal with the speed change on the next xfer. > OK. Makes sense.
Hi Simon, More comments again. On Mon, 24 Nov 2014 11:57:15 -0700 > + > +static int i2c_probe_chip(struct udevice *bus, uint chip_addr) > +{ > + struct dm_i2c_ops *ops = i2c_get_ops(bus); > + struct i2c_msg msg[1]; > + uint8_t ch = 0; > + > + if (!ops->xfer) > + return -ENOSYS; > + > + msg->addr = chip_addr; > + msg->flags = 0; > + msg->len = 1; > + msg->buf = &ch; > + > + return ops->xfer(bus, msg, 1); > +} i2c_probe_chip() issues a write transaction with one length, but a read transaction should be used. For most of chips, the first write data is the first byte of the offset address, so no real data will be written into the chip. But it is possible to have offset_address_length == 0. The read transaction is always safer than the write transaction. BTW, I implemented an i2c driver for my Panasonic board base on this series, and I am playing around with it. I found a strange behavior. Assume an EEPROM chip is assigned with slave-address 0x50. There is no other chip on this i2c bus. If I run "i2c probe 50" command, it correctly detects the EEPROM chip and create a generic device node "generic_50". If I run "i2c probe 49" command, it simply fails and nothing else happens. On the other hand, when I run "i2c read 49 0.2 1 84000000", it forcibly create a generic device node "generic_49". "i2c read" command directly calls i2c_get_chip() and skips the probing step. This is odd. My "dm tree" output is like this: => dm tree ROOT 9fb49028 - * root_driver @ 9fb49028 `- * soc @ 9fb49098 |- * serial@54006800 @ 9fb49108, 1409312768 |- serial@54006900 @ 9fb49158, 1409313024 |- serial@54006a00 @ 9fb491a8, 1409313280 |- serial@54006b00 @ 9fb491f8, 1409313536 |- * i2c@58400000 @ 9fb49268, 0 ||- * generic_50 @ 9fb51f00 |`- * generic_49 @ 9fb51f70 <--- nothing exists on slave address 0x49 !!! |- i2c@58480000 @ 9fb492b8, 1 |- i2c@58500000 @ 9fb49308, 2 `- i2c@58580000 @ 9fb49358, 3 It should not create any device node for non-existing slave address. I think i2c_get_chip() implementation is wrong. My rough image of the correct implemenation is like this: The key is to split it into i2c_create_chip() and i2c_get_chip(), I think int i2c_probe ( .... ) { i2c_probe_chip(); if (failed) return; i2c_create_chip() } int i2c_create_chip() { search the suitable chip driver based on DeviceTree if (found) { use it } else { call i2c_bind_driver() to create "generic" chip } } int i2c_get_chip () { search a child node with the given chip_addr if (found) return dev; i2c_probe(); } Best Regards Masahiro Yamada
Hi Masahiro, On 3 December 2014 at 19:01, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > More comments again. > > > On Mon, 24 Nov 2014 11:57:15 -0700 >> + >> +static int i2c_probe_chip(struct udevice *bus, uint chip_addr) >> +{ >> + struct dm_i2c_ops *ops = i2c_get_ops(bus); >> + struct i2c_msg msg[1]; >> + uint8_t ch = 0; >> + >> + if (!ops->xfer) >> + return -ENOSYS; >> + >> + msg->addr = chip_addr; >> + msg->flags = 0; >> + msg->len = 1; >> + msg->buf = &ch; >> + >> + return ops->xfer(bus, msg, 1); >> +} > > i2c_probe_chip() issues a write transaction with one length, > but a read transaction should be used. > > For most of chips, the first write data is the first byte of > the offset address, so no real data will be written into the chip. > > But it is possible to have offset_address_length == 0. > > The read transaction is always safer than the write transaction. > > > > > > > > BTW, I implemented an i2c driver for my Panasonic board base on this series, > and I am playing around with it. > > I found a strange behavior. > > > Assume an EEPROM chip is assigned with slave-address 0x50. > There is no other chip on this i2c bus. > > If I run "i2c probe 50" command, it correctly detects the EEPROM > chip and create a generic device node "generic_50". > If I run "i2c probe 49" command, it simply fails and nothing else happens. > > On the other hand, when I run "i2c read 49 0.2 1 84000000", > it forcibly create a generic device node "generic_49". > "i2c read" command directly calls i2c_get_chip() and skips the probing step. > This is odd. > > My "dm tree" output is like this: > > => dm tree > ROOT 9fb49028 > - * root_driver @ 9fb49028 > `- * soc @ 9fb49098 > |- * serial@54006800 @ 9fb49108, 1409312768 > |- serial@54006900 @ 9fb49158, 1409313024 > |- serial@54006a00 @ 9fb491a8, 1409313280 > |- serial@54006b00 @ 9fb491f8, 1409313536 > |- * i2c@58400000 @ 9fb49268, 0 > ||- * generic_50 @ 9fb51f00 > |`- * generic_49 @ 9fb51f70 <--- nothing exists on slave address 0x49 !!! > |- i2c@58480000 @ 9fb492b8, 1 > |- i2c@58500000 @ 9fb49308, 2 > `- i2c@58580000 @ 9fb49358, 3 > > It should not create any device node for non-existing slave address. > > > I think i2c_get_chip() implementation is wrong. > > > > My rough image of the correct implemenation is like this: > The key is to split it into i2c_create_chip() and i2c_get_chip(), I think > > > > > int i2c_probe ( .... ) > { > i2c_probe_chip(); > > if (failed) > return; > > i2c_create_chip() > } > > > int i2c_create_chip() > { > > search the suitable chip driver based on DeviceTree > > > if (found) { > use it > } else { > call i2c_bind_driver() to create "generic" chip > } > } > > > > int i2c_get_chip () > { > search a child node with the given chip_addr > > if (found) > return dev; > > > i2c_probe(); > } > But that would change the bahaviour - it would then become impossible to access a chip that does not probe. The probe is a feature of the uclass, but it does not gate use of a device. In fact with the device tree we will typically create devices without probing them. Regards, Simon
Hi Masahiro, On 3 December 2014 at 19:01, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > More comments again. > > > On Mon, 24 Nov 2014 11:57:15 -0700 >> + >> +static int i2c_probe_chip(struct udevice *bus, uint chip_addr) >> +{ >> + struct dm_i2c_ops *ops = i2c_get_ops(bus); >> + struct i2c_msg msg[1]; >> + uint8_t ch = 0; >> + >> + if (!ops->xfer) >> + return -ENOSYS; >> + >> + msg->addr = chip_addr; >> + msg->flags = 0; >> + msg->len = 1; >> + msg->buf = &ch; >> + >> + return ops->xfer(bus, msg, 1); >> +} > > i2c_probe_chip() issues a write transaction with one length, > but a read transaction should be used. > > For most of chips, the first write data is the first byte of > the offset address, so no real data will be written into the chip. > > But it is possible to have offset_address_length == 0. > > The read transaction is always safer than the write transaction. > I originally had a probe method to allow the driver to decide what to do. Many drivers can in fact just write the address and don't need a data / offset byte. But not all. With a read byte the tegra driver takes ages to respond and the probe takes a lot longer than it should. Yes I agree that read is safer, but even safer is nothing. Perhaps I should go back to having a probe_chip() method? Or at least it could be optional. Regards, Simon
Hi Simon, I am testing this series on my board and found some bugs. On Mon, 24 Nov 2014 11:57:15 -0700 Simon Glass <sjg@chromium.org> wrote: > + > +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, > + uint8_t offset_buf[], struct i2c_msg *msg) > +{ > + if (!chip->offset_len) > + return false; > + msg->addr = chip->chip_addr; > + msg->flags = chip->flags; > + msg->len = chip->offset_len; > + msg->buf = offset_buf; > + offset_buf[0] = offset; > + offset_buf[1] = offset >> 8; > + offset_buf[2] = offset >> 16; > + offset_buf[3] = offset >> 24; > + > + return true; > +} The i2c_setup_offset() function includes two bugs. [1] Even if chip->offset_len is zero, it should not return immediately. struct i2c_msg *msg has not been initialized. msg->addr and msg->len include unexpected values and they are passed to the driver. It results in an enexpected behavior. [2] The endian of offset_buf[] should be big endian, not little endian. So, if I rewrote this function locally as follows, it is working file: static bool 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; msg->len = chip->offset_len; msg->buf = offset_buf; offset_len = chip->offset_len; while(offset_len--) *offset_buf++ = offset >> (8 * offset_len); return true; } Best Regards Masahiro Yamada
Hi Masahiro, On 3 December 2014 at 21:36, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > I am testing this series on my board > and found some bugs. > > > > > On Mon, 24 Nov 2014 11:57:15 -0700 > Simon Glass <sjg@chromium.org> wrote: > > >> + >> +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, >> + uint8_t offset_buf[], struct i2c_msg *msg) >> +{ >> + if (!chip->offset_len) >> + return false; >> + msg->addr = chip->chip_addr; >> + msg->flags = chip->flags; >> + msg->len = chip->offset_len; >> + msg->buf = offset_buf; >> + offset_buf[0] = offset; >> + offset_buf[1] = offset >> 8; >> + offset_buf[2] = offset >> 16; >> + offset_buf[3] = offset >> 24; >> + >> + return true; >> +} > > > The i2c_setup_offset() function includes two bugs. > > [1] Even if chip->offset_len is zero, it should not > return immediately. > > struct i2c_msg *msg has not been initialized. > msg->addr and msg->len include unexpected values > and they are passed to the driver. > > It results in an enexpected behavior. > > > [2] The endian of offset_buf[] should be big endian, > not little endian. > > > > So, if I rewrote this function locally as follows, it is working file: > > > > static bool 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; > msg->len = chip->offset_len; > msg->buf = offset_buf; > > offset_len = chip->offset_len; > > while(offset_len--) > *offset_buf++ = offset >> (8 * offset_len); > > return true; > } > Thanks. I am about half-way through finishing the unit tests and have found several other bugs. I never did get around to finishing the tests when I put the original patches together. But don't worry, I will not merge this until we have reasonable test coverage. I will add tests for your bugs also - I had not noticed the offset endian problem! Regards, Simon
Hi Simon, On Wed, 3 Dec 2014 19:32:18 -0700 Simon Glass <sjg@chromium.org> wrote: > > > > BTW, I implemented an i2c driver for my Panasonic board base on this series, > > and I am playing around with it. > > > > I found a strange behavior. > > > > > > Assume an EEPROM chip is assigned with slave-address 0x50. > > There is no other chip on this i2c bus. > > > > If I run "i2c probe 50" command, it correctly detects the EEPROM > > chip and create a generic device node "generic_50". > > If I run "i2c probe 49" command, it simply fails and nothing else happens. > > > > On the other hand, when I run "i2c read 49 0.2 1 84000000", > > it forcibly create a generic device node "generic_49". > > "i2c read" command directly calls i2c_get_chip() and skips the probing step. > > This is odd. > > > > My "dm tree" output is like this: > > > > => dm tree > > ROOT 9fb49028 > > - * root_driver @ 9fb49028 > > `- * soc @ 9fb49098 > > |- * serial@54006800 @ 9fb49108, 1409312768 > > |- serial@54006900 @ 9fb49158, 1409313024 > > |- serial@54006a00 @ 9fb491a8, 1409313280 > > |- serial@54006b00 @ 9fb491f8, 1409313536 > > |- * i2c@58400000 @ 9fb49268, 0 > > ||- * generic_50 @ 9fb51f00 > > |`- * generic_49 @ 9fb51f70 <--- nothing exists on slave address 0x49 !!! > > |- i2c@58480000 @ 9fb492b8, 1 > > |- i2c@58500000 @ 9fb49308, 2 > > `- i2c@58580000 @ 9fb49358, 3 > > > > It should not create any device node for non-existing slave address. > > > > > > I think i2c_get_chip() implementation is wrong. > > > > > > > > My rough image of the correct implemenation is like this: > > The key is to split it into i2c_create_chip() and i2c_get_chip(), I think > > > > > > > > > > int i2c_probe ( .... ) > > { > > i2c_probe_chip(); > > > > if (failed) > > return; > > > > i2c_create_chip() > > } > > > > > > int i2c_create_chip() > > { > > > > search the suitable chip driver based on DeviceTree > > > > > > if (found) { > > use it > > } else { > > call i2c_bind_driver() to create "generic" chip > > } > > } > > > > > > > > int i2c_get_chip () > > { > > search a child node with the given chip_addr > > > > if (found) > > return dev; > > > > > > i2c_probe(); > > } > > > > But that would change the bahaviour - it would then become impossible > to access a chip that does not probe. What's the problem? If it does not probe, it means no device exists on that slave address. Trying further access against non-exising device has no point. > The probe is a feature of the > uclass, but it does not gate use of a device. In fact with the device > tree we will typically create devices without probing them. Precisely, devices are *bound* without probing, but should not be activated until we make sure the device really exists on the bus. > > => dm tree > > ROOT 9fb49028 > > - * root_driver @ 9fb49028 > > `- * soc @ 9fb49098 > > |- * serial@54006800 @ 9fb49108, 1409312768 > > |- serial@54006900 @ 9fb49158, 1409313024 > > |- serial@54006a00 @ 9fb491a8, 1409313280 > > |- serial@54006b00 @ 9fb491f8, 1409313536 > > |- * i2c@58400000 @ 9fb49268, 0 > > ||- * generic_50 @ 9fb51f00 > > |`- * generic_49 @ 9fb51f70 <--- nothing exists on slave address 0x49 !!! > > |- i2c@58480000 @ 9fb492b8, 1 > > |- i2c@58500000 @ 9fb49308, 2 > > `- i2c@58580000 @ 9fb49358, 3 Here, while generic_49 does not really exist, nevertheless it is activated (has '*' mark). Very strange. Best Regards Masahiro Yamada
Hi Simon, On Wed, 3 Dec 2014 19:36:15 -0700 Simon Glass <sjg@chromium.org> wrote: > Hi Masahiro, > > On 3 December 2014 at 19:01, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > > Hi Simon, > > > > > > More comments again. > > > > > > On Mon, 24 Nov 2014 11:57:15 -0700 > >> + > >> +static int i2c_probe_chip(struct udevice *bus, uint chip_addr) > >> +{ > >> + struct dm_i2c_ops *ops = i2c_get_ops(bus); > >> + struct i2c_msg msg[1]; > >> + uint8_t ch = 0; > >> + > >> + if (!ops->xfer) > >> + return -ENOSYS; > >> + > >> + msg->addr = chip_addr; > >> + msg->flags = 0; > >> + msg->len = 1; > >> + msg->buf = &ch; > >> + > >> + return ops->xfer(bus, msg, 1); > >> +} > > > > i2c_probe_chip() issues a write transaction with one length, > > but a read transaction should be used. > > > > For most of chips, the first write data is the first byte of > > the offset address, so no real data will be written into the chip. > > > > But it is possible to have offset_address_length == 0. > > > > The read transaction is always safer than the write transaction. > > > > I originally had a probe method to allow the driver to decide what to > do. Many drivers can in fact just write the address and don't need a > data / offset byte. But not all. With a read byte the tegra driver > takes ages to respond and the probe takes a lot longer than it should. I can't believe this. Reading one byte should be done pretty fast. If not, the driver implemetation has crewed up. > Yes I agree that read is safer, but even safer is nothing. Perhaps I > should go back to having a probe_chip() method? Or at least it could > be optional. Safety takes precedence over efficiency. I don't think we should go back to a probe_chip(). Probing happens only one time before the use of I2C. Even if it takes longer, no problem. Best Regards Masahiro Yamada
Hi Masahiro, On 4 December 2014 at 00:24, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > > > On Wed, 3 Dec 2014 19:32:18 -0700 > Simon Glass <sjg@chromium.org> wrote: > >> > >> > BTW, I implemented an i2c driver for my Panasonic board base on this series, >> > and I am playing around with it. >> > >> > I found a strange behavior. >> > >> > >> > Assume an EEPROM chip is assigned with slave-address 0x50. >> > There is no other chip on this i2c bus. >> > >> > If I run "i2c probe 50" command, it correctly detects the EEPROM >> > chip and create a generic device node "generic_50". >> > If I run "i2c probe 49" command, it simply fails and nothing else happens. >> > >> > On the other hand, when I run "i2c read 49 0.2 1 84000000", >> > it forcibly create a generic device node "generic_49". >> > "i2c read" command directly calls i2c_get_chip() and skips the probing step. >> > This is odd. >> > >> > My "dm tree" output is like this: >> > >> > => dm tree >> > ROOT 9fb49028 >> > - * root_driver @ 9fb49028 >> > `- * soc @ 9fb49098 >> > |- * serial@54006800 @ 9fb49108, 1409312768 >> > |- serial@54006900 @ 9fb49158, 1409313024 >> > |- serial@54006a00 @ 9fb491a8, 1409313280 >> > |- serial@54006b00 @ 9fb491f8, 1409313536 >> > |- * i2c@58400000 @ 9fb49268, 0 >> > ||- * generic_50 @ 9fb51f00 >> > |`- * generic_49 @ 9fb51f70 <--- nothing exists on slave address 0x49 !!! >> > |- i2c@58480000 @ 9fb492b8, 1 >> > |- i2c@58500000 @ 9fb49308, 2 >> > `- i2c@58580000 @ 9fb49358, 3 >> > >> > It should not create any device node for non-existing slave address. >> > >> > >> > I think i2c_get_chip() implementation is wrong. >> > >> > >> > >> > My rough image of the correct implemenation is like this: >> > The key is to split it into i2c_create_chip() and i2c_get_chip(), I think >> > >> > >> > >> > >> > int i2c_probe ( .... ) >> > { >> > i2c_probe_chip(); >> > >> > if (failed) >> > return; >> > >> > i2c_create_chip() >> > } >> > >> > >> > int i2c_create_chip() >> > { >> > >> > search the suitable chip driver based on DeviceTree >> > >> > >> > if (found) { >> > use it >> > } else { >> > call i2c_bind_driver() to create "generic" chip >> > } >> > } >> > >> > >> > >> > int i2c_get_chip () >> > { >> > search a child node with the given chip_addr >> > >> > if (found) >> > return dev; >> > >> > >> > i2c_probe(); >> > } >> > >> >> But that would change the bahaviour - it would then become impossible >> to access a chip that does not probe. > > What's the problem? > If it does not probe, it means no device exists on that slave address. > Trying further access against non-exising device has no point. > I have a pretty cast-iron rule that I don't want to change behaviour when boards move to driver model. We can always decide to change things later but I am trying to avoid the situation where people hit problems when switching to driver model. If we have one such problem for each uclass, it could be an insurmountable problem for some people. I don't see a problem with talking to a device which doesn't respond to probe, or in fact isn't even there. It should produce an -EREMOTEIO error. Provided that it does that, things are fine. While it might be considered to have no point, that is the user's decisions. There could be problems with probing, problems with the bus, etc. U-Boot is used for bring-up of new hardware. > >> The probe is a feature of the >> uclass, but it does not gate use of a device. In fact with the device >> tree we will typically create devices without probing them. > > Precisely, devices are *bound* without probing, > but should not be activated until we make sure the device really exists on the bus. > I see this as new behaviour. I would be happy to enable it as an option, presumably at the bus level, with a new flag (although not in this series). > > >> > => dm tree >> > ROOT 9fb49028 >> > - * root_driver @ 9fb49028 >> > `- * soc @ 9fb49098 >> > |- * serial@54006800 @ 9fb49108, 1409312768 >> > |- serial@54006900 @ 9fb49158, 1409313024 >> > |- serial@54006a00 @ 9fb491a8, 1409313280 >> > |- serial@54006b00 @ 9fb491f8, 1409313536 >> > |- * i2c@58400000 @ 9fb49268, 0 >> > ||- * generic_50 @ 9fb51f00 >> > |`- * generic_49 @ 9fb51f70 <--- nothing exists on slave address 0x49 !!! >> > |- i2c@58480000 @ 9fb492b8, 1 >> > |- i2c@58500000 @ 9fb49308, 2 >> > `- i2c@58580000 @ 9fb49358, 3 > > > Here, while generic_49 does not really exist, > nevertheless it is activated (has '*' mark). > Very strange. It does exist in driver model, just not on the bus (yet). The driver model probe() method has been called, and if someone connected a device to the I2C pins then it would start working. There is the option to use 'i2c probe' to get the behaviour you are looking for. I can't understand why you would not use that in this case. Regards, Simon
Hi Masahiro, On 4 December 2014 at 00:27, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: > Hi Simon, > > > > On Wed, 3 Dec 2014 19:36:15 -0700 > Simon Glass <sjg@chromium.org> wrote: > >> Hi Masahiro, >> >> On 3 December 2014 at 19:01, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote: >> > Hi Simon, >> > >> > >> > More comments again. >> > >> > >> > On Mon, 24 Nov 2014 11:57:15 -0700 >> >> + >> >> +static int i2c_probe_chip(struct udevice *bus, uint chip_addr) >> >> +{ >> >> + struct dm_i2c_ops *ops = i2c_get_ops(bus); >> >> + struct i2c_msg msg[1]; >> >> + uint8_t ch = 0; >> >> + >> >> + if (!ops->xfer) >> >> + return -ENOSYS; >> >> + >> >> + msg->addr = chip_addr; >> >> + msg->flags = 0; >> >> + msg->len = 1; >> >> + msg->buf = &ch; >> >> + >> >> + return ops->xfer(bus, msg, 1); >> >> +} >> > >> > i2c_probe_chip() issues a write transaction with one length, >> > but a read transaction should be used. >> > >> > For most of chips, the first write data is the first byte of >> > the offset address, so no real data will be written into the chip. >> > >> > But it is possible to have offset_address_length == 0. >> > >> > The read transaction is always safer than the write transaction. >> > >> >> I originally had a probe method to allow the driver to decide what to >> do. Many drivers can in fact just write the address and don't need a >> data / offset byte. But not all. With a read byte the tegra driver >> takes ages to respond and the probe takes a lot longer than it should. > > I can't believe this. > Reading one byte should be done pretty fast. > If not, the driver implemetation has crewed up. > > >> Yes I agree that read is safer, but even safer is nothing. Perhaps I >> should go back to having a probe_chip() method? Or at least it could >> be optional. > > Safety takes precedence over efficiency. > > I don't think we should go back to a probe_chip(). > > Probing happens only one time before the use of I2C. > Even if it takes longer, no problem. This would still be considered a regression. Yes I suspect that the Tegra driver has a problem, but I'm not sure. I don't want to be in the business of looking for bugs in all the drivers just so they can convert to driver model without problems. IMO the right way to probe is to write the address with no data at all. I was hoping that there was a standard probe method which would work with all drivers but I think that might have been a mistake. I'm going to reinstate the probe() method and make it optional. That way drivers with particular needs can still use it, but driver model will have a default probe implementation. That implementation will just send a zero-length message. That should resolve your concern and mine. Regards, Simon
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index dae3d71..063e097 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -4,6 +4,7 @@ # # SPDX-License-Identifier: GPL-2.0+ # +obj-$(CONFIG_DM_I2C) += i2c-uclass.o obj-$(CONFIG_SYS_I2C_ADI) += adi_i2c.o obj-$(CONFIG_I2C_MV) += mv_i2c.o diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c new file mode 100644 index 0000000..4063a80 --- /dev/null +++ b/drivers/i2c/i2c-uclass.c @@ -0,0 +1,411 @@ +/* + * Copyright (c) 2014 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <dm.h> +#include <errno.h> +#include <fdtdec.h> +#include <i2c.h> +#include <malloc.h> +#include <dm/device-internal.h> +#include <dm/lists.h> +#include <dm/root.h> + +DECLARE_GLOBAL_DATA_PTR; + +static bool i2c_setup_offset(struct dm_i2c_chip *chip, uint offset, + uint8_t offset_buf[], struct i2c_msg *msg) +{ + if (!chip->offset_len) + return false; + msg->addr = chip->chip_addr; + msg->flags = chip->flags; + msg->len = chip->offset_len; + msg->buf = offset_buf; + offset_buf[0] = offset; + offset_buf[1] = offset >> 8; + offset_buf[2] = offset >> 16; + offset_buf[3] = offset >> 24; + + return true; +} + +static int i2c_read_bytewise(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]; + uint8_t buf[5]; + int ret; + int i; + + for (i = 0; i < len; i++) { + i2c_setup_offset(chip, offset, buf, msg); + msg->len++; + buf[chip->offset_len] = buffer[i]; + + ret = ops->xfer(bus, msg, 1); + if (ret) + return ret; + } + + return 0; +} + +int i2c_read(struct udevice *dev, uint offset, 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[2], *ptr; + uint8_t offset_buf[4]; + int msg_count; + + if (!ops->xfer) + return -ENOSYS; + if (chip->flags & DM_I2C_CHIP_RE_ADDRESS) + return i2c_read_bytewise(dev, offset, buffer, len); + ptr = msg; + if (i2c_setup_offset(chip, offset, offset_buf, ptr)) + ptr++; + + if (len) { + ptr->addr = chip->chip_addr; + ptr->flags = chip->flags | I2C_M_RD; + ptr->len = len; + ptr->buf = buffer; + ptr++; + } + msg_count = ptr - msg; + + return ops->xfer(bus, msg, msg_count); +} + +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; + + /* + * The simple approach would be to send two messages here: one to + * set the offset and one to write the bytes. However some drivers + * will not be expecting this, and some chips won't like how the + * driver presents this on the I2C bus. + * + * The API does not support separate offset and data. We could extend + * it with a flag indicating that there is data in the next message + * that needs to be processed in the same transaction. We could + * instead add an additional buffer to each message. For now, handle + * this in the uclass since it isn't clear what the impact on drivers + * would be with this extra complication. Unfortunately this means + * copying the message. + * + * Use the stack for small messages, malloc() for larger ones. We + * need to allow space for the offset (up to 4 bytes) and the message + * itself. + */ + if (len < 64) { + uint8_t buf[4 + len]; + + i2c_setup_offset(chip, offset, buf, msg); + msg->len += len; + memcpy(buf + chip->offset_len, buffer, len); + + return ops->xfer(bus, msg, 1); + } else { + uint8_t *buf; + int ret; + + buf = malloc(4 + len); + if (!buf) + return -ENOMEM; + i2c_setup_offset(chip, offset, buf, msg); + msg->len += len; + memcpy(buf + chip->offset_len, buffer, len); + + ret = ops->xfer(bus, msg, 1); + free(buf); + return ret; + } +} + +static int i2c_probe_chip(struct udevice *bus, uint chip_addr) +{ + struct dm_i2c_ops *ops = i2c_get_ops(bus); + struct i2c_msg msg[1]; + uint8_t ch = 0; + + if (!ops->xfer) + return -ENOSYS; + + msg->addr = chip_addr; + msg->flags = 0; + msg->len = 1; + msg->buf = &ch; + + return ops->xfer(bus, msg, 1); +} + +static int i2c_bind_driver(struct udevice *bus, uint chip_addr, + struct udevice **devp) +{ + struct dm_i2c_chip *chip; + char name[30], *str; + struct udevice *dev; + int ret; + + snprintf(name, sizeof(name), "generic_%x", chip_addr); + str = strdup(name); + ret = device_bind_driver(bus, "i2c_generic_drv", str, &dev); + debug("%s: device_bind_driver: ret=%d\n", __func__, ret); + if (ret) + goto err_bind; + + /* Tell the device what we know about it */ + chip = calloc(1, sizeof(struct dm_i2c_chip)); + if (!chip) { + ret = -ENOMEM; + goto err_mem; + } + chip->chip_addr = chip_addr; + chip->offset_len = 1; /* we assume */ + ret = device_probe_child(dev, chip); + debug("%s: device_probe_child: ret=%d\n", __func__, ret); + free(chip); + if (ret) + goto err_probe; + + *devp = dev; + return 0; + +err_probe: +err_mem: + device_unbind(dev); +err_bind: + free(str); + return ret; +} + +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; + } + } + debug("not found\n"); + return i2c_bind_driver(bus, chip_addr, devp); +} + +int i2c_get_chip_for_busnum(int busnum, int chip_addr, struct udevice **devp) +{ + struct udevice *bus; + int ret; + + ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus); + if (ret) { + debug("Cannot find I2C bus %d\n", busnum); + return ret; + } + ret = i2c_get_chip(bus, chip_addr, devp); + if (ret) { + debug("Cannot find I2C chip %02x on bus %d\n", chip_addr, + busnum); + return ret; + } + + return 0; +} + +int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp) +{ + int ret; + + *devp = NULL; + + /* First probe that chip */ + ret = i2c_probe_chip(bus, chip_addr); + debug("%s: bus='%s', address %02x, ret=%d\n", __func__, bus->name, + chip_addr, ret); + if (ret) + return ret; + + /* 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); + + return ret; +} + +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_chip_flags(struct udevice *dev, uint flags) +{ + 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 (ops->set_flags) { + ret = ops->set_flags(dev, flags); + if (ret) + return ret; + } + chip->flags = flags; + + return 0; +} + +int i2c_get_chip_flags(struct udevice *dev, uint *flagsp) +{ + struct dm_i2c_chip *chip = dev_get_parentdata(dev); + + *flagsp = chip->flags; + + return 0; +} + +int i2c_set_chip_offset_len(struct udevice *dev, uint offset_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 (offset_len > 3) + return -EINVAL; + if (ops->set_offset_len) { + ret = ops->set_offset_len(dev, offset_len); + if (ret) + return ret; + } + chip->offset_len = offset_len; + + return 0; +} + +int i2c_deblock(struct udevice *bus) +{ + struct dm_i2c_ops *ops = i2c_get_ops(bus); + + /* + * We could implement a software deblocking here if we could get + * access to the GPIOs used by I2C, and switch them to GPIO mode + * and then back to I2C. This is somewhat beyond our powers in + * driver model at present, so for now just fail. + * + * See https://patchwork.ozlabs.org/patch/399040/ + */ + if (!ops->deblock) + return -ENOSYS; + + return ops->deblock(bus); +} + +int i2c_chip_ofdata_to_platdata(const void *blob, int node, + struct dm_i2c_chip *chip) +{ + chip->offset_len = 1; /* default */ + chip->flags = 0; + chip->chip_addr = fdtdec_get_int(gd->fdt_blob, node, "reg", -1); + if (chip->chip_addr == -1) { + debug("%s: I2C Node '%s' has no 'reg' property\n", __func__, + fdt_get_name(blob, node, NULL)); + return -EINVAL; + } + + return 0; +} + +static int i2c_post_probe(struct udevice *dev) +{ + struct dm_i2c_bus *i2c = dev->uclass_priv; + + i2c->speed_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset, + "clock-frequency", 100000); + + return i2c_set_bus_speed(dev, i2c->speed_hz); +} + +int i2c_post_bind(struct udevice *dev) +{ + /* Scan the bus for devices */ + return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false); +} + +UCLASS_DRIVER(i2c) = { + .id = UCLASS_I2C, + .name = "i2c", + .per_device_auto_alloc_size = sizeof(struct dm_i2c_bus), + .post_bind = i2c_post_bind, + .post_probe = i2c_post_probe, +}; + +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, +}; diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h index 7d8daa2..91a0a43 100644 --- a/include/config_fallbacks.h +++ b/include/config_fallbacks.h @@ -87,4 +87,10 @@ #undef CONFIG_IMAGE_FORMAT_LEGACY #endif +#ifdef CONFIG_DM_I2C +# ifdef CONFIG_SYS_I2C +# error "Cannot define CONFIG_SYS_I2C when CONFIG_DM_I2C is used" +# endif +#endif + #endif /* __CONFIG_FALLBACKS_H */ diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index a8944c9..43514bc 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -28,6 +28,8 @@ enum uclass_id { UCLASS_SPI_GENERIC, /* Generic SPI flash target */ UCLASS_SPI_FLASH, /* SPI flash */ UCLASS_CROS_EC, /* Chrome OS EC */ + UCLASS_I2C, /* I2C bus */ + UCLASS_I2C_GENERIC, /* Generic I2C device */ UCLASS_COUNT, UCLASS_INVALID = -1, diff --git a/include/i2c.h b/include/i2c.h index 1b4078e..dd8fd2e 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -18,6 +18,351 @@ #define _I2C_H_ /* + * For now there are essentially two parts to this file - driver model + * here at the top, and the older code below (with CONFIG_SYS_I2C being + * most recent). The plan is to migrate everything to driver model. + * The driver model structures and API are separate as they are different + * enough as to be incompatible for compilation purposes. + */ + +#ifdef CONFIG_DM_I2C + +enum dm_i2c_chip_flags { + DM_I2C_CHIP_10BIT = 1 << 0, /* Use 10-bit addressing */ + DM_I2C_CHIP_RE_ADDRESS = 1 << 1, /* Send address for every byte */ +}; + +/** + * struct dm_i2c_chip - information about an i2c chip + * + * An I2C chip is a device on the I2C bus. It sits at a particular address + * and normally supports 7-bit or 10-bit addressing. + * + * To obtain this structure, use dev_get_parentdata(dev) where dev is the + * chip to examine. + * + * @chip_addr: Chip address on bus + * @offset_len: Length of offset in bytes. A single byte offset can + * represent up to 256 bytes. A value larger than 1 may be + * needed for larger devices. + * @flags: Flags for this chip (dm_i2c_chip_flags) + * @emul: Emulator for this chip address (only used for emulation) + */ +struct dm_i2c_chip { + uint chip_addr; + uint offset_len; + uint flags; +#ifdef CONFIG_SANDBOX + struct udevice *emul; +#endif +}; + +/** + * struct dm_i2c_bus- information about an i2c bus + * + * An I2C bus contains 0 or more chips on it, each at its own address. The + * bus can operate at different speeds (measured in Hz, typically 100KHz + * or 400KHz). + * + * To obtain this structure, use bus->uclass_priv where bus is the I2C + * bus udevice. + * + * @speed_hz: Bus speed in hertz (typically 100000) + */ +struct dm_i2c_bus { + int speed_hz; +}; + +/** + * i2c_read() - read bytes from an I2C chip + * + * To obtain an I2C device (called a 'chip') given the I2C bus address you + * can use i2c_get_chip(). To obtain a bus by bus number use + * uclass_get_device_by_seq(UCLASS_I2C, <bus number>). + * + * To set the address length of a devce use i2c_set_addr_len(). It + * defaults to 1. + * + * @dev: Chip to read from + * @offset: Offset within chip to start reading + * @buffer: Place to put data + * @len: Number of bytes to read + * + * @return 0 on success, -ve on failure + */ +int i2c_read(struct udevice *dev, uint offset, uint8_t *buffer, + int len); + +/** + * i2c_write() - write bytes to an I2C chip + * + * See notes for i2c_read() above. + * + * @dev: Chip to write to + * @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 i2c_write(struct udevice *dev, uint offset, const uint8_t *buffer, + int len); + +/** + * i2c_probe() - probe a particular chip address + * + * This can be useful to check for the existence of a chip on the bus. + * It is typically implemented by writing the chip address to the bus + * and checking that the chip replies with an ACK. + * + * @bus: Bus to probe + * @chip_addr: 7-bit address to probe (10-bit and others are not supported) + * @devp: Returns the device found, or NULL if none + * @return 0 if a chip was found at that address, -ve if not + */ +int i2c_probe(struct udevice *bus, uint chip_addr, struct udevice **devp); + +/** + * i2c_set_bus_speed() - set the speed of a bus + * + * @bus: Bus to adjust + * @speed: Requested speed in Hz + * @return 0 if OK, -EINVAL for invalid values + */ +int i2c_set_bus_speed(struct udevice *bus, unsigned int speed); + +/** + * i2c_get_bus_speed() - get the speed of a bus + * + * @bus: Bus to check + * @return speed of selected I2C bus in Hz, -ve on error + */ +int i2c_get_bus_speed(struct udevice *bus); + +/** + * i2c_set_chip_flags() - set flags for a chip + * + * Typically addresses are 7 bits, but for 10-bit addresses you should set + * flags to DM_I2C_CHIP_10BIT. All accesses will then use 10-bit addressing. + * + * @dev: Chip to adjust + * @flags: New flags + * @return 0 if OK, -EINVAL if value is unsupported, other -ve value on error + */ +int i2c_set_chip_flags(struct udevice *dev, uint flags); + +/** + * i2c_get_chip_flags() - get flags for a chip + * + * @dev: Chip to check + * @flagsp: Place to put flags + * @return 0 if OK, other -ve value on error + */ +int i2c_get_chip_flags(struct udevice *dev, uint *flagsp); + +/** + * i2c_set_offset_len() - set the offset length for a chip + * + * The offset used to access a chip may be up to 4 bytes long. Typically it + * is only 1 byte, which is enough for chips with 256 bytes of memory or + * registers. The default value is 1, but you can call this function to + * change it. + * + * @offset_len: New offset length value (typically 1 or 2) + */ + +int i2c_set_chip_offset_len(struct udevice *dev, uint offset_len); +/** + * i2c_deblock() - recover a bus that is in an unknown state + * + * See the deblock() method in 'struct dm_i2c_ops' for full information + * + * @bus: Bus to recover + * @return 0 if OK, -ve on error + */ +int i2c_deblock(struct udevice *bus); + +/* + * Not all of these flags are implemented in the U-Boot API + */ +enum dm_i2c_msg_flags { + I2C_M_TEN = 0x0010, /* ten-bit chip address */ + I2C_M_RD = 0x0001, /* read data, from slave to master */ + I2C_M_STOP = 0x8000, /* send stop after this message */ + I2C_M_NOSTART = 0x4000, /* no start before this message */ + I2C_M_REV_DIR_ADDR = 0x2000, /* invert polarity of R/W bit */ + I2C_M_IGNORE_NAK = 0x1000, /* continue after NAK */ + I2C_M_NO_RD_ACK = 0x0800, /* skip the Ack bit on reads */ + I2C_M_RECV_LEN = 0x0400, /* length is first received byte */ +}; + +/** + * struct i2c_msg - an I2C message + * + * @addr: Slave address + * @flags: Flags (see enum dm_i2c_msg_flags) + * @len: Length of buffer in bytes + * @buf: Buffer to send/receive + */ +struct i2c_msg { + uint addr; + uint flags; + uint len; + u8 *buf; +}; + +/** + * struct i2c_msg_list - a list of I2C messages + * + * This is called i2c_rdwr_ioctl_data in Linux but the name does not seem + * appropriate in U-Boot. + * + * @msg: Pointer to i2c_msg array + * @nmsgs: Number of elements in the array + */ +struct i2c_msg_list { + struct i2c_msg *msgs; + uint nmsgs; +}; + +/** + * 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 { + /** + * xfer() - transfer a list of I2C messages + * + * @bus: Bus to read from + * @chip_addr: Chip address to read from + * @offset: Offset within chip to start reading + * @olen: Length of chip offset in bytes + * @buffer: Place to put data + * @len: Number of bytes to read + * @return 0 if OK, -EREMOTEIO if the slave did not ACK a byte, + * other -ve value on some other error + */ + int (*xfer)(struct udevice *bus, struct i2c_msg *msg, int nmsgs); + + /** + * 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. + * + * @bus: Bus to adjust + * @speed: Requested speed in Hz + * @return 0 if OK, -INVAL for invalid values + */ + int (*set_bus_speed)(struct udevice *bus, unsigned int speed); + + /** + * get_bus_speed() - get the speed of a bus (optional) + * + * Normally this can be provided by the uclass, but if you want your + * driver to check the bus speed by looking at the hardware, you can + * implement that here. + * + * @bus: Bus to check + * @return speed of selected I2C bus in Hz, -ve on error + */ + int (*get_bus_speed)(struct udevice *bus); + + /** + * set_flags() - set the flags for a chip (optional) + * + * This is generally implemented by the uclass, but drivers can + * check the value to ensure that unsupported options are not used. + * If provided, this method will always be called when the flags + * change. + * + * @dev: Chip to adjust + * @flags: New flags value + * @return 0 if OK, -EINVAL if value is unsupported + */ + int (*set_flags)(struct udevice *dev, uint flags); + + /** + * set_offset_len() - set the offset length of a chip (optional) + * + * This is generally implemented by the uclass, but drivers can + * check the value to ensure that unsupported options are not used. + * If provided, this method will always be called when the offset + * length changes from the default of 1. + * + * @dev: Chip to adjust + * @addr_len: New offset length value (typically 1 or 2) + * @return 0 if OK, -INVAL if value is unsupported + */ + int (*set_offset_len)(struct udevice *dev, uint offset_len); + + /** + * deblock() - recover a bus that is in an unknown state + * + * I2C is a synchronous protocol and resets of the processor in the + * middle of an access can block the I2C Bus until a powerdown of + * the full unit is done. This is because slaves can be stuck + * waiting for addition bus transitions for a transaction that will + * never complete. Resetting the I2C master does not help. The only + * way is to force the bus through a series of transitions to make + * sure that all slaves are done with the transaction. This method + * performs this 'deblocking' if support by the driver. + * + * This method is optional. + */ + int (*deblock)(struct udevice *bus); +}; + +#define i2c_get_ops(dev) ((struct dm_i2c_ops *)(dev)->driver->ops) + +/** + * i2c_get_chip() - get a device to use to access a chip on a bus + * + * This returns the device for the given chip address. The device can then + * be used with calls to i2c_read(), i2c_write(), i2c_probe(), etc. + * + * @bus: Bus to examine + * @chip_addr: Chip address for the new device + * @devp: Returns pointer to new device if found or -ENODEV if not + * found + */ +int i2c_get_chip(struct udevice *bus, uint chip_addr, struct udevice **devp); + +/** + * i2c_get_chip() - get a device to use to access a chip on a bus number + * + * This returns the device for the given chip address on a particular bus + * number. + * + * @busnum: Bus number to examine + * @chip_addr: Chip address for the new device + * @devp: Returns pointer to new device if found or -ENODEV if not + * found + */ +int i2c_get_chip_for_busnum(int busnum, int chip_addr, struct udevice **devp); + +/** + * i2c_chip_ofdata_to_platdata() - Decode standard I2C platform data + * + * This decodes the chip address from a device tree node and puts it into + * its dm_i2c_chip structure. This should be called in your driver's + * ofdata_to_platdata() method. + * + * @blob: Device tree blob + * @node: Node offset to read from + * @spi: Place to put the decoded information + */ +int i2c_chip_ofdata_to_platdata(const void *blob, int node, + struct dm_i2c_chip *chip); + +#endif + +#ifndef CONFIG_DM_I2C + +/* * WARNING WARNING WARNING WARNING WARNING WARNING WARNING WARNING * * The implementation MUST NOT use static or global variables if the @@ -451,4 +796,7 @@ int i2c_get_bus_num_fdt(int node); * @return 0 if port was reset, -1 if not found */ int i2c_reset_port_fdt(const void *blob, int node); + +#endif /* !CONFIG_DM_I2C */ + #endif /* _I2C_H_ */
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 v3: - Change uclass <=> driver API to use a message list - Correct bogus address len code (was confused with chip address length) - Drop extra call to i2c_bind_driver() in i2c_probe() - Add a helper to query chip flags - Add support for reading a byte at a time with an address for each byte 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 drivers/i2c/Makefile | 1 + drivers/i2c/i2c-uclass.c | 411 +++++++++++++++++++++++++++++++++++++++++++++ include/config_fallbacks.h | 6 + include/dm/uclass-id.h | 2 + include/i2c.h | 348 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 768 insertions(+) create mode 100644 drivers/i2c/i2c-uclass.c