[U-Boot] [PATCH v3 0/10] dm: Add I2C support and convert sandbox, tegra
This series adds I2C support to driver model. It has become apparent that this is a high priority as it is widely used. It follows along to some extent from the SPI conversion.
Several changes are made from the original I2C implementations.
Firstly it is not necessary to specify the chip address with every call, since each chip knows its own address - it is stored in struct dm_i2c_chip which is attached to each chip on the I2C bus. However, this information *is* passed to the driver since I presume most drivers need it and it would be cumbersome to look up in every call.
Secondly there is no concept of a 'current' I2C bus so all associated logic is removed. With driver model i2c_set_bus_num() and i2c_get_bus_num() are not available. Since the chip device specifies both the bus and the chip address, there is no need for this concept. It also causes problems when one driver changes the current bus and forgets to change it back.
Thirdly initialisation is handled by driver model's normal probe() method on each device so there should be no need for i2c_init_all(), i2c_init(), i2c_init_board(), i2c_board_late_init() and board_i2c_init().
I2C muxes are not yet supported. To support these we will need to maintain state of the current mux settings to avoid resetting every mux every time. Probably we need to add a sandbox I2C mux driver to permit testing of this. This can probably be done later.
Platform data is not yet supported either, only device tree. The U_BOOT_I2C_MKENT_COMPLETE() and U_BOOT_I2C_ADAP_COMPLETE() macros are not used. Also struct i2c_adapter is not defined anymore. This will need to be addressed, perhaps as part of converting over a board that does not use device tree, assuming that we want to support this.
The following I2C CONFIGs are no-longer needed when driver model is used:
CONFIG_SYS_I2C_INIT_BOARD - each I2C bus is inited in its probe() method CONFIG_I2C_MULTI_BUS - we always support multi-bus with driver model CONFIG_SYS_MAX_I2C_BUS - the device tree aliases define available buses CONFIG_SYS_I2C_SPEED - the device tree specifies the speed for each bus CONFIG_SYS_I2C - this is the 'new old' API, now deprecated
There are a few SPI patches included here due to a dependency on a new device binding function.
v3 changes the uclass to driver interface significantly. It is now a list of messages to be processed by the driver. This matches the Linux user space API which may be a benefit. It does introduce one complication, which is that the protocol does not support separate chip offset and data. I have enhanced it to do so.
This series is available at u-boot-dm/i2c-working2.
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 - Adjust for slightly altered I2C uclass API - Add new 'i2c flags' command to get/set chip flags - Update for new uclass <=> driver interface - Update emulator for new uclass interface - Update for new uclass <=> emulateor interface - Change driver to support new message-based I2C uclass API
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 - Change alen to int so that it can be -1 (this was a bug) - Call the deblock() method for 'i2c reset' - Update commit message for EEPROM driver - Add a test for automatic binding of generic I2C devices - Add a new asm/test.h header for tests in sandbox - Adjust tegra_i2c_child_pre_probe() to permit generic I2C devices - Correct the compatible strings for I2C buses - Don't init if the speed is 0, since this breaks the controller - Expand coverage to all Tegra boards
Simon Glass (10): dm: i2c: Add a uclass for I2C dm: i2c: Implement driver model support in the i2c command dm: i2c: Add I2C emulation driver for sandbox dm: i2c: Add a sandbox I2C driver dm: i2c: Add an I2C EEPROM simulator dm: i2c: config: Enable I2C for sandbox using driver model dm: i2c: dts: Add an I2C bus for sandbox dm: Add a simple EEPROM driver dm: i2c: Add tests for I2C dm: i2c: tegra: Convert to driver model
arch/arm/cpu/tegra20-common/pmu.c | 21 +- arch/arm/dts/tegra124-jetson-tk1.dts | 1 - arch/arm/dts/tegra124-norrin.dts | 1 - arch/arm/dts/tegra30-tec-ng.dts | 4 + arch/arm/include/asm/arch-tegra/tegra_i2c.h | 2 +- arch/sandbox/dts/sandbox.dts | 17 ++ arch/sandbox/include/asm/test.h | 15 + board/avionic-design/common/tamonten-ng.c | 12 +- board/nvidia/cardhu/cardhu.c | 13 +- board/nvidia/common/board.c | 4 - board/nvidia/dalmore/dalmore.c | 21 +- board/nvidia/whistler/whistler.c | 29 +- board/toradex/apalis_t30/apalis_t30.c | 19 +- common/cmd_i2c.c | 376 +++++++++++++++++++++---- drivers/i2c/Makefile | 2 + drivers/i2c/i2c-emul-uclass.c | 14 + drivers/i2c/i2c-uclass.c | 411 ++++++++++++++++++++++++++++ drivers/i2c/sandbox_i2c.c | 142 ++++++++++ drivers/i2c/tegra_i2c.c | 363 ++++++++---------------- drivers/misc/Makefile | 4 + drivers/misc/i2c_eeprom.c | 51 ++++ drivers/misc/i2c_eeprom_emul.c | 108 ++++++++ drivers/power/tps6586x.c | 27 +- include/config_fallbacks.h | 6 + include/configs/apalis_t30.h | 3 - include/configs/beaver.h | 3 - include/configs/cardhu.h | 5 - include/configs/colibri_t30.h | 3 - include/configs/dalmore.h | 5 - include/configs/jetson-tk1.h | 5 - include/configs/norrin.h | 5 - include/configs/sandbox.h | 6 + include/configs/seaboard.h | 3 - include/configs/tec-ng.h | 5 - include/configs/tegra-common.h | 1 + include/configs/tegra114-common.h | 3 - include/configs/tegra124-common.h | 3 - include/configs/tegra20-common.h | 3 - include/configs/tegra30-common.h | 3 - include/configs/trimslice.h | 3 - include/configs/venice2.h | 5 - include/configs/whistler.h | 3 - include/dm/uclass-id.h | 4 + include/i2c.h | 348 +++++++++++++++++++++++ include/i2c_eeprom.h | 19 ++ include/tps6586x.h | 4 +- test/dm/Makefile | 1 + test/dm/i2c.c | 112 ++++++++ test/dm/test.dts | 17 ++ 49 files changed, 1805 insertions(+), 430 deletions(-) create mode 100644 arch/sandbox/include/asm/test.h create mode 100644 drivers/i2c/i2c-emul-uclass.c create mode 100644 drivers/i2c/i2c-uclass.c create mode 100644 drivers/i2c/sandbox_i2c.c create mode 100644 drivers/misc/i2c_eeprom.c create mode 100644 drivers/misc/i2c_eeprom_emul.c create mode 100644 include/i2c_eeprom.h create mode 100644 test/dm/i2c.c
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_ */
+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 couldextend
* 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 ondrivers
* 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 themessage
* 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 tomaster */
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 thisfunction
* 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 wantyour
* driver to check the bus speed by looking at the hardware, youcan
* 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 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 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 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 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 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 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,
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: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
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
The concept of a 'current bus' is now implemented in the command line rather than in the uclass. Also the address length does not need to be specified with each command - really we should consider dropping this from most commands but it works OK for now.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Adjust for slightly altered I2C uclass API - Add new 'i2c flags' command to get/set chip flags
Changes in v2: - Change alen to int so that it can be -1 (this was a bug) - Call the deblock() method for 'i2c reset'
common/cmd_i2c.c | 376 +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 323 insertions(+), 53 deletions(-)
diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c index c266b88..39f9735 100644 --- a/common/cmd_i2c.c +++ b/common/cmd_i2c.c @@ -69,8 +69,10 @@ #include <bootretry.h> #include <cli.h> #include <command.h> +#include <dm.h> #include <edid.h> #include <environment.h> +#include <errno.h> #include <i2c.h> #include <malloc.h> #include <asm/byteorder.h> @@ -117,6 +119,60 @@ static uchar i2c_no_probes[] = CONFIG_SYS_I2C_NOPROBES;
#define DISP_LINE_LEN 16
+/* + * Default for driver model is to use the chip's existing address length. + * For legacy code, this is not stored, so we need to use a suitable + * default. + */ +#ifdef CONFIG_DM_I2C +#define DEFAULT_ADDR_LEN (-1) +#else +#define DEFAULT_ADDR_LEN 1 +#endif + +#ifdef CONFIG_DM_I2C +static struct udevice *i2c_cur_bus; + +static int i2c_set_bus_num(unsigned int busnum) +{ + struct udevice *bus; + int ret; + + ret = uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus); + if (ret) { + debug("%s: No bus %d\n", __func__, busnum); + return ret; + } + i2c_cur_bus = bus; + + return 0; +} + +static int i2c_get_cur_bus(struct udevice **busp) +{ + if (!i2c_cur_bus) { + puts("No I2C bus selected\n"); + return -ENODEV; + } + *busp = i2c_cur_bus; + + return 0; +} + +static int i2c_get_cur_bus_chip(uint chip_addr, struct udevice **devp) +{ + struct udevice *bus; + int ret; + + ret = i2c_get_cur_bus(&bus); + if (ret) + return ret; + + return i2c_get_chip(bus, chip_addr, devp); +} + +#endif + /** * i2c_init_board() - Board-specific I2C bus init * @@ -143,7 +199,7 @@ void i2c_init_board(void) * * Returns I2C bus speed in Hz. */ -#if !defined(CONFIG_SYS_I2C) +#if !defined(CONFIG_SYS_I2C) && !defined(CONFIG_DM_I2C) /* * TODO: Implement architecture-specific get/set functions * Should go away, if we switched completely to new multibus support @@ -182,12 +238,12 @@ int i2c_set_bus_speed(unsigned int speed) * * Returns the address length. */ -static uint get_alen(char *arg) +static uint get_alen(char *arg, int default_len) { int j; int alen;
- alen = 1; + alen = default_len; for (j = 0; j < 8; j++) { if (arg[j] == '.') { alen = arg[j+1] - '0'; @@ -227,8 +283,13 @@ static int i2c_report_err(int ret, enum i2c_err_op op) static int do_i2c_read ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { u_char chip; - uint devaddr, alen, length; + uint devaddr, length; + int alen; u_char *memaddr; + int ret; +#ifdef CONFIG_DM_I2C + struct udevice *dev; +#endif
if (argc != 5) return CMD_RET_USAGE; @@ -243,7 +304,7 @@ static int do_i2c_read ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv * 2 bytes long. Some day it might be 3 bytes long :-). */ devaddr = simple_strtoul(argv[2], NULL, 16); - alen = get_alen(argv[2]); + alen = get_alen(argv[2], DEFAULT_ADDR_LEN); if (alen > 3) return CMD_RET_USAGE;
@@ -257,18 +318,31 @@ static int do_i2c_read ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv */ memaddr = (u_char *)simple_strtoul(argv[4], NULL, 16);
- if (i2c_read(chip, devaddr, alen, memaddr, length) != 0) { - i2c_report_err(-1, I2C_ERR_READ); - return 1; - } +#ifdef CONFIG_DM_I2C + ret = i2c_get_cur_bus_chip(chip, &dev); + if (!ret && alen != -1) + ret = i2c_set_chip_offset_len(dev, alen); + if (!ret) + ret = i2c_read(dev, devaddr, memaddr, length); +#else + ret = i2c_read(chip, devaddr, alen, memaddr, length); +#endif + if (ret) + return i2c_report_err(ret, I2C_ERR_READ); + return 0; }
static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { u_char chip; - uint devaddr, alen, length; + uint devaddr, length; + int alen; u_char *memaddr; + int ret; +#ifdef CONFIG_DM_I2C + struct udevice *dev; +#endif
if (argc != 5) return cmd_usage(cmdtp); @@ -288,7 +362,7 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ * 2 bytes long. Some day it might be 3 bytes long :-). */ devaddr = simple_strtoul(argv[3], NULL, 16); - alen = get_alen(argv[3]); + alen = get_alen(argv[3], DEFAULT_ADDR_LEN); if (alen > 3) return cmd_usage(cmdtp);
@@ -297,10 +371,22 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ */ length = simple_strtoul(argv[4], NULL, 16);
+#ifdef CONFIG_DM_I2C + ret = i2c_get_cur_bus_chip(chip, &dev); + if (!ret && alen != -1) + ret = i2c_set_chip_offset_len(dev, alen); + if (ret) + return i2c_report_err(ret, I2C_ERR_WRITE); +#endif + while (length-- > 0) { - if (i2c_write(chip, devaddr++, alen, memaddr++, 1) != 0) { - return i2c_report_err(-1, I2C_ERR_WRITE); - } +#ifdef CONFIG_DM_I2C + ret = i2c_write(dev, devaddr++, memaddr++, 1); +#else + ret = i2c_write(chip, devaddr++, alen, memaddr++, 1); +#endif + if (ret) + return i2c_report_err(ret, I2C_ERR_WRITE); /* * No write delay with FRAM devices. */ @@ -311,6 +397,38 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ return 0; }
+#ifdef CONFIG_DM_I2C +static int do_i2c_flags(cmd_tbl_t *cmdtp, int flag, int argc, + char *const argv[]) +{ + struct udevice *dev; + uint flags; + int chip; + int ret; + + if (argc < 2) + return CMD_RET_USAGE; + + chip = simple_strtoul(argv[1], NULL, 16); + ret = i2c_get_cur_bus_chip(chip, &dev); + if (ret) + return i2c_report_err(ret, I2C_ERR_READ); + + if (argc > 2) { + flags = simple_strtoul(argv[2], NULL, 16); + ret = i2c_set_chip_flags(dev, flags); + } else { + ret = i2c_get_chip_flags(dev, &flags); + if (!ret) + printf("%x\n", flags); + } + if (ret) + return i2c_report_err(ret, I2C_ERR_READ); + + return 0; +} +#endif + /** * do_i2c_md() - Handle the "i2c md" command-line command * @cmdtp: Command data struct pointer @@ -327,8 +445,13 @@ static int do_i2c_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[ static int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { u_char chip; - uint addr, alen, length; + uint addr, length; + int alen; int j, nbytes, linebytes; + int ret; +#ifdef CONFIG_DM_I2C + struct udevice *dev; +#endif
/* We use the last specified parameters, unless new ones are * entered. @@ -356,7 +479,7 @@ static int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] * 2 bytes long. Some day it might be 3 bytes long :-). */ addr = simple_strtoul(argv[2], NULL, 16); - alen = get_alen(argv[2]); + alen = get_alen(argv[2], DEFAULT_ADDR_LEN); if (alen > 3) return CMD_RET_USAGE;
@@ -368,6 +491,14 @@ static int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] length = simple_strtoul(argv[3], NULL, 16); }
+#ifdef CONFIG_DM_I2C + ret = i2c_get_cur_bus_chip(chip, &dev); + if (!ret && alen != -1) + ret = i2c_set_chip_offset_len(dev, alen); + if (ret) + return i2c_report_err(ret, I2C_ERR_READ); +#endif + /* * Print the lines. * @@ -381,8 +512,13 @@ static int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]
linebytes = (nbytes > DISP_LINE_LEN) ? DISP_LINE_LEN : nbytes;
- if (i2c_read(chip, addr, alen, linebuf, linebytes) != 0) - i2c_report_err(-1, I2C_ERR_READ); +#ifdef CONFIG_DM_I2C + ret = i2c_read(dev, addr, linebuf, linebytes); +#else + ret = i2c_read(chip, addr, alen, linebuf, linebytes); +#endif + if (ret) + i2c_report_err(ret, I2C_ERR_READ); else { printf("%04x:", addr); cp = linebuf; @@ -429,9 +565,13 @@ static int do_i2c_mw ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] { uchar chip; ulong addr; - uint alen; + int alen; uchar byte; int count; + int ret; +#ifdef CONFIG_DM_I2C + struct udevice *dev; +#endif
if ((argc < 4) || (argc > 5)) return CMD_RET_USAGE; @@ -445,10 +585,17 @@ static int do_i2c_mw ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] * Address is always specified. */ addr = simple_strtoul(argv[2], NULL, 16); - alen = get_alen(argv[2]); + alen = get_alen(argv[2], DEFAULT_ADDR_LEN); if (alen > 3) return CMD_RET_USAGE;
+#ifdef CONFIG_DM_I2C + ret = i2c_get_cur_bus_chip(chip, &dev); + if (!ret && alen != -1) + ret = i2c_set_chip_offset_len(dev, alen); + if (ret) + return i2c_report_err(ret, I2C_ERR_WRITE); +#endif /* * Value to write is always specified. */ @@ -463,8 +610,13 @@ static int do_i2c_mw ( cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] count = 1;
while (count-- > 0) { - if (i2c_write(chip, addr++, alen, &byte, 1) != 0) - i2c_report_err(-1, I2C_ERR_WRITE); +#ifdef CONFIG_DM_I2C + ret = i2c_write(dev, addr++, &byte, 1); +#else + ret = i2c_write(chip, addr++, alen, &byte, 1); +#endif + if (ret) + i2c_report_err(ret, I2C_ERR_WRITE); /* * Wait for the write to complete. The write can take * up to 10mSec (we allow a little more time). @@ -499,11 +651,15 @@ static int do_i2c_crc (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] { uchar chip; ulong addr; - uint alen; + int alen; int count; uchar byte; ulong crc; ulong err; + int ret = 0; +#ifdef CONFIG_DM_I2C + struct udevice *dev; +#endif
if (argc < 4) return CMD_RET_USAGE; @@ -517,10 +673,17 @@ static int do_i2c_crc (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] * Address is always specified. */ addr = simple_strtoul(argv[2], NULL, 16); - alen = get_alen(argv[2]); + alen = get_alen(argv[2], DEFAULT_ADDR_LEN); if (alen > 3) return CMD_RET_USAGE;
+#ifdef CONFIG_DM_I2C + ret = i2c_get_cur_bus_chip(chip, &dev); + if (!ret && alen != -1) + ret = i2c_set_chip_offset_len(dev, alen); + if (ret) + return i2c_report_err(ret, I2C_ERR_READ); +#endif /* * Count is always specified */ @@ -534,13 +697,18 @@ static int do_i2c_crc (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] crc = 0; err = 0; while (count-- > 0) { - if (i2c_read(chip, addr, alen, &byte, 1) != 0) +#ifdef CONFIG_DM_I2C + ret = i2c_read(dev, addr, &byte, 1); +#else + ret = i2c_read(chip, addr, alen, &byte, 1); +#endif + if (ret) err++; crc = crc32 (crc, &byte, 1); addr++; } if (err > 0) - i2c_report_err(-1, I2C_ERR_READ); + i2c_report_err(ret, I2C_ERR_READ); else printf ("%08lx\n", crc);
@@ -568,10 +736,14 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char * const arg { uchar chip; ulong addr; - uint alen; + int alen; ulong data; int size = 1; int nbytes; + int ret; +#ifdef CONFIG_DM_I2C + struct udevice *dev; +#endif
if (argc != 3) return CMD_RET_USAGE; @@ -601,19 +773,32 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char * const arg * Address is always specified. */ addr = simple_strtoul(argv[2], NULL, 16); - alen = get_alen(argv[2]); + alen = get_alen(argv[2], DEFAULT_ADDR_LEN); if (alen > 3) return CMD_RET_USAGE; }
+#ifdef CONFIG_DM_I2C + ret = i2c_get_cur_bus_chip(chip, &dev); + if (!ret && alen != -1) + ret = i2c_set_chip_offset_len(dev, alen); + if (ret) + return i2c_report_err(ret, I2C_ERR_WRITE); +#endif + /* * Print the address, followed by value. Then accept input for * the next value. A non-converted value exits. */ do { printf("%08lx:", addr); - if (i2c_read(chip, addr, alen, (uchar *)&data, size) != 0) - i2c_report_err(-1, I2C_ERR_READ); +#ifdef CONFIG_DM_I2C + ret = i2c_read(dev, addr, (uchar *)&data, size); +#else + ret = i2c_read(chip, addr, alen, (uchar *)&data, size); +#endif + if (ret) + i2c_report_err(ret, I2C_ERR_READ); else { data = cpu_to_be32(data); if (size == 1) @@ -655,8 +840,15 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char * const arg * good enough to not time out */ bootretry_reset_cmd_timeout(); - if (i2c_write(chip, addr, alen, (uchar *)&data, size) != 0) - i2c_report_err(-1, I2C_ERR_WRITE); +#ifdef CONFIG_DM_I2C + ret = i2c_write(dev, addr, (uchar *)&data, + size); +#else + ret = i2c_write(chip, addr, alen, + (uchar *)&data, size); +#endif + if (ret) + i2c_report_err(ret, I2C_ERR_WRITE); #ifdef CONFIG_SYS_EEPROM_PAGE_WRITE_DELAY_MS udelay(CONFIG_SYS_EEPROM_PAGE_WRITE_DELAY_MS * 1000); #endif @@ -697,6 +889,13 @@ static int do_i2c_probe (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv int k, skip; unsigned int bus = GET_BUS_NUM; #endif /* NOPROBES */ + int ret; +#ifdef CONFIG_DM_I2C + struct udevice *bus, *dev; + + if (i2c_get_cur_bus(&bus)) + return CMD_RET_FAILURE; +#endif
if (argc == 2) addr = simple_strtol(argv[1], 0, 16); @@ -717,7 +916,12 @@ static int do_i2c_probe (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv if (skip) continue; #endif - if (i2c_probe(j) == 0) { +#ifdef CONFIG_DM_I2C + ret = i2c_probe(bus, j, &dev); +#else + ret = i2c_probe(j); +#endif + if (ret == 0) { printf(" %02X", j); found++; } @@ -754,11 +958,15 @@ static int do_i2c_probe (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv static int do_i2c_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { u_char chip; - ulong alen; + int alen; uint addr; uint length; u_char bytes[16]; int delay; + int ret; +#ifdef CONFIG_DM_I2C + struct udevice *dev; +#endif
if (argc < 3) return CMD_RET_USAGE; @@ -772,9 +980,16 @@ static int do_i2c_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] * Address is always specified. */ addr = simple_strtoul(argv[2], NULL, 16); - alen = get_alen(argv[2]); + alen = get_alen(argv[2], DEFAULT_ADDR_LEN); if (alen > 3) return CMD_RET_USAGE; +#ifdef CONFIG_DM_I2C + ret = i2c_get_cur_bus_chip(chip, &dev); + if (!ret && alen != -1) + ret = i2c_set_chip_offset_len(dev, alen); + if (ret) + return i2c_report_err(ret, I2C_ERR_WRITE); +#endif
/* * Length is the number of objects, not number of bytes. @@ -794,8 +1009,13 @@ static int do_i2c_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[] * Run the loop... */ while (1) { - if (i2c_read(chip, addr, alen, bytes, length) != 0) - i2c_report_err(-1, I2C_ERR_READ); +#ifdef CONFIG_DM_I2C + ret = i2c_read(dev, addr, bytes, length); +#else + ret = i2c_read(chip, addr, alen, bytes, length); +#endif + if (ret) + i2c_report_err(ret, I2C_ERR_READ); udelay(delay); }
@@ -1345,6 +1565,10 @@ int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) { u_char chip; struct edid1_info edid; + int ret; +#ifdef CONFIG_DM_I2C + struct udevice *dev; +#endif
if (argc < 2) { cmd_usage(cmdtp); @@ -1352,10 +1576,15 @@ int do_edid(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) }
chip = simple_strtoul(argv[1], NULL, 16); - if (i2c_read(chip, 0, 1, (uchar *)&edid, sizeof(edid)) != 0) { - i2c_report_err(-1, I2C_ERR_READ); - return 1; - } +#ifdef CONFIG_DM_I2C + ret = i2c_get_cur_bus_chip(chip, &dev); + if (!ret) + ret = i2c_read(dev, 0, (uchar *)&edid, sizeof(edid)); +#else + ret = i2c_read(chip, 0, 1, (uchar *)&edid, sizeof(edid)); +#endif + if (ret) + return i2c_report_err(ret, I2C_ERR_READ);
if (edid_check_info(&edid)) { puts("Content isn't valid EDID.\n"); @@ -1437,17 +1666,28 @@ static int do_i2c_show_bus(cmd_tbl_t *cmdtp, int flag, int argc, * Returns zero on success, CMD_RET_USAGE in case of misuse and negative * on error. */ -#if defined(CONFIG_SYS_I2C) || defined(CONFIG_I2C_MULTI_BUS) +#if defined(CONFIG_SYS_I2C) || defined(CONFIG_I2C_MULTI_BUS) || \ + defined(CONFIG_DM_I2C) static int do_i2c_bus_num(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { int ret = 0; - unsigned int bus_no; + int bus_no;
- if (argc == 1) + if (argc == 1) { /* querying current setting */ - printf("Current bus is %d\n", i2c_get_bus_num()); - else { +#ifdef CONFIG_DM_I2C + struct udevice *bus; + + if (!i2c_get_cur_bus(&bus)) + bus_no = bus->seq; + else + bus_no = -1; +#else + bus_no = i2c_get_bus_num(); +#endif + printf("Current bus is %d\n", bus_no); + } else { bus_no = simple_strtoul(argv[1], NULL, 10); #if defined(CONFIG_SYS_I2C) if (bus_no >= CONFIG_SYS_NUM_I2C_BUSES) { @@ -1478,13 +1718,28 @@ static int do_i2c_bus_speed(cmd_tbl_t * cmdtp, int flag, int argc, char * const { int speed, ret=0;
- if (argc == 1) +#ifdef CONFIG_DM_I2C + struct udevice *bus; + + if (i2c_get_cur_bus(&bus)) + return 1; +#endif + if (argc == 1) { +#ifdef CONFIG_DM_I2C + speed = i2c_get_bus_speed(bus); +#else + speed = i2c_get_bus_speed(); +#endif /* querying current speed */ - printf("Current bus speed=%d\n", i2c_get_bus_speed()); - else { + printf("Current bus speed=%d\n", speed); + } else { speed = simple_strtoul(argv[1], NULL, 10); printf("Setting bus speed to %d Hz\n", speed); +#ifdef CONFIG_DM_I2C + ret = i2c_set_bus_speed(bus, speed); +#else ret = i2c_set_bus_speed(speed); +#endif if (ret) printf("Failure changing bus speed (%d)\n", ret); } @@ -1532,7 +1787,16 @@ static int do_i2c_nm(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[]) */ static int do_i2c_reset(cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[]) { -#if defined(CONFIG_SYS_I2C) +#if defined(CONFIG_DM_I2C) + struct udevice *bus; + + if (i2c_get_cur_bus(&bus)) + return CMD_RET_FAILURE; + if (i2c_deblock(bus)) { + printf("Error: Not supported by the driver\n"); + return CMD_RET_FAILURE; + } +#elif defined(CONFIG_SYS_I2C) i2c_init(I2C_ADAP->speed, I2C_ADAP->slaveaddr); #else i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); @@ -1546,7 +1810,7 @@ static cmd_tbl_t cmd_i2c_sub[] = { #endif U_BOOT_CMD_MKENT(crc32, 3, 1, do_i2c_crc, "", ""), #if defined(CONFIG_SYS_I2C) || \ - defined(CONFIG_I2C_MULTI_BUS) + defined(CONFIG_I2C_MULTI_BUS) || defined(CONFIG_DM_I2C) U_BOOT_CMD_MKENT(dev, 1, 1, do_i2c_bus_num, "", ""), #endif /* CONFIG_I2C_MULTI_BUS */ #if defined(CONFIG_I2C_EDID) @@ -1560,6 +1824,9 @@ static cmd_tbl_t cmd_i2c_sub[] = { U_BOOT_CMD_MKENT(probe, 0, 1, do_i2c_probe, "", ""), U_BOOT_CMD_MKENT(read, 5, 1, do_i2c_read, "", ""), U_BOOT_CMD_MKENT(write, 5, 0, do_i2c_write, "", ""), +#ifdef CONFIG_DM_I2C + U_BOOT_CMD_MKENT(flags, 2, 1, do_i2c_flags, "", ""), +#endif U_BOOT_CMD_MKENT(reset, 0, 1, do_i2c_reset, "", ""), #if defined(CONFIG_CMD_SDRAM) U_BOOT_CMD_MKENT(sdram, 1, 1, do_sdram, "", ""), @@ -1610,7 +1877,7 @@ static char i2c_help_text[] = #endif "crc32 chip address[.0, .1, .2] count - compute CRC32 checksum\n" #if defined(CONFIG_SYS_I2C) || \ - defined(CONFIG_I2C_MULTI_BUS) + defined(CONFIG_I2C_MULTI_BUS) || defined(CONFIG_DM_I2C) "i2c dev [dev] - show or set current I2C bus\n" #endif /* CONFIG_I2C_MULTI_BUS */ #if defined(CONFIG_I2C_EDID) @@ -1622,8 +1889,11 @@ static char i2c_help_text[] = "i2c mw chip address[.0, .1, .2] value [count] - write to I2C device (fill)\n" "i2c nm chip address[.0, .1, .2] - write to I2C device (constant address)\n" "i2c probe [address] - test for and show device(s) on the I2C bus\n" - "i2c read chip address[.0, .1, .2] length memaddress - read to memory \n" + "i2c read chip address[.0, .1, .2] length memaddress - read to memory\n" "i2c write memaddress chip address[.0, .1, .2] length - write memory to i2c\n" +#ifdef CONFIG_DM_I2C + "i2c flags chip [flags] - set or get chip flags\n" +#endif "i2c reset - re-init the I2C Controller\n" #if defined(CONFIG_CMD_SDRAM) "i2c sdram chip - print SDRAM configuration information\n"
In order to test I2C we need some sort of emulation interface. Add hooks to allow a driver to emulate an I2C device for sandbox.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: None Changes in v2: None
drivers/i2c/Makefile | 1 + drivers/i2c/i2c-emul-uclass.c | 14 ++++++++++++++ include/dm/uclass-id.h | 1 + 3 files changed, 16 insertions(+) create mode 100644 drivers/i2c/i2c-emul-uclass.c
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index 063e097..5a93473 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_SYS_I2C_OMAP34XX) += omap24xx_i2c.o obj-$(CONFIG_SYS_I2C_PPC4XX) += ppc4xx_i2c.o obj-$(CONFIG_SYS_I2C_RCAR) += rcar_i2c.o obj-$(CONFIG_SYS_I2C_S3C24X0) += s3c24x0_i2c.o +obj-$(CONFIG_SYS_I2C_SANDBOX) += i2c-emul-uclass.o obj-$(CONFIG_SYS_I2C_SH) += sh_i2c.o obj-$(CONFIG_SYS_I2C_SOFT) += soft_i2c.o obj-$(CONFIG_SYS_I2C_TEGRA) += tegra_i2c.o diff --git a/drivers/i2c/i2c-emul-uclass.c b/drivers/i2c/i2c-emul-uclass.c new file mode 100644 index 0000000..aa89f95 --- /dev/null +++ b/drivers/i2c/i2c-emul-uclass.c @@ -0,0 +1,14 @@ +/* + * Copyright (c) 2014 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <dm.h> +#include <i2c.h> + +UCLASS_DRIVER(i2c_emul) = { + .id = UCLASS_I2C_EMUL, + .name = "i2c_emul", +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 43514bc..e9f6104 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -19,6 +19,7 @@ enum uclass_id { UCLASS_TEST_FDT, UCLASS_TEST_BUS, UCLASS_SPI_EMUL, /* sandbox SPI device emulator */ + UCLASS_I2C_EMUL, /* sandbox I2C device emulator */ UCLASS_SIMPLE_BUS,
/* U-Boot uclasses start here */
This driver includes some test features such as only supporting certain bus speeds. It passes its I2C traffic through to an emulator.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Update for new uclass <=> driver interface
Changes in v2: None
drivers/i2c/Makefile | 2 +- drivers/i2c/sandbox_i2c.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 drivers/i2c/sandbox_i2c.c
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index 5a93473..6f3c86c 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -27,7 +27,7 @@ obj-$(CONFIG_SYS_I2C_OMAP34XX) += omap24xx_i2c.o obj-$(CONFIG_SYS_I2C_PPC4XX) += ppc4xx_i2c.o obj-$(CONFIG_SYS_I2C_RCAR) += rcar_i2c.o obj-$(CONFIG_SYS_I2C_S3C24X0) += s3c24x0_i2c.o -obj-$(CONFIG_SYS_I2C_SANDBOX) += i2c-emul-uclass.o +obj-$(CONFIG_SYS_I2C_SANDBOX) += sandbox_i2c.o i2c-emul-uclass.o obj-$(CONFIG_SYS_I2C_SH) += sh_i2c.o obj-$(CONFIG_SYS_I2C_SOFT) += soft_i2c.o obj-$(CONFIG_SYS_I2C_TEGRA) += tegra_i2c.o diff --git a/drivers/i2c/sandbox_i2c.c b/drivers/i2c/sandbox_i2c.c new file mode 100644 index 0000000..35e3daa --- /dev/null +++ b/drivers/i2c/sandbox_i2c.c @@ -0,0 +1,141 @@ +/* + * Simulate an I2C port + * + * 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 <asm/test.h> +#include <dm/lists.h> +#include <dm/device-internal.h> + +DECLARE_GLOBAL_DATA_PTR; + +struct dm_sandbox_i2c_emul_priv { + struct udevice *emul; +}; + +static int get_emul(struct udevice *bus, uint chip_addr, struct udevice **devp, + struct dm_i2c_ops **opsp) +{ + const void *blob = gd->fdt_blob; + struct dm_i2c_chip *priv; + struct udevice *dev; + int ret; + + *devp = NULL; + *opsp = NULL; + ret = i2c_get_chip(bus, chip_addr, &dev); + if (ret) + return ret; + priv = dev_get_parentdata(dev); + if (!priv->emul) { + int node; + + debug("Scanning i2c bus '%s' for devices\n", dev->name); + for (node = fdt_first_subnode(blob, dev->of_offset); + node >= 0; + node = fdt_next_subnode(blob, node)) { + int ret; + + ret = lists_bind_fdt(dev, blob, node, &priv->emul); + if (ret) + return ret; + debug("Found emul '%s' for i2c device '%s'\n", + priv->emul->name, dev->name); + break; + } + } + + if (!priv->emul) + return -ENODEV; + ret = device_probe(priv->emul); + if (ret) + return ret; + *devp = priv->emul; + *opsp = i2c_get_ops(priv->emul); + + return 0; +} + +static int sandbox_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, + int nmsgs) +{ + struct dm_i2c_bus *i2c = bus->uclass_priv; + struct dm_i2c_ops *ops; + struct udevice *emul, *dev; + struct dm_i2c_chip *chip; + bool is_read; + int ret; + + /* Special test code to return success but with no emulation */ + if (msg->addr == SANDBOX_I2C_TEST_ADDR) + return 0; + + ret = get_emul(bus, msg->addr, &emul, &ops); + if (ret) + return ret; + + /* For testing, require an offset length of 1 */ + ret = i2c_get_chip(bus, msg->addr, &dev); + if (ret) + return ret; + chip = dev_get_parentdata(dev); + if (chip->offset_len != 1) + return -EINVAL; + + /* + * For testing, don't allow writing above 100KHz for writes and + * 400KHz for reads + */ + is_read = nmsgs > 1; + if (i2c->speed_hz > (is_read ? 400000 : 100000)) + return -EINVAL; + return ops->xfer(emul, msg, nmsgs); +} + +static int sandbox_i2c_set_offset_len(struct udevice *dev, uint offset_len) +{ + if (offset_len == 3) + return -EINVAL; + + return 0; +} + +static const struct dm_i2c_ops sandbox_i2c_ops = { + .xfer = sandbox_i2c_xfer, + .set_offset_len = sandbox_i2c_set_offset_len, +}; + +static int sandbox_i2c_child_pre_probe(struct udevice *dev) +{ + struct dm_i2c_chip *i2c_chip = dev_get_parentdata(dev); + + /* Ignore our test address */ + if (i2c_chip->chip_addr == SANDBOX_I2C_TEST_ADDR) + return 0; + if (dev->of_offset == -1) + return 0; + return i2c_chip_ofdata_to_platdata(gd->fdt_blob, dev->of_offset, + i2c_chip); +} + +static const struct udevice_id sandbox_i2c_ids[] = { + { .compatible = "sandbox,i2c" }, + { } +}; + +U_BOOT_DRIVER(i2c_sandbox) = { + .name = "i2c_sandbox", + .id = UCLASS_I2C, + .of_match = sandbox_i2c_ids, + .per_child_auto_alloc_size = sizeof(struct dm_i2c_chip), + .child_pre_probe = sandbox_i2c_child_pre_probe, + .ops = &sandbox_i2c_ops, +};
To enable testing of I2C, add a simple I2C EEPROM simulator for sandbox. It supports reading and writing from a small data store.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Update emulator for new uclass interface
Changes in v2: None
drivers/misc/Makefile | 3 ++ drivers/misc/i2c_eeprom_emul.c | 108 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 drivers/misc/i2c_eeprom_emul.c
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 2f2e48f..ff02184 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -20,6 +20,9 @@ obj-$(CONFIG_MXC_OCOTP) += mxc_ocotp.o obj-$(CONFIG_MXS_OCOTP) += mxs_ocotp.o obj-$(CONFIG_NS87308) += ns87308.o obj-$(CONFIG_PDSP188x) += pdsp188x.o +ifdef CONFIG_DM_I2C +obj-$(CONFIG_SANDBOX) += i2c_eeprom_emul.o +endif obj-$(CONFIG_STATUS_LED) += status_led.o obj-$(CONFIG_TWL4030_LED) += twl4030_led.o obj-$(CONFIG_FSL_IFC) += fsl_ifc.o diff --git a/drivers/misc/i2c_eeprom_emul.c b/drivers/misc/i2c_eeprom_emul.c new file mode 100644 index 0000000..4cdf442 --- /dev/null +++ b/drivers/misc/i2c_eeprom_emul.c @@ -0,0 +1,108 @@ +/* + * Simulate an I2C eeprom + * + * Copyright (c) 2014 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <dm.h> +#include <fdtdec.h> +#include <i2c.h> +#include <malloc.h> + +DECLARE_GLOBAL_DATA_PTR; + +struct sandbox_i2c_flash_plat_data { + const char *filename; + int size; +}; + +struct sandbox_i2c_flash { + uint8_t *data; +}; + +static int sandbox_i2c_eprom_xfer(struct udevice *emul, struct i2c_msg *msg, + int nmsgs) +{ + int offset = 0; + + /* Always let probe succeed */ + if (nmsgs == 1 && msg->len == 1) { + printf("%s: Detected probe\n", __func__); + return 0; + } + + for (; nmsgs > 0; nmsgs--, msg++) { + struct sandbox_i2c_flash_plat_data *plat = + dev_get_platdata(emul); + struct sandbox_i2c_flash *priv = dev_get_priv(emul); + + if (!plat->size) + return -ENODEV; + if (msg->addr + msg->len > plat->size) { + debug("%s: Address %x, len %x is outside range 0..%x\n", + __func__, msg->addr, msg->len, plat->size); + return -EINVAL; + } + if (msg->flags & I2C_M_RD) { + memcpy(msg->buf, priv->data + offset, msg->len); + } else { + offset = *msg->buf; + memcpy(priv->data + offset, msg->buf + 1, + msg->len - 1); + } + } + + return 0; +} + +struct dm_i2c_ops sandbox_i2c_emul_ops = { + .xfer = sandbox_i2c_eprom_xfer, +}; + +static int sandbox_i2c_eeprom_ofdata_to_platdata(struct udevice *dev) +{ + struct sandbox_i2c_flash_plat_data *plat = dev_get_platdata(dev); + + plat->size = fdtdec_get_int(gd->fdt_blob, dev->of_offset, + "sandbox,size", 32); + plat->filename = fdt_getprop(gd->fdt_blob, dev->of_offset, + "sandbox,filename", NULL); + if (!plat->filename) { + debug("%s: No filename for device '%s'\n", __func__, + dev->name); + return -EINVAL; + } + + return 0; +} + +static int sandbox_i2c_eeprom_probe(struct udevice *dev) +{ + struct sandbox_i2c_flash_plat_data *plat = dev_get_platdata(dev); + struct sandbox_i2c_flash *priv = dev_get_priv(dev); + + priv->data = calloc(1, plat->size); + if (!priv->data) + return -ENOMEM; + + return 0; +} + +static const struct udevice_id sandbox_i2c_ids[] = { + { .compatible = "sandbox,i2c-eeprom" }, + { } +}; + +U_BOOT_DRIVER(sandbox_i2c_emul) = { + .name = "sandbox_i2c_eeprom_emul", + .id = UCLASS_I2C_EMUL, + .of_match = sandbox_i2c_ids, + .ofdata_to_platdata = sandbox_i2c_eeprom_ofdata_to_platdata, + .probe = sandbox_i2c_eeprom_probe, + .priv_auto_alloc_size = sizeof(struct sandbox_i2c_flash), + .platdata_auto_alloc_size = sizeof(struct sandbox_i2c_flash_plat_data), + .ops = &sandbox_i2c_emul_ops, +};
Enable the options to bring up I2C on sandbox. Also enable all the available I2C commands for testing purposes.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: None Changes in v2: None
arch/sandbox/include/asm/test.h | 15 +++++++++++++++ include/configs/sandbox.h | 6 ++++++ 2 files changed, 21 insertions(+) create mode 100644 arch/sandbox/include/asm/test.h
diff --git a/arch/sandbox/include/asm/test.h b/arch/sandbox/include/asm/test.h new file mode 100644 index 0000000..d7f7bb5 --- /dev/null +++ b/arch/sandbox/include/asm/test.h @@ -0,0 +1,15 @@ +/* + * Test-related constants for sandbox + * + * Copyright (c) 2014 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef __ASM_TEST_H +#define __ASM_TEST_H + +/* The sandbox driver always permits an I2C device with this address */ +#define SANDBOX_I2C_TEST_ADDR 0x59 + +#endif diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index ee4b244..d4ebe6a 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -110,6 +110,12 @@ #define CONFIG_SPI_FLASH_STMICRO #define CONFIG_SPI_FLASH_WINBOND
+#define CONFIG_DM_I2C +#define CONFIG_CMD_I2C +#define CONFIG_SYS_I2C_SANDBOX +#define CONFIG_I2C_EDID +#define CONFIG_I2C_EEPROM + /* Memory things - we don't really want a memory test */ #define CONFIG_SYS_LOAD_ADDR 0x00000000 #define CONFIG_SYS_MEMTEST_START 0x00100000
Add an I2C bus to the device tree, with an EEPROM emulator attached to one of the addresses.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: None Changes in v2: None
arch/sandbox/dts/sandbox.dts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts index 7614715..11748ae 100644 --- a/arch/sandbox/dts/sandbox.dts +++ b/arch/sandbox/dts/sandbox.dts @@ -134,6 +134,23 @@ num-gpios = <20>; };
+ i2c@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + compatible = "sandbox,i2c"; + clock-frequency = <400000>; + eeprom@2c { + reg = <0x2c>; + compatible = "i2c-eeprom"; + emul { + compatible = "sandbox,i2c-eeprom"; + sandbox,filename = "i2c.bin"; + sandbox,size = <128>; + }; + }; + }; + spi@0 { #address-cells = <1>; #size-cells = <0>;
There seem to be a few EEPROM drivers around - perhaps we should have a single standard one? This simple driver is used for sandbox testing, but could be pressed into more active service.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: None Changes in v2: - Update commit message for EEPROM driver
drivers/misc/Makefile | 1 + drivers/misc/i2c_eeprom.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/i2c_eeprom.h | 19 ++++++++++++++++++ 4 files changed, 72 insertions(+) create mode 100644 drivers/misc/i2c_eeprom.c create mode 100644 include/i2c_eeprom.h
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index ff02184..6fa836f 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_CROS_EC_SANDBOX) += cros_ec_sandbox.o obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o obj-$(CONFIG_FSL_IIM) += fsl_iim.o obj-$(CONFIG_GPIO_LED) += gpio_led.o +obj-$(CONFIG_I2C_EEPROM) += i2c_eeprom.o obj-$(CONFIG_FSL_MC9SDZ60) += mc9sdz60.o obj-$(CONFIG_MXC_OCOTP) += mxc_ocotp.o obj-$(CONFIG_MXS_OCOTP) += mxs_ocotp.o diff --git a/drivers/misc/i2c_eeprom.c b/drivers/misc/i2c_eeprom.c new file mode 100644 index 0000000..d0548ec --- /dev/null +++ b/drivers/misc/i2c_eeprom.c @@ -0,0 +1,51 @@ +/* + * Copyright (c) 2014 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <dm.h> +#include <i2c.h> +#include <i2c_eeprom.h> + +static int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf, + int size) +{ + return -ENODEV; +} + +static int i2c_eeprom_write(struct udevice *dev, int offset, + const uint8_t *buf, int size) +{ + return -ENODEV; +} + +struct i2c_eeprom_ops i2c_eeprom_std_ops = { + .read = i2c_eeprom_read, + .write = i2c_eeprom_write, +}; + +int i2c_eeprom_std_probe(struct udevice *dev) +{ + return 0; +} + +static const struct udevice_id i2c_eeprom_std_ids[] = { + { .compatible = "i2c-eeprom" }, + { } +}; + +U_BOOT_DRIVER(i2c_eeprom_std) = { + .name = "i2c_eeprom", + .id = UCLASS_I2C_EEPROM, + .of_match = i2c_eeprom_std_ids, + .probe = i2c_eeprom_std_probe, + .priv_auto_alloc_size = sizeof(struct i2c_eeprom), + .ops = &i2c_eeprom_std_ops, +}; + +UCLASS_DRIVER(i2c_eeprom) = { + .id = UCLASS_I2C_EEPROM, + .name = "i2c_eeprom", +}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index e9f6104..7f7dcf7 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -31,6 +31,7 @@ enum uclass_id { UCLASS_CROS_EC, /* Chrome OS EC */ UCLASS_I2C, /* I2C bus */ UCLASS_I2C_GENERIC, /* Generic I2C device */ + UCLASS_I2C_EEPROM, /* I2C EEPROM device */
UCLASS_COUNT, UCLASS_INVALID = -1, diff --git a/include/i2c_eeprom.h b/include/i2c_eeprom.h new file mode 100644 index 0000000..ea6c962 --- /dev/null +++ b/include/i2c_eeprom.h @@ -0,0 +1,19 @@ +/* + * Copyright (c) 2014 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef __I2C_EEPROM +#define __I2C_EEPROM + +struct i2c_eeprom_ops { + int (*read)(struct udevice *dev, int offset, uint8_t *buf, int size); + int (*write)(struct udevice *dev, int offset, const uint8_t *buf, + int size); +}; + +struct i2c_eeprom { +}; + +#endif
Hi Simon,
On Mon, 24 Nov 2014 11:57:22 -0700 Simon Glass sjg@chromium.org wrote:
--- /dev/null +++ b/drivers/misc/i2c_eeprom.c @@ -0,0 +1,51 @@ +/*
- Copyright (c) 2014 Google, Inc
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <common.h> +#include <dm.h> +#include <i2c.h> +#include <i2c_eeprom.h>
+static int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf,
int size)+{
- return -ENODEV;
+}
+static int i2c_eeprom_write(struct udevice *dev, int offset,
const uint8_t *buf, int size)+{
- return -ENODEV;
+}
Isn't there any possibility to reach i2c_eeprom_read/i2c_eeprom_write handler?
Maybe, is it better to fallback to i2c_read()/i2c_write() like this?
static int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf, int size) { return i2c_read(dev, offset, buf, size); }
static int i2c_eeprom_write(struct udevice *dev, int offset, const uint8_t *buf, int size) { return i2c_write(dev, offset, buf, size); }
Moreover, can we read data from EEPROM without knowing if its parent is I2C bus or SPI bus ?
My rough image is like this:
int eeprom_read(struct udevice *dev, int offset, uint8_t buf, int size) { struct udevice *bus = dev->parent; struct generic_bus_operation *ops = bus->uclass->uc_drv->ops;
return ops->read(dev, bus, offset, buf, size); }
I am not sure, but if this approach is possible, we do not need to have both of "i2c_eeprom uclass" and "spi_eeprom uclass".
struct generic_bus_operation is the operaton some uclasses have.
We can move i2c_read() and i2c_write() to struct generic_bus_operation of i2c uclass.
Likewise, we can move spi_read() and spi_write() to struct generic_bus_operation of spi uclass.
Best Regards Masahiro Yamada
Hi Masahiro,
On 1 December 2014 at 04:48, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
Hi Simon,
On Mon, 24 Nov 2014 11:57:22 -0700 Simon Glass sjg@chromium.org wrote:
--- /dev/null +++ b/drivers/misc/i2c_eeprom.c @@ -0,0 +1,51 @@ +/*
- Copyright (c) 2014 Google, Inc
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <common.h> +#include <dm.h> +#include <i2c.h> +#include <i2c_eeprom.h>
+static int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf,
int size)+{
return -ENODEV;+}
+static int i2c_eeprom_write(struct udevice *dev, int offset,
const uint8_t *buf, int size)+{
return -ENODEV;+}
Isn't there any possibility to reach i2c_eeprom_read/i2c_eeprom_write handler?
No it's just a stub for now.
Maybe, is it better to fallback to i2c_read()/i2c_write() like this?
static int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf, int size) { return i2c_read(dev, offset, buf, size); }
static int i2c_eeprom_write(struct udevice *dev, int offset, const uint8_t *buf, int size) { return i2c_write(dev, offset, buf, size); }
Sure we can - I haven't implemented this yet, but if we could have a generic EEPROM driver like this it would be nice.
Moreover, can we read data from EEPROM without knowing if its parent is I2C bus or SPI bus ?
My rough image is like this:
int eeprom_read(struct udevice *dev, int offset, uint8_t buf, int size) { struct udevice *bus = dev->parent; struct generic_bus_operation *ops = bus->uclass->uc_drv->ops;
return ops->read(dev, bus, offset, buf, size);}
I am not sure, but if this approach is possible, we do not need to have both of "i2c_eeprom uclass" and "spi_eeprom uclass".
struct generic_bus_operation is the operaton some uclasses have.
We can move i2c_read() and i2c_write() to struct generic_bus_operation of i2c uclass.
Likewise, we can move spi_read() and spi_write() to struct generic_bus_operation of spi uclass.
Yes we should do join up I2C and SPI. In order to do it we need SPI uclass to support reading and writing a particular offset (rather than just the current generic xfer()).
Perhaps we should have a generic register access uclass. Drivers that want to support it could add themselves to the I2C and REGISTER uclass. But it would be better to handle this entirely in the I2C/SPI uclasses if we can.
I added Przemyslaw as he needs something like this for the PMIC uclass.
Regards, Simon
Hi Simon,
2014-12-04 0:18 GMT+09:00 Simon Glass sjg@chromium.org:
Moreover, can we read data from EEPROM without knowing if its parent is I2C bus or SPI bus ?
My rough image is like this:
int eeprom_read(struct udevice *dev, int offset, uint8_t buf, int size) { struct udevice *bus = dev->parent; struct generic_bus_operation *ops = bus->uclass->uc_drv->ops;
return ops->read(dev, bus, offset, buf, size);}
I am not sure, but if this approach is possible, we do not need to have both of "i2c_eeprom uclass" and "spi_eeprom uclass".
struct generic_bus_operation is the operaton some uclasses have.
We can move i2c_read() and i2c_write() to struct generic_bus_operation of i2c uclass.
Likewise, we can move spi_read() and spi_write() to struct generic_bus_operation of spi uclass.
Yes we should do join up I2C and SPI. In order to do it we need SPI uclass to support reading and writing a particular offset (rather than just the current generic xfer()).
Perhaps we should have a generic register access uclass. Drivers that want to support it could add themselves to the I2C and REGISTER uclass. But it would be better to handle this entirely in the I2C/SPI uclasses if we can.
I added Przemyslaw as he needs something like this for the PMIC uclass.
Good. Or you can do this after SPI uclass things are prepared. Please feel free to postpone it if it is too much work to be included in this series.
Except some minor mistakes, this series looks good!
Hello, On 12/03/2014 04:18 PM, Simon Glass wrote:
Hi Masahiro,
On 1 December 2014 at 04:48, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
Hi Simon,
On Mon, 24 Nov 2014 11:57:22 -0700 Simon Glass sjg@chromium.org wrote:
--- /dev/null +++ b/drivers/misc/i2c_eeprom.c @@ -0,0 +1,51 @@ +/*
- Copyright (c) 2014 Google, Inc
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <common.h> +#include <dm.h> +#include <i2c.h> +#include <i2c_eeprom.h>
+static int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf,
int size)+{
return -ENODEV;+}
+static int i2c_eeprom_write(struct udevice *dev, int offset,
const uint8_t *buf, int size)+{
return -ENODEV;+}
Isn't there any possibility to reach i2c_eeprom_read/i2c_eeprom_write handler?
No it's just a stub for now.
Maybe, is it better to fallback to i2c_read()/i2c_write() like this?
static int i2c_eeprom_read(struct udevice *dev, int offset, uint8_t *buf, int size) { return i2c_read(dev, offset, buf, size); }
static int i2c_eeprom_write(struct udevice *dev, int offset, const uint8_t *buf, int size) { return i2c_write(dev, offset, buf, size); }
Sure we can - I haven't implemented this yet, but if we could have a generic EEPROM driver like this it would be nice.
Moreover, can we read data from EEPROM without knowing if its parent is I2C bus or SPI bus ?
My rough image is like this:
int eeprom_read(struct udevice *dev, int offset, uint8_t buf, int size) { struct udevice *bus = dev->parent; struct generic_bus_operation *ops = bus->uclass->uc_drv->ops;
return ops->read(dev, bus, offset, buf, size);}
I am not sure, but if this approach is possible, we do not need to have both of "i2c_eeprom uclass" and "spi_eeprom uclass".
struct generic_bus_operation is the operaton some uclasses have.
We can move i2c_read() and i2c_write() to struct generic_bus_operation of i2c uclass.
Likewise, we can move spi_read() and spi_write() to struct generic_bus_operation of spi uclass.
Yes we should do join up I2C and SPI. In order to do it we need SPI uclass to support reading and writing a particular offset (rather than just the current generic xfer()).
Perhaps we should have a generic register access uclass. Drivers that want to support it could add themselves to the I2C and REGISTER uclass. But it would be better to handle this entirely in the I2C/SPI uclasses if we can.
I added Przemyslaw as he needs something like this for the PMIC uclass.
Regards, Simon
I send some general comments as a reply for patch number 0 and it also touches the bus accessing problem.
Best regards,
Add some basic tests to check that the system works as expected.
c Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Update for new uclass <=> emulateor interface
Changes in v2: - Add a test for automatic binding of generic I2C devices - Add a new asm/test.h header for tests in sandbox
drivers/i2c/sandbox_i2c.c | 1 + test/dm/Makefile | 1 + test/dm/i2c.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++ test/dm/test.dts | 17 +++++++ 4 files changed, 131 insertions(+) create mode 100644 test/dm/i2c.c
diff --git a/drivers/i2c/sandbox_i2c.c b/drivers/i2c/sandbox_i2c.c index 35e3daa..acee384 100644 --- a/drivers/i2c/sandbox_i2c.c +++ b/drivers/i2c/sandbox_i2c.c @@ -122,6 +122,7 @@ static int sandbox_i2c_child_pre_probe(struct udevice *dev) return 0; if (dev->of_offset == -1) return 0; + return i2c_chip_ofdata_to_platdata(gd->fdt_blob, dev->of_offset, i2c_chip); } diff --git a/test/dm/Makefile b/test/dm/Makefile index 75d3d41..612aa95 100644 --- a/test/dm/Makefile +++ b/test/dm/Makefile @@ -20,4 +20,5 @@ ifneq ($(CONFIG_SANDBOX),) obj-$(CONFIG_DM_GPIO) += gpio.o obj-$(CONFIG_DM_SPI) += spi.o obj-$(CONFIG_DM_SPI_FLASH) += sf.o +obj-$(CONFIG_DM_I2C) += i2c.o endif diff --git a/test/dm/i2c.c b/test/dm/i2c.c new file mode 100644 index 0000000..94f7934 --- /dev/null +++ b/test/dm/i2c.c @@ -0,0 +1,112 @@ +/* + * Copyright (C) 2013 Google, Inc + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <dm.h> +#include <fdtdec.h> +#include <i2c.h> +#include <dm/device-internal.h> +#include <dm/test.h> +#include <dm/uclass-internal.h> +#include <dm/ut.h> +#include <dm/util.h> +#include <asm/state.h> +#include <asm/test.h> + +static const int busnum; +static const int chip = 0x2c; + +/* Test that we can find buses and chips */ +static int dm_test_i2c_find(struct dm_test_state *dms) +{ + struct udevice *bus, *dev; + const int no_chip = 0x10; + + ut_asserteq(-ENODEV, uclass_find_device_by_seq(UCLASS_I2C, busnum, + false, &bus)); + + /* + * i2c_post_bind() will bind devices to chip selects. Check this then + * remove the emulation and the slave device. + */ + ut_assertok(uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus)); + ut_assertok(i2c_probe(bus, chip, &dev)); + ut_asserteq(-ENODEV, i2c_probe(bus, no_chip, &dev)); + ut_asserteq(-ENODEV, uclass_get_device_by_seq(UCLASS_I2C, 1, &bus)); + + return 0; +} +DM_TEST(dm_test_i2c_find, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); + +static int dm_test_i2c_read_write(struct dm_test_state *dms) +{ + struct udevice *bus, *dev; + uint8_t buf[5]; + + ut_assertok(uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus)); + ut_assertok(i2c_get_chip(bus, chip, &dev)); + ut_assertok(i2c_read(dev, 0, buf, 5)); + ut_assertok(memcmp(buf, "\0\0\0\0\0", sizeof(buf))); + ut_assertok(i2c_write(dev, 2, (uint8_t *)"AB", 2)); + ut_assertok(i2c_read(dev, 0, buf, 5)); + ut_assertok(memcmp(buf, "\0\0AB\0", sizeof(buf))); + + return 0; +} +DM_TEST(dm_test_i2c_read_write, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); + +static int dm_test_i2c_speed(struct dm_test_state *dms) +{ + struct udevice *bus, *dev; + uint8_t buf[5]; + + ut_assertok(uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus)); + ut_assertok(i2c_get_chip(bus, chip, &dev)); + ut_assertok(i2c_set_bus_speed(bus, 100000)); + ut_assertok(i2c_read(dev, 0, buf, 5)); + ut_assertok(i2c_set_bus_speed(bus, 400000)); + ut_asserteq(400000, i2c_get_bus_speed(bus)); + ut_assertok(i2c_read(dev, 0, buf, 5)); + ut_asserteq(-EINVAL, i2c_write(dev, 0, buf, 5)); + + return 0; +} +DM_TEST(dm_test_i2c_speed, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); + +static int dm_test_i2c_offset_len(struct dm_test_state *dms) +{ + struct udevice *bus, *dev; + uint8_t buf[5]; + + ut_assertok(uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus)); + ut_assertok(i2c_get_chip(bus, chip, &dev)); + ut_assertok(i2c_set_chip_offset_len(dev, 1)); + ut_assertok(i2c_read(dev, 0, buf, 5)); + + /* The sandbox driver allows this setting, but then fails reads */ + ut_assertok(i2c_set_chip_offset_len(dev, 2)); + ut_asserteq(-EINVAL, i2c_read(dev, 0, buf, 5)); + + /* This is not supported by the uclass */ + ut_asserteq(-EINVAL, i2c_set_chip_offset_len(dev, 4)); + + /* This is faulted by the sandbox driver */ + ut_asserteq(-EINVAL, i2c_set_chip_offset_len(dev, 3)); + + return 0; +} +DM_TEST(dm_test_i2c_offset_len, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); + +static int dm_test_i2c_probe_empty(struct dm_test_state *dms) +{ + struct udevice *bus, *dev; + + ut_assertok(uclass_get_device_by_seq(UCLASS_I2C, busnum, &bus)); + ut_assertok(i2c_probe(bus, SANDBOX_I2C_TEST_ADDR, &dev)); + + return 0; +} +DM_TEST(dm_test_i2c_probe_empty, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT); diff --git a/test/dm/test.dts b/test/dm/test.dts index 1fba792..69991a3 100644 --- a/test/dm/test.dts +++ b/test/dm/test.dts @@ -93,6 +93,23 @@ num-gpios = <10>; };
+ i2c@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + compatible = "sandbox,i2c"; + clock-frequency = <100000>; + eeprom@2c { + reg = <0x2c>; + compatible = "i2c-eeprom"; + emul { + compatible = "sandbox,i2c-eeprom"; + sandbox,filename = "i2c.bin"; + sandbox,size = <128>; + }; + }; + }; + spi@0 { #address-cells = <1>; #size-cells = <0>;
This converts all Tegra boards over to use driver model for I2C. The driver is adjusted to use driver model and the following obsolete CONFIGs are removed:
- CONFIG_SYS_I2C_INIT_BOARD - CONFIG_I2C_MULTI_BUS - CONFIG_SYS_MAX_I2C_BUS - CONFIG_SYS_I2C_SPEED - CONFIG_SYS_I2C
This has been tested on: - trimslice (no I2C) - beaver - Jetson-TK1
It has not been tested on Tegra 114 as I don't have that board.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Change driver to support new message-based I2C uclass API
Changes in v2: - Adjust tegra_i2c_child_pre_probe() to permit generic I2C devices - Correct the compatible strings for I2C buses - Don't init if the speed is 0, since this breaks the controller - Expand coverage to all Tegra boards
arch/arm/cpu/tegra20-common/pmu.c | 21 +- arch/arm/dts/tegra124-jetson-tk1.dts | 1 - arch/arm/dts/tegra124-norrin.dts | 1 - arch/arm/dts/tegra30-tec-ng.dts | 4 + arch/arm/include/asm/arch-tegra/tegra_i2c.h | 2 +- board/avionic-design/common/tamonten-ng.c | 12 +- board/nvidia/cardhu/cardhu.c | 13 +- board/nvidia/common/board.c | 4 - board/nvidia/dalmore/dalmore.c | 21 +- board/nvidia/whistler/whistler.c | 29 ++- board/toradex/apalis_t30/apalis_t30.c | 19 +- drivers/i2c/tegra_i2c.c | 363 +++++++++------------------- drivers/power/tps6586x.c | 27 +-- include/configs/apalis_t30.h | 3 - include/configs/beaver.h | 3 - include/configs/cardhu.h | 5 - include/configs/colibri_t30.h | 3 - include/configs/dalmore.h | 5 - include/configs/jetson-tk1.h | 5 - include/configs/norrin.h | 5 - include/configs/seaboard.h | 3 - include/configs/tec-ng.h | 5 - include/configs/tegra-common.h | 1 + include/configs/tegra114-common.h | 3 - include/configs/tegra124-common.h | 3 - include/configs/tegra20-common.h | 3 - include/configs/tegra30-common.h | 3 - include/configs/trimslice.h | 3 - include/configs/venice2.h | 5 - include/configs/whistler.h | 3 - include/tps6586x.h | 4 +- 31 files changed, 205 insertions(+), 377 deletions(-)
diff --git a/arch/arm/cpu/tegra20-common/pmu.c b/arch/arm/cpu/tegra20-common/pmu.c index c595f70..36a76a2 100644 --- a/arch/arm/cpu/tegra20-common/pmu.c +++ b/arch/arm/cpu/tegra20-common/pmu.c @@ -6,6 +6,7 @@ */
#include <common.h> +#include <i2c.h> #include <tps6586x.h> #include <asm/io.h> #include <asm/arch/tegra.h> @@ -23,9 +24,13 @@ #define VDD_TRANSITION_STEP 0x06 /* 150mv */ #define VDD_TRANSITION_RATE 0x06 /* 3.52mv/us */
+#define PMI_I2C_ADDRESS 0x34 /* chip requires this address */ + int pmu_set_nominal(void) { - int core, cpu, bus; + struct udevice *bus, *dev; + int core, cpu; + int ret;
/* by default, the table has been filled with T25 settings */ switch (tegra_get_chip_sku()) { @@ -42,12 +47,18 @@ int pmu_set_nominal(void) return -1; }
- bus = tegra_i2c_get_dvc_bus_num(); - if (bus == -1) { + ret = tegra_i2c_get_dvc_bus(&bus); + if (ret) { debug("%s: Cannot find DVC I2C bus\n", __func__); - return -1; + return ret; } - tps6586x_init(bus); + ret = i2c_get_chip(bus, PMI_I2C_ADDRESS, &dev); + if (ret) { + debug("%s: Cannot find DVC I2C chip\n", __func__); + return ret; + } + + tps6586x_init(dev); tps6586x_set_pwm_mode(TPS6586X_PWM_SM1); return tps6586x_adjust_sm0_sm1(core, cpu, VDD_TRANSITION_STEP, VDD_TRANSITION_RATE, VDD_RELATION); diff --git a/arch/arm/dts/tegra124-jetson-tk1.dts b/arch/arm/dts/tegra124-jetson-tk1.dts index ffad116..f6fe9a0 100644 --- a/arch/arm/dts/tegra124-jetson-tk1.dts +++ b/arch/arm/dts/tegra124-jetson-tk1.dts @@ -16,7 +16,6 @@ i2c2 = "/i2c@7000c400"; i2c3 = "/i2c@7000c500"; i2c4 = "/i2c@7000c700"; - i2c5 = "/i2c@7000d100"; sdhci0 = "/sdhci@700b0600"; sdhci1 = "/sdhci@700b0400"; spi0 = "/spi@7000d400"; diff --git a/arch/arm/dts/tegra124-norrin.dts b/arch/arm/dts/tegra124-norrin.dts index b07630c..2dbeab8 100644 --- a/arch/arm/dts/tegra124-norrin.dts +++ b/arch/arm/dts/tegra124-norrin.dts @@ -13,7 +13,6 @@ i2c2 = "/i2c@7000c400"; i2c3 = "/i2c@7000c500"; i2c4 = "/i2c@7000c700"; - i2c5 = "/i2c@7000d100"; sdhci0 = "/sdhci@700b0600"; sdhci1 = "/sdhci@700b0400"; spi0 = "/spi@7000d400"; diff --git a/arch/arm/dts/tegra30-tec-ng.dts b/arch/arm/dts/tegra30-tec-ng.dts index 8a69e81..e924acc 100644 --- a/arch/arm/dts/tegra30-tec-ng.dts +++ b/arch/arm/dts/tegra30-tec-ng.dts @@ -6,6 +6,10 @@ model = "Avionic Design Tamonten™ NG Evaluation Carrier"; compatible = "ad,tec-ng", "nvidia,tegra30";
+ aliases { + i2c0 = "/i2c@7000c400"; + }; + /* GEN2 */ i2c@7000c400 { status = "okay"; diff --git a/arch/arm/include/asm/arch-tegra/tegra_i2c.h b/arch/arm/include/asm/arch-tegra/tegra_i2c.h index 7ca6907..eeeb247 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_i2c.h +++ b/arch/arm/include/asm/arch-tegra/tegra_i2c.h @@ -167,6 +167,6 @@ struct i2c_ctlr { * * @return number of bus, or -1 if there is no DVC active */ -int tegra_i2c_get_dvc_bus_num(void); +int tegra_i2c_get_dvc_bus(struct udevice **busp);
#endif /* _TEGRA_I2C_H_ */ diff --git a/board/avionic-design/common/tamonten-ng.c b/board/avionic-design/common/tamonten-ng.c index 5870b95..86a0844 100644 --- a/board/avionic-design/common/tamonten-ng.c +++ b/board/avionic-design/common/tamonten-ng.c @@ -6,6 +6,7 @@ */
#include <common.h> +#include <dm.h> #include <asm/arch/pinmux.h> #include <asm/arch/gp_padctrl.h> #include <asm/arch/gpio.h> @@ -51,8 +52,15 @@ void gpio_early_init(void)
void pmu_write(uchar reg, uchar data) { - i2c_set_bus_num(4); /* PMU is on bus 4 */ - i2c_write(PMU_I2C_ADDRESS, reg, 1, &data, 1); + struct udevice *dev; + int ret; + + ret = i2c_get_chip_for_busnum(4, PMU_I2C_ADDRESS, &dev); + if (ret) { + debug("%s: Cannot find PMIC I2C chip\n", __func__); + return; + } + i2c_write(dev, reg, &data, 1); }
/* diff --git a/board/nvidia/cardhu/cardhu.c b/board/nvidia/cardhu/cardhu.c index cc0e5e1..026f45c 100644 --- a/board/nvidia/cardhu/cardhu.c +++ b/board/nvidia/cardhu/cardhu.c @@ -6,6 +6,7 @@ */
#include <common.h> +#include <dm.h> #include <asm/arch/pinmux.h> #include <asm/arch/gp_padctrl.h> #include "pinmux-config-cardhu.h" @@ -37,17 +38,23 @@ void pinmux_init(void) */ void board_sdmmc_voltage_init(void) { + struct udevice *dev; uchar reg, data_buffer[1]; + int ret; int i;
- i2c_set_bus_num(0); /* PMU is on bus 0 */ + ret = i2c_get_chip_for_busnum(0, PMU_I2C_ADDRESS, &dev); + if (ret) { + debug("%s: Cannot find PMIC I2C chip\n", __func__); + return; + }
/* TPS659110: LDO5_REG = 3.3v, ACTIVE to SDMMC1 */ data_buffer[0] = 0x65; reg = 0x32;
for (i = 0; i < MAX_I2C_RETRY; ++i) { - if (i2c_write(PMU_I2C_ADDRESS, reg, 1, data_buffer, 1)) + if (i2c_write(dev, reg, data_buffer, 1)) udelay(100); }
@@ -56,7 +63,7 @@ void board_sdmmc_voltage_init(void) reg = 0x67;
for (i = 0; i < MAX_I2C_RETRY; ++i) { - if (i2c_write(PMU_I2C_ADDRESS, reg, 1, data_buffer, 1)) + if (i2c_write(dev, reg, data_buffer, 1)) udelay(100); } } diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c index 0e4a65a..4bdbf01 100644 --- a/board/nvidia/common/board.c +++ b/board/nvidia/common/board.c @@ -113,10 +113,6 @@ int board_init(void) power_det_init();
#ifdef CONFIG_SYS_I2C_TEGRA -#ifndef CONFIG_SYS_I2C_INIT_BOARD -#error "You must define CONFIG_SYS_I2C_INIT_BOARD to use i2c on Nvidia boards" -#endif - i2c_init_board(); # ifdef CONFIG_TEGRA_PMU if (pmu_set_nominal()) debug("Failed to select nominal voltages\n"); diff --git a/board/nvidia/dalmore/dalmore.c b/board/nvidia/dalmore/dalmore.c index f2d05af..2a73746 100644 --- a/board/nvidia/dalmore/dalmore.c +++ b/board/nvidia/dalmore/dalmore.c @@ -15,6 +15,7 @@ */
#include <common.h> +#include <dm.h> #include <asm/arch/pinmux.h> #include <asm/arch/gp_padctrl.h> #include "pinmux-config-dalmore.h" @@ -50,18 +51,21 @@ void pinmux_init(void) */ void board_sdmmc_voltage_init(void) { + struct udevice *dev; uchar reg, data_buffer[1]; int ret;
- ret = i2c_set_bus_num(0);/* PMU is on bus 0 */ - if (ret) - printf("%s: i2c_set_bus_num returned %d\n", __func__, ret); + ret = i2c_get_chip_for_busnum(0, PMU_I2C_ADDRESS, &dev); + if (ret) { + debug("%s: Cannot find PMIC I2C chip\n", __func__); + return; + }
/* TPS65913: LDO9_VOLTAGE = 3.3V */ data_buffer[0] = 0x31; reg = 0x61;
- ret = i2c_write(PMU_I2C_ADDRESS, reg, 1, data_buffer, 1); + ret = i2c_write(dev, reg, data_buffer, 1); if (ret) printf("%s: PMU i2c_write %02X<-%02X returned %d\n", __func__, reg, data_buffer[0], ret); @@ -70,7 +74,7 @@ void board_sdmmc_voltage_init(void) data_buffer[0] = 0x01; reg = 0x60;
- ret = i2c_write(PMU_I2C_ADDRESS, reg, 1, data_buffer, 1); + ret = i2c_write(dev, reg, data_buffer, 1); if (ret) printf("%s: PMU i2c_write %02X<-%02X returned %d\n", __func__, reg, data_buffer[0], ret); @@ -79,7 +83,12 @@ void board_sdmmc_voltage_init(void) data_buffer[0] = 0x03; reg = 0x14;
- ret = i2c_write(BAT_I2C_ADDRESS, reg, 1, data_buffer, 1); + ret = i2c_get_chip_for_busnum(0, BAT_I2C_ADDRESS, &dev); + if (ret) { + debug("%s: Cannot find charger I2C chip\n", __func__); + return; + } + ret = i2c_write(dev, reg, data_buffer, 1); if (ret) printf("%s: BAT i2c_write %02X<-%02X returned %d\n", __func__, reg, data_buffer[0], ret); diff --git a/board/nvidia/whistler/whistler.c b/board/nvidia/whistler/whistler.c index 3e9d3d9..3114b20 100644 --- a/board/nvidia/whistler/whistler.c +++ b/board/nvidia/whistler/whistler.c @@ -6,6 +6,7 @@ */
#include <common.h> +#include <dm.h> #include <asm/io.h> #include <asm/arch/tegra.h> #include <asm/arch/clock.h> @@ -21,23 +22,26 @@ */ void pin_mux_mmc(void) { + struct udevice *dev; uchar val; int ret;
/* Turn on MAX8907B LDO12 to 2.8V for J40 power */ - ret = i2c_set_bus_num(0); - if (ret) - printf("i2c_set_bus_num failed: %d\n", ret); + ret = i2c_get_chip_for_busnum(0, 0x3c, &dev); + if (ret) { + printf("%s: Cannot find MAX8907B I2C chip\n", __func__); + return; + } val = 0x29; - ret = i2c_write(0x3c, 0x46, 1, &val, 1); + ret = i2c_write(dev, 0x46, &val, 1); if (ret) printf("i2c_write 0 0x3c 0x46 failed: %d\n", ret); val = 0x00; - ret = i2c_write(0x3c, 0x45, 1, &val, 1); + ret = i2c_write(dev, 0x45, &val, 1); if (ret) printf("i2c_write 0 0x3c 0x45 failed: %d\n", ret); val = 0x1f; - ret = i2c_write(0x3c, 0x44, 1, &val, 1); + ret = i2c_write(dev, 0x44, &val, 1); if (ret) printf("i2c_write 0 0x3c 0x44 failed: %d\n", ret);
@@ -49,6 +53,7 @@ void pin_mux_mmc(void) /* this is a weak define that we are overriding */ void pin_mux_usb(void) { + struct udevice *dev; uchar val; int ret;
@@ -59,15 +64,17 @@ void pin_mux_usb(void) */
/* Turn on TAC6416's GPIO 0+1 for USB1/3's VBUS */ - ret = i2c_set_bus_num(0); - if (ret) - printf("i2c_set_bus_num failed: %d\n", ret); + ret = i2c_get_chip_for_busnum(0, 0x20, &dev); + if (ret) { + printf("%s: Cannot find TAC6416 I2C chip\n", __func__); + return; + } val = 0x03; - ret = i2c_write(0x20, 2, 1, &val, 1); + ret = i2c_write(dev, 2, &val, 1); if (ret) printf("i2c_write 0 0x20 2 failed: %d\n", ret); val = 0xfc; - ret = i2c_write(0x20, 6, 1, &val, 1); + ret = i2c_write(dev, 6, &val, 1); if (ret) printf("i2c_write 0 0x20 6 failed: %d\n", ret); } diff --git a/board/toradex/apalis_t30/apalis_t30.c b/board/toradex/apalis_t30/apalis_t30.c index b9d694a..5d2c024 100644 --- a/board/toradex/apalis_t30/apalis_t30.c +++ b/board/toradex/apalis_t30/apalis_t30.c @@ -6,7 +6,7 @@ */
#include <common.h> - +#include <dm.h> #include <asm/arch/gp_padctrl.h> #include <asm/arch/pinmux.h> #include <asm/gpio.h> @@ -38,23 +38,20 @@ void pinmux_init(void) #ifdef CONFIG_PCI_TEGRA int tegra_pcie_board_init(void) { - unsigned int old_bus; + struct udevice *dev; u8 addr, data[1]; int err;
- old_bus = i2c_get_bus_num(); - - err = i2c_set_bus_num(0); + err = i2c_get_chip_for_busnum(0, PMU_I2C_ADDRESS, &dev); if (err) { - debug("failed to set I2C bus\n"); + debug("%s: Cannot find PMIC I2C chip\n", __func__); return err; } - /* TPS659110: VDD2_OP_REG = 1.05V */ data[0] = 0x27; addr = 0x25;
- err = i2c_write(PMU_I2C_ADDRESS, addr, 1, data, 1); + err = i2c_write(dev, addr, data, 1); if (err) { debug("failed to set VDD supply\n"); return err; @@ -64,7 +61,7 @@ int tegra_pcie_board_init(void) data[0] = 0x0D; addr = 0x24;
- err = i2c_write(PMU_I2C_ADDRESS, addr, 1, data, 1); + err = i2c_write(dev, addr, data, 1); if (err) { debug("failed to enable VDD supply\n"); return err; @@ -74,14 +71,12 @@ int tegra_pcie_board_init(void) data[0] = 0x0D; addr = 0x35;
- err = i2c_write(PMU_I2C_ADDRESS, addr, 1, data, 1); + err = i2c_write(dev, addr, data, 1); if (err) { debug("failed to set AVDD supply\n"); return err; }
- i2c_set_bus_num(old_bus); - return 0; }
diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c index 562211e..0c7fe84 100644 --- a/drivers/i2c/tegra_i2c.c +++ b/drivers/i2c/tegra_i2c.c @@ -7,6 +7,8 @@ */
#include <common.h> +#include <dm.h> +#include <errno.h> #include <fdtdec.h> #include <i2c.h> #include <asm/io.h> @@ -19,6 +21,12 @@
DECLARE_GLOBAL_DATA_PTR;
+enum i2c_type { + TYPE_114, + TYPE_STD, + TYPE_DVC, +}; + /* Information about i2c controller */ struct i2c_bus { int id; @@ -27,20 +35,17 @@ struct i2c_bus { int pinmux_config; struct i2c_control *control; struct i2c_ctlr *regs; - int is_dvc; /* DVC type, rather than I2C */ - int is_scs; /* single clock source (T114+) */ + enum i2c_type type; int inited; /* bus is inited */ };
-static struct i2c_bus i2c_controllers[TEGRA_I2C_NUM_CONTROLLERS]; - static void set_packet_mode(struct i2c_bus *i2c_bus) { u32 config;
config = I2C_CNFG_NEW_MASTER_FSM_MASK | I2C_CNFG_PACKET_MODE_MASK;
- if (i2c_bus->is_dvc) { + if (i2c_bus->type == TYPE_DVC) { struct dvc_ctlr *dvc = (struct dvc_ctlr *)i2c_bus->regs;
writel(config, &dvc->cnfg); @@ -65,6 +70,9 @@ static void i2c_reset_controller(struct i2c_bus *i2c_bus)
static void i2c_init_controller(struct i2c_bus *i2c_bus) { + if (!i2c_bus->speed) + return; + debug("%s: speed=%d\n", __func__, i2c_bus->speed); /* * Use PLLP - DP-04508-001_v06 datasheet indicates a divisor of 8 * here, in section 23.3.1, but in fact we seem to need a factor of @@ -73,7 +81,7 @@ static void i2c_init_controller(struct i2c_bus *i2c_bus) clock_start_periph_pll(i2c_bus->periph_id, CLOCK_ID_PERIPH, i2c_bus->speed * 2 * 8);
- if (i2c_bus->is_scs) { + if (i2c_bus->type == TYPE_114) { /* * T114 I2C went to a single clock source for standard/fast and * HS clock speeds. The new clock rate setting calculation is: @@ -98,7 +106,7 @@ static void i2c_init_controller(struct i2c_bus *i2c_bus) i2c_reset_controller(i2c_bus);
/* Configure I2C controller. */ - if (i2c_bus->is_dvc) { /* only for DVC I2C */ + if (i2c_bus->type == TYPE_DVC) { /* only for DVC I2C */ struct dvc_ctlr *dvc = (struct dvc_ctlr *)i2c_bus->regs;
setbits_le32(&dvc->ctrl3, DVC_CTRL_REG3_I2C_HW_SW_PROG_MASK); @@ -272,7 +280,7 @@ exit: return error; }
-static int tegra_i2c_write_data(struct i2c_bus *bus, u32 addr, u8 *data, +static int tegra_i2c_write_data(struct i2c_bus *i2c_bus, u32 addr, u8 *data, u32 len, bool end_with_repeated_start) { int error; @@ -286,14 +294,14 @@ static int tegra_i2c_write_data(struct i2c_bus *bus, u32 addr, u8 *data, trans_info.num_bytes = len; trans_info.is_10bit_address = 0;
- error = send_recv_packets(bus, &trans_info); + error = send_recv_packets(i2c_bus, &trans_info); if (error) debug("tegra_i2c_write_data: Error (%d) !!!\n", error);
return error; }
-static int tegra_i2c_read_data(struct i2c_bus *bus, u32 addr, u8 *data, +static int tegra_i2c_read_data(struct i2c_bus *i2c_bus, u32 addr, u8 *data, u32 len) { int error; @@ -305,52 +313,32 @@ static int tegra_i2c_read_data(struct i2c_bus *bus, u32 addr, u8 *data, trans_info.num_bytes = len; trans_info.is_10bit_address = 0;
- error = send_recv_packets(bus, &trans_info); + error = send_recv_packets(i2c_bus, &trans_info); if (error) debug("tegra_i2c_read_data: Error (%d) !!!\n", error);
return error; }
-#ifndef CONFIG_OF_CONTROL -#error "Please enable device tree support to use this driver" -#endif - -/** - * Check that a bus number is valid and return a pointer to it - * - * @param bus_num Bus number to check / return - * @return pointer to bus, if valid, else NULL - */ -static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap) +static int tegra_i2c_set_bus_speed(struct udevice *dev, unsigned int speed) { - struct i2c_bus *bus; + struct i2c_bus *i2c_bus = dev_get_priv(dev);
- bus = &i2c_controllers[adap->hwadapnr]; - if (!bus->inited) { - debug("%s: Bus %u not available\n", __func__, adap->hwadapnr); - return NULL; - } - - return bus; -} - -static unsigned int tegra_i2c_set_bus_speed(struct i2c_adapter *adap, - unsigned int speed) -{ - struct i2c_bus *bus; - - bus = tegra_i2c_get_bus(adap); - if (!bus) - return 0; - bus->speed = speed; - i2c_init_controller(bus); + i2c_bus->speed = speed; + i2c_init_controller(i2c_bus);
return 0; }
-static int i2c_get_config(const void *blob, int node, struct i2c_bus *i2c_bus) +static int tegra_i2c_probe(struct udevice *dev) { + struct i2c_bus *i2c_bus = dev_get_priv(dev); + const void *blob = gd->fdt_blob; + int node = dev->of_offset; + bool is_dvc; + + i2c_bus->id = dev->seq; + i2c_bus->type = dev_get_of_data(dev); i2c_bus->regs = (struct i2c_ctlr *)fdtdec_get_addr(blob, node, "reg");
/* @@ -358,7 +346,6 @@ static int i2c_get_config(const void *blob, int node, struct i2c_bus *i2c_bus) * far no one needs anything other than the default. */ i2c_bus->pinmux_config = FUNCMUX_DEFAULT; - i2c_bus->speed = fdtdec_get_int(blob, node, "clock-frequency", 0); i2c_bus->periph_id = clock_decode_periph_id(blob, node);
/* @@ -371,107 +358,25 @@ static int i2c_get_config(const void *blob, int node, struct i2c_bus *i2c_bus) * i2c_bus->pinmux_config = FUNCMUX_I2C2_PTA; */ if (i2c_bus->periph_id == -1) - return -FDT_ERR_NOTFOUND; + return -EINVAL;
- return 0; -} - -/* - * Process a list of nodes, adding them to our list of I2C ports. - * - * @param blob fdt blob - * @param node_list list of nodes to process (any <=0 are ignored) - * @param count number of nodes to process - * @param is_dvc 1 if these are DVC ports, 0 if standard I2C - * @param is_scs 1 if this HW uses a single clock source (T114+) - * @return 0 if ok, -1 on error - */ -static int process_nodes(const void *blob, int node_list[], int count, - int is_dvc, int is_scs) -{ - struct i2c_bus *i2c_bus; - int i; - - /* build the i2c_controllers[] for each controller */ - for (i = 0; i < count; i++) { - int node = node_list[i]; - - if (node <= 0) - continue; - - i2c_bus = &i2c_controllers[i]; - i2c_bus->id = i; - - if (i2c_get_config(blob, node, i2c_bus)) { - printf("i2c_init_board: failed to decode bus %d\n", i); - return -1; - } - - i2c_bus->is_scs = is_scs; - - i2c_bus->is_dvc = is_dvc; - if (is_dvc) { - i2c_bus->control = - &((struct dvc_ctlr *)i2c_bus->regs)->control; - } else { - i2c_bus->control = &i2c_bus->regs->control; - } - debug("%s: controller bus %d at %p, periph_id %d, speed %d: ", - is_dvc ? "dvc" : "i2c", i, i2c_bus->regs, - i2c_bus->periph_id, i2c_bus->speed); - i2c_init_controller(i2c_bus); - debug("ok\n"); - i2c_bus->inited = 1; - - /* Mark position as used */ - node_list[i] = -1; + is_dvc = dev_get_of_data(dev) == TYPE_DVC; + if (is_dvc) { + i2c_bus->control = + &((struct dvc_ctlr *)i2c_bus->regs)->control; + } else { + i2c_bus->control = &i2c_bus->regs->control; } + i2c_init_controller(i2c_bus); + debug("%s: controller bus %d at %p, periph_id %d, speed %d: ", + is_dvc ? "dvc" : "i2c", dev->seq, i2c_bus->regs, + i2c_bus->periph_id, i2c_bus->speed);
return 0; }
-/* Sadly there is no error return from this function */ -void i2c_init_board(void) -{ - int node_list[TEGRA_I2C_NUM_CONTROLLERS]; - const void *blob = gd->fdt_blob; - int count; - - /* First check for newer (T114+) I2C ports */ - count = fdtdec_find_aliases_for_id(blob, "i2c", - COMPAT_NVIDIA_TEGRA114_I2C, node_list, - TEGRA_I2C_NUM_CONTROLLERS); - if (process_nodes(blob, node_list, count, 0, 1)) - return; - - /* Now get the older (T20/T30) normal I2C ports */ - count = fdtdec_find_aliases_for_id(blob, "i2c", - COMPAT_NVIDIA_TEGRA20_I2C, node_list, - TEGRA_I2C_NUM_CONTROLLERS); - if (process_nodes(blob, node_list, count, 0, 0)) - return; - - /* Now look for dvc ports */ - count = fdtdec_add_aliases_for_id(blob, "i2c", - COMPAT_NVIDIA_TEGRA20_DVC, node_list, - TEGRA_I2C_NUM_CONTROLLERS); - if (process_nodes(blob, node_list, count, 1, 0)) - return; -} - -static void tegra_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) -{ - /* No i2c support prior to relocation */ - if (!(gd->flags & GD_FLG_RELOC)) - return; - - /* This will override the speed selected in the fdt for that port */ - debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr); - i2c_set_bus_speed(speed); -} - /* i2c write version without the register address */ -static int i2c_write_data(struct i2c_bus *bus, uchar chip, uchar *buffer, +static int i2c_write_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buffer, int len, bool end_with_repeated_start) { int rc; @@ -484,7 +389,7 @@ static int i2c_write_data(struct i2c_bus *bus, uchar chip, uchar *buffer, debug("\n");
/* Shift 7-bit address over for lower-level i2c functions */ - rc = tegra_i2c_write_data(bus, chip << 1, buffer, len, + rc = tegra_i2c_write_data(i2c_bus, chip << 1, buffer, len, end_with_repeated_start); if (rc) debug("i2c_write_data(): rc=%d\n", rc); @@ -493,14 +398,14 @@ static int i2c_write_data(struct i2c_bus *bus, uchar chip, uchar *buffer, }
/* i2c read version without the register address */ -static int i2c_read_data(struct i2c_bus *bus, uchar chip, uchar *buffer, - int len) +static int i2c_read_data(struct i2c_bus *i2c_bus, uchar chip, uchar *buffer, + int len) { int rc;
debug("inside i2c_read_data():\n"); /* Shift 7-bit address over for lower-level i2c functions */ - rc = tegra_i2c_read_data(bus, chip << 1, buffer, len); + rc = tegra_i2c_read_data(i2c_bus, chip << 1, buffer, len); if (rc) { debug("i2c_read_data(): rc=%d\n", rc); return rc; @@ -515,133 +420,93 @@ static int i2c_read_data(struct i2c_bus *bus, uchar chip, uchar *buffer, return 0; }
-/* Probe to see if a chip is present. */ -static int tegra_i2c_probe(struct i2c_adapter *adap, uchar chip) +static int tegra_i2c_set_offset_len(struct udevice *dev, const uint olen) { - struct i2c_bus *bus; - int rc; - uchar reg; - - debug("i2c_probe: addr=0x%x\n", chip); - bus = tegra_i2c_get_bus(adap); - if (!bus) - return 1; - reg = 0; - rc = i2c_write_data(bus, chip, ®, 1, false); - if (rc) { - debug("Error probing 0x%x.\n", chip); - return 1; - } - return 0; -} + if (olen < 4) + return 0;
-static int i2c_addr_ok(const uint addr, const int alen) -{ - /* We support 7 or 10 bit addresses, so one or two bytes each */ - return alen == 1 || alen == 2; + return -EINVAL; }
-/* Read bytes */ -static int tegra_i2c_read(struct i2c_adapter *adap, uchar chip, uint addr, - int alen, uchar *buffer, int len) +static int tegra_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, + int nmsgs) { - struct i2c_bus *bus; - uint offset; - int i; - - debug("i2c_read: chip=0x%x, addr=0x%x, alen=0x%x len=0x%x\n", - chip, addr, alen, len); - bus = tegra_i2c_get_bus(adap); - if (!bus) - return 1; - if (!i2c_addr_ok(addr, alen)) { - debug("i2c_read: Bad address %x.%d.\n", addr, alen); - return 1; - } - for (offset = 0; offset < len; offset++) { - if (alen) { - uchar data[alen]; - for (i = 0; i < alen; i++) { - data[alen - i - 1] = - (addr + offset) >> (8 * i); - } - if (i2c_write_data(bus, chip, data, alen, true)) { - debug("i2c_read: error sending (0x%x)\n", - addr); - return 1; - } + struct i2c_bus *i2c_bus = dev_get_priv(bus); + int ret; + + debug("i2c_xfer: %d messages\n", nmsgs); + for (; nmsgs > 0; nmsgs--, msg++) { + bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD); + + debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len); + if (msg->flags & I2C_M_RD) { + ret = i2c_read_data(i2c_bus, msg->addr, msg->buf, + msg->len); + } else { + ret = i2c_write_data(i2c_bus, msg->addr, msg->buf, + msg->len, next_is_read); } - if (i2c_read_data(bus, chip, buffer + offset, 1)) { - debug("i2c_read: error reading (0x%x)\n", addr); - return 1; + if (ret) { + debug("i2c_write: error sending\n"); + return -EREMOTEIO; } }
return 0; }
-/* Write bytes */ -static int tegra_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr, - int alen, uchar *buffer, int len) +int tegra_i2c_get_dvc_bus(struct udevice **busp) { - struct i2c_bus *bus; - uint offset; - int i; - - debug("i2c_write: chip=0x%x, addr=0x%x, alen=0x%x len=0x%x\n", - chip, addr, alen, len); - bus = tegra_i2c_get_bus(adap); - if (!bus) - return 1; - if (!i2c_addr_ok(addr, alen)) { - debug("i2c_write: Bad address %x.%d.\n", addr, alen); - return 1; - } - for (offset = 0; offset < len; offset++) { - uchar data[alen + 1]; - for (i = 0; i < alen; i++) - data[alen - i - 1] = (addr + offset) >> (8 * i); - data[alen] = buffer[offset]; - if (i2c_write_data(bus, chip, data, alen + 1, false)) { - debug("i2c_write: error sending (0x%x)\n", addr); - return 1; + struct udevice *bus; + + for (uclass_first_device(UCLASS_I2C, &bus); + bus; + uclass_next_device(&bus)) { + if (dev_get_of_data(bus) == TYPE_DVC) { + *busp = bus; + return 0; } }
- return 0; + return -ENODEV; }
-int tegra_i2c_get_dvc_bus_num(void) -{ - int i; +static const struct dm_i2c_ops tegra_i2c_ops = { + .xfer = tegra_i2c_xfer, + .set_bus_speed = tegra_i2c_set_bus_speed, + .set_offset_len = tegra_i2c_set_offset_len, +};
- for (i = 0; i < TEGRA_I2C_NUM_CONTROLLERS; i++) { - struct i2c_bus *bus = &i2c_controllers[i]; +static int tegra_i2c_child_pre_probe(struct udevice *dev) +{ + struct dm_i2c_chip *i2c_chip = dev_get_parentdata(dev);
- if (bus->inited && bus->is_dvc) - return i; - } + if (dev->of_offset == -1) + return 0; + return i2c_chip_ofdata_to_platdata(gd->fdt_blob, dev->of_offset, + i2c_chip); +}
- return -1; +static int tegra_i2c_ofdata_to_platdata(struct udevice *dev) +{ + return 0; }
-/* - * Register soft i2c adapters - */ -U_BOOT_I2C_ADAP_COMPLETE(tegra0, tegra_i2c_init, tegra_i2c_probe, - tegra_i2c_read, tegra_i2c_write, - tegra_i2c_set_bus_speed, 100000, 0, 0) -U_BOOT_I2C_ADAP_COMPLETE(tegra1, tegra_i2c_init, tegra_i2c_probe, - tegra_i2c_read, tegra_i2c_write, - tegra_i2c_set_bus_speed, 100000, 0, 1) -U_BOOT_I2C_ADAP_COMPLETE(tegra2, tegra_i2c_init, tegra_i2c_probe, - tegra_i2c_read, tegra_i2c_write, - tegra_i2c_set_bus_speed, 100000, 0, 2) -U_BOOT_I2C_ADAP_COMPLETE(tegra3, tegra_i2c_init, tegra_i2c_probe, - tegra_i2c_read, tegra_i2c_write, - tegra_i2c_set_bus_speed, 100000, 0, 3) -#if TEGRA_I2C_NUM_CONTROLLERS > 4 -U_BOOT_I2C_ADAP_COMPLETE(tegra4, tegra_i2c_init, tegra_i2c_probe, - tegra_i2c_read, tegra_i2c_write, - tegra_i2c_set_bus_speed, 100000, 0, 4) -#endif +static const struct udevice_id tegra_i2c_ids[] = { + { .compatible = "nvidia,tegra114-i2c", .data = TYPE_114 }, + { .compatible = "nvidia,tegra20-i2c", .data = TYPE_STD }, + { .compatible = "nvidia,tegra20-i2c-dvc", .data = TYPE_DVC }, + { } +}; + +U_BOOT_DRIVER(i2c_tegra) = { + .name = "i2c_tegra", + .id = UCLASS_I2C, + .of_match = tegra_i2c_ids, + .ofdata_to_platdata = tegra_i2c_ofdata_to_platdata, + .probe = tegra_i2c_probe, + .per_child_auto_alloc_size = sizeof(struct dm_i2c_chip), + .child_pre_probe = tegra_i2c_child_pre_probe, + .priv_auto_alloc_size = sizeof(struct i2c_bus), + .ops = &tegra_i2c_ops, +}; diff --git a/drivers/power/tps6586x.c b/drivers/power/tps6586x.c index d29d969..29bab4c 100644 --- a/drivers/power/tps6586x.c +++ b/drivers/power/tps6586x.c @@ -10,9 +10,7 @@ #include <asm/io.h> #include <i2c.h>
-static int bus_num; /* I2C bus we are on */ -#define I2C_ADDRESS 0x34 /* chip requires this address */ -static char inited; /* 1 if we have been inited */ +static struct udevice *tps6586x_dev;
enum { /* Registers that we access */ @@ -37,13 +35,9 @@ static int tps6586x_read(int reg) int i; uchar data; int retval = -1; - int old_bus_num; - - old_bus_num = i2c_get_bus_num(); - i2c_set_bus_num(bus_num);
for (i = 0; i < MAX_I2C_RETRY; ++i) { - if (!i2c_read(I2C_ADDRESS, reg, 1, &data, 1)) { + if (!i2c_read(tps6586x_dev, reg, &data, 1)) { retval = (int)data; goto exit; } @@ -53,7 +47,6 @@ static int tps6586x_read(int reg) }
exit: - i2c_set_bus_num(old_bus_num); debug("pmu_read %x=%x\n", reg, retval); if (retval < 0) debug("%s: failed to read register %#x: %d\n", __func__, reg, @@ -65,13 +58,9 @@ static int tps6586x_write(int reg, uchar *data, uint len) { int i; int retval = -1; - int old_bus_num; - - old_bus_num = i2c_get_bus_num(); - i2c_set_bus_num(bus_num);
for (i = 0; i < MAX_I2C_RETRY; ++i) { - if (!i2c_write(I2C_ADDRESS, reg, 1, data, len)) { + if (!i2c_write(tps6586x_dev, reg, data, len)) { retval = 0; goto exit; } @@ -81,7 +70,6 @@ static int tps6586x_write(int reg, uchar *data, uint len) }
exit: - i2c_set_bus_num(old_bus_num); debug("pmu_write %x=%x: ", reg, retval); for (i = 0; i < len; i++) debug("%x ", data[i]); @@ -163,7 +151,7 @@ int tps6586x_set_pwm_mode(int mask) uchar val; int ret;
- assert(inited); + assert(tps6586x_dev); ret = tps6586x_read(PFM_MODE); if (ret != -1) { val = (uchar)ret; @@ -184,7 +172,7 @@ int tps6586x_adjust_sm0_sm1(int sm0_target, int sm1_target, int step, int rate, int sm0, sm1; int bad;
- assert(inited); + assert(tps6586x_dev);
/* get current voltage settings */ if (read_voltages(&sm0, &sm1)) { @@ -255,10 +243,9 @@ int tps6586x_adjust_sm0_sm1(int sm0_target, int sm1_target, int step, int rate, return bad ? -1 : 0; }
-int tps6586x_init(int bus) +int tps6586x_init(struct udevice *dev) { - bus_num = bus; - inited = 1; + tps6586x_dev = dev;
return 0; } diff --git a/include/configs/apalis_t30.h b/include/configs/apalis_t30.h index 3cde923..61809fc 100644 --- a/include/configs/apalis_t30.h +++ b/include/configs/apalis_t30.h @@ -26,10 +26,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/beaver.h b/include/configs/beaver.h index 164b2dd..5d765f3 100644 --- a/include/configs/beaver.h +++ b/include/configs/beaver.h @@ -40,10 +40,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/cardhu.h b/include/configs/cardhu.h index 09129c7..758b7ad 100644 --- a/include/configs/cardhu.h +++ b/include/configs/cardhu.h @@ -43,12 +43,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS -#define CONFIG_SYS_MAX_I2C_BUS TEGRA_I2C_NUM_CONTROLLERS -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/colibri_t30.h b/include/configs/colibri_t30.h index a582e25..ce6f23b 100644 --- a/include/configs/colibri_t30.h +++ b/include/configs/colibri_t30.h @@ -25,10 +25,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/dalmore.h b/include/configs/dalmore.h index ff7ec4a..0b04ee6 100644 --- a/include/configs/dalmore.h +++ b/include/configs/dalmore.h @@ -36,12 +36,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS -#define CONFIG_SYS_MAX_I2C_BUS TEGRA_I2C_NUM_CONTROLLERS -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h index d67c025..a7d7665 100644 --- a/include/configs/jetson-tk1.h +++ b/include/configs/jetson-tk1.h @@ -25,12 +25,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS -#define CONFIG_SYS_MAX_I2C_BUS TEGRA_I2C_NUM_CONTROLLERS -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/norrin.h b/include/configs/norrin.h index ef0b63d..d393aef 100644 --- a/include/configs/norrin.h +++ b/include/configs/norrin.h @@ -25,12 +25,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS -#define CONFIG_SYS_MAX_I2C_BUS TEGRA_I2C_NUM_CONTROLLERS -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h index 04e4f82..5f77051 100644 --- a/include/configs/seaboard.h +++ b/include/configs/seaboard.h @@ -37,10 +37,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/tec-ng.h b/include/configs/tec-ng.h index 51f87da..e37b233 100644 --- a/include/configs/tec-ng.h +++ b/include/configs/tec-ng.h @@ -23,12 +23,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS -#define CONFIG_SYS_MAX_I2C_BUS TEGRA_I2C_NUM_CONTROLLERS -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/tegra-common.h b/include/configs/tegra-common.h index d690045..0685328 100644 --- a/include/configs/tegra-common.h +++ b/include/configs/tegra-common.h @@ -26,6 +26,7 @@ #endif #define CONFIG_DM_SPI #define CONFIG_DM_SPI_FLASH +#define CONFIG_DM_I2C
#define CONFIG_SYS_TIMER_RATE 1000000 #define CONFIG_SYS_TIMER_COUNTER NV_PA_TMRUS_BASE diff --git a/include/configs/tegra114-common.h b/include/configs/tegra114-common.h index 555c237..9eba5d5 100644 --- a/include/configs/tegra114-common.h +++ b/include/configs/tegra114-common.h @@ -76,9 +76,6 @@ #define CONFIG_SYS_SPL_MALLOC_START 0x80090000 #define CONFIG_SPL_STACK 0x800ffffc
-/* Total I2C ports on Tegra114 */ -#define TEGRA_I2C_NUM_CONTROLLERS 5 - /* For USB EHCI controller */ #define CONFIG_EHCI_IS_TDI #define CONFIG_USB_EHCI_TXFIFO_THRESH 0x10 diff --git a/include/configs/tegra124-common.h b/include/configs/tegra124-common.h index 61e5026..f2b3774 100644 --- a/include/configs/tegra124-common.h +++ b/include/configs/tegra124-common.h @@ -68,9 +68,6 @@ #define CONFIG_SYS_SPL_MALLOC_START 0x80090000 #define CONFIG_SPL_STACK 0x800ffffc
-/* Total I2C ports on Tegra124 */ -#define TEGRA_I2C_NUM_CONTROLLERS 5 - /* For USB EHCI controller */ #define CONFIG_EHCI_IS_TDI #define CONFIG_USB_EHCI_TXFIFO_THRESH 0x10 diff --git a/include/configs/tegra20-common.h b/include/configs/tegra20-common.h index 21bf977..6330281 100644 --- a/include/configs/tegra20-common.h +++ b/include/configs/tegra20-common.h @@ -97,9 +97,6 @@ #define CONFIG_EHCI_IS_TDI #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 1
-/* Total I2C ports on Tegra20 */ -#define TEGRA_I2C_NUM_CONTROLLERS 4 - #define CONFIG_SYS_NAND_SELF_INIT #define CONFIG_SYS_NAND_ONFI_DETECTION
diff --git a/include/configs/tegra30-common.h b/include/configs/tegra30-common.h index 443c842..bfdbeb7 100644 --- a/include/configs/tegra30-common.h +++ b/include/configs/tegra30-common.h @@ -73,9 +73,6 @@ #define CONFIG_SYS_SPL_MALLOC_START 0x80090000 #define CONFIG_SPL_STACK 0x800ffffc
-/* Total I2C ports on Tegra30 */ -#define TEGRA_I2C_NUM_CONTROLLERS 5 - /* For USB EHCI controller */ #define CONFIG_EHCI_IS_TDI #define CONFIG_USB_EHCI_TXFIFO_THRESH 0x10 diff --git a/include/configs/trimslice.h b/include/configs/trimslice.h index 7c00642..a254f86 100644 --- a/include/configs/trimslice.h +++ b/include/configs/trimslice.h @@ -34,10 +34,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/venice2.h b/include/configs/venice2.h index 6897aa8..8880de8 100644 --- a/include/configs/venice2.h +++ b/include/configs/venice2.h @@ -25,12 +25,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_I2C_MULTI_BUS -#define CONFIG_SYS_MAX_I2C_BUS TEGRA_I2C_NUM_CONTROLLERS -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/configs/whistler.h b/include/configs/whistler.h index 10e70d2..e083cbd 100644 --- a/include/configs/whistler.h +++ b/include/configs/whistler.h @@ -26,10 +26,7 @@
/* I2C */ #define CONFIG_SYS_I2C_TEGRA -#define CONFIG_SYS_I2C_INIT_BOARD -#define CONFIG_SYS_I2C_SPEED 100000 #define CONFIG_CMD_I2C -#define CONFIG_SYS_I2C
/* SD/MMC */ #define CONFIG_MMC diff --git a/include/tps6586x.h b/include/tps6586x.h index 78ce428..eefc95f 100644 --- a/include/tps6586x.h +++ b/include/tps6586x.h @@ -44,9 +44,9 @@ int tps6586x_adjust_sm0_sm1(int sm0_target, int sm1_target, int step, int rate, * Set up the TPS6586X I2C bus number. This will be used for all operations * on the device. This function must be called before using other functions. * - * @param bus I2C bus number containing the TPS6586X chip + * @param bus I2C bus containing the TPS6586X chip * @return 0 (always succeeds) */ -int tps6586x_init(int bus); +int tps6586x_init(struct udevice *bus);
#endif /* _TPS6586X_H_ */
Hi Simon,
On Mon, 24 Nov 2014 11:57:24 -0700 Simon Glass sjg@chromium.org wrote:
+U_BOOT_DRIVER(i2c_tegra) = {
- .name = "i2c_tegra",
- .id = UCLASS_I2C,
- .of_match = tegra_i2c_ids,
- .ofdata_to_platdata = tegra_i2c_ofdata_to_platdata,
- .probe = tegra_i2c_probe,
- .per_child_auto_alloc_size = sizeof(struct dm_i2c_chip),
I guess every i2c driver is supposed to have .per_child_auto_alloc_size = sizeof(struct dm_i2c_chip), but it it unfortunate.
Can we have it in uclass too?
If U_BOOT_DRIVER does not have .per_child_auto_alloc_size but UCLASS_DRIVER has it, use it.
If both U_BOOT_DRIVER and UCLASS_DRIVER have .per_child_auto_alloc_size, the former takes precedence.
- .child_pre_probe = tegra_i2c_child_pre_probe,
Ditto.
tegra_i2c_child_pre_probe() does not seem Tegra-specific code at all.
Best Regards Masahiro Yamada
Hi Simon,
Another comment for this patch.
On Mon, 24 Nov 2014 11:57:24 -0700 Simon Glass sjg@chromium.org wrote:
-/* Probe to see if a chip is present. */ -static int tegra_i2c_probe(struct i2c_adapter *adap, uchar chip) +static int tegra_i2c_set_offset_len(struct udevice *dev, const uint olen) {
- struct i2c_bus *bus;
- int rc;
- uchar reg;
- debug("i2c_probe: addr=0x%x\n", chip);
- bus = tegra_i2c_get_bus(adap);
- if (!bus)
return 1;- reg = 0;
- rc = i2c_write_data(bus, chip, ®, 1, false);
- if (rc) {
debug("Error probing 0x%x.\n", chip);return 1;- }
- return 0;
-}
- if (olen < 4)
return 0;
The maximum length 4 is not tegra-specific. I can't find good reason to have .set_offset_len handler.
Best Regards Masahiro Yamada
Hi Masahiro,
On 3 December 2014 at 06:30, Masahiro Yamada yamada.m@jp.panasonic.com wrote:
Hi Simon,
Another comment for this patch.
On Mon, 24 Nov 2014 11:57:24 -0700 Simon Glass sjg@chromium.org wrote:
-/* Probe to see if a chip is present. */ -static int tegra_i2c_probe(struct i2c_adapter *adap, uchar chip) +static int tegra_i2c_set_offset_len(struct udevice *dev, const uint olen) {
struct i2c_bus *bus;int rc;uchar reg;debug("i2c_probe: addr=0x%x\n", chip);bus = tegra_i2c_get_bus(adap);if (!bus)return 1;reg = 0;rc = i2c_write_data(bus, chip, ®, 1, false);if (rc) {debug("Error probing 0x%x.\n", chip);return 1;}return 0;-}
if (olen < 4)return 0;The maximum length 4 is not tegra-specific. I can't find good reason to have .set_offset_len handler.
It's not needed here as 4 is the maximum supported by the uclass. Some drivers may support less though, so perhaps we should keep the method?
Regards, Simon
Hi Simon,
2014-12-04 0:23 GMT+09:00 Simon Glass sjg@chromium.org:
-/* Probe to see if a chip is present. */ -static int tegra_i2c_probe(struct i2c_adapter *adap, uchar chip) +static int tegra_i2c_set_offset_len(struct udevice *dev, const uint olen) {
struct i2c_bus *bus;int rc;uchar reg;debug("i2c_probe: addr=0x%x\n", chip);bus = tegra_i2c_get_bus(adap);if (!bus)return 1;reg = 0;rc = i2c_write_data(bus, chip, ®, 1, false);if (rc) {debug("Error probing 0x%x.\n", chip);return 1;}return 0;-}
if (olen < 4)return 0;The maximum length 4 is not tegra-specific. I can't find good reason to have .set_offset_len handler.
It's not needed here as 4 is the maximum supported by the uclass. Some drivers may support less though, so perhaps we should keep the method?
I think this handler is totally useless.
Moreover, the offset address within a chip is out of the scope of I2C protocol. I think it is odd for each of I2C drivers to care about it.
Hi Masahiro,
On 3 December 2014 at 08:52, Masahiro YAMADA yamada.m@jp.panasonic.com wrote:
Hi Simon,
2014-12-04 0:23 GMT+09:00 Simon Glass sjg@chromium.org:
-/* Probe to see if a chip is present. */ -static int tegra_i2c_probe(struct i2c_adapter *adap, uchar chip) +static int tegra_i2c_set_offset_len(struct udevice *dev, const uint olen) {
struct i2c_bus *bus;int rc;uchar reg;debug("i2c_probe: addr=0x%x\n", chip);bus = tegra_i2c_get_bus(adap);if (!bus)return 1;reg = 0;rc = i2c_write_data(bus, chip, ®, 1, false);if (rc) {debug("Error probing 0x%x.\n", chip);return 1;}return 0;-}
if (olen < 4)return 0;The maximum length 4 is not tegra-specific. I can't find good reason to have .set_offset_len handler.
It's not needed here as 4 is the maximum supported by the uclass. Some drivers may support less though, so perhaps we should keep the method?
I think this handler is totally useless.
Moreover, the offset address within a chip is out of the scope of I2C protocol. I think it is odd for each of I2C drivers to care about it.
The Intel SPI driver requires us to figure out the offset from the spi_xfer() call, and it is painful. I wondered if there might be I2C drivers that need to do the same thing. I have not looked.
Regards, Simon
Hello all,
On 11/24/2014 07:57 PM, Simon Glass wrote:
This series adds I2C support to driver model. It has become apparent that this is a high priority as it is widely used. It follows along to some extent from the SPI conversion.
Several changes are made from the original I2C implementations.
Firstly it is not necessary to specify the chip address with every call, since each chip knows its own address - it is stored in struct dm_i2c_chip which is attached to each chip on the I2C bus. However, this information *is* passed to the driver since I presume most drivers need it and it would be cumbersome to look up in every call.
Secondly there is no concept of a 'current' I2C bus so all associated logic is removed. With driver model i2c_set_bus_num() and i2c_get_bus_num() are not available. Since the chip device specifies both the bus and the chip address, there is no need for this concept. It also causes problems when one driver changes the current bus and forgets to change it back.
Thirdly initialisation is handled by driver model's normal probe() method on each device so there should be no need for i2c_init_all(), i2c_init(), i2c_init_board(), i2c_board_late_init() and board_i2c_init().
I2C muxes are not yet supported. To support these we will need to maintain state of the current mux settings to avoid resetting every mux every time. Probably we need to add a sandbox I2C mux driver to permit testing of this. This can probably be done later.
Platform data is not yet supported either, only device tree. The U_BOOT_I2C_MKENT_COMPLETE() and U_BOOT_I2C_ADAP_COMPLETE() macros are not used. Also struct i2c_adapter is not defined anymore. This will need to be addressed, perhaps as part of converting over a board that does not use device tree, assuming that we want to support this.
The following I2C CONFIGs are no-longer needed when driver model is used:
CONFIG_SYS_I2C_INIT_BOARD - each I2C bus is inited in its probe() method CONFIG_I2C_MULTI_BUS - we always support multi-bus with driver model CONFIG_SYS_MAX_I2C_BUS - the device tree aliases define available buses CONFIG_SYS_I2C_SPEED - the device tree specifies the speed for each bus CONFIG_SYS_I2C - this is the 'new old' API, now deprecated
There are a few SPI patches included here due to a dependency on a new device binding function.
v3 changes the uclass to driver interface significantly. It is now a list of messages to be processed by the driver. This matches the Linux user space API which may be a benefit. It does introduce one complication, which is that the protocol does not support separate chip offset and data. I have enhanced it to do so.
This series is available at u-boot-dm/i2c-working2.
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
- Adjust for slightly altered I2C uclass API
- Add new 'i2c flags' command to get/set chip flags
- Update for new uclass <=> driver interface
- Update emulator for new uclass interface
- Update for new uclass <=> emulateor interface
- Change driver to support new message-based I2C uclass API
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
- Change alen to int so that it can be -1 (this was a bug)
- Call the deblock() method for 'i2c reset'
- Update commit message for EEPROM driver
- Add a test for automatic binding of generic I2C devices
- Add a new asm/test.h header for tests in sandbox
- Adjust tegra_i2c_child_pre_probe() to permit generic I2C devices
- Correct the compatible strings for I2C buses
- Don't init if the speed is 0, since this breaks the controller
- Expand coverage to all Tegra boards
Simon Glass (10): dm: i2c: Add a uclass for I2C dm: i2c: Implement driver model support in the i2c command dm: i2c: Add I2C emulation driver for sandbox dm: i2c: Add a sandbox I2C driver dm: i2c: Add an I2C EEPROM simulator dm: i2c: config: Enable I2C for sandbox using driver model dm: i2c: dts: Add an I2C bus for sandbox dm: Add a simple EEPROM driver dm: i2c: Add tests for I2C dm: i2c: tegra: Convert to driver model
arch/arm/cpu/tegra20-common/pmu.c | 21 +- arch/arm/dts/tegra124-jetson-tk1.dts | 1 - arch/arm/dts/tegra124-norrin.dts | 1 - arch/arm/dts/tegra30-tec-ng.dts | 4 + arch/arm/include/asm/arch-tegra/tegra_i2c.h | 2 +- arch/sandbox/dts/sandbox.dts | 17 ++ arch/sandbox/include/asm/test.h | 15 + board/avionic-design/common/tamonten-ng.c | 12 +- board/nvidia/cardhu/cardhu.c | 13 +- board/nvidia/common/board.c | 4 - board/nvidia/dalmore/dalmore.c | 21 +- board/nvidia/whistler/whistler.c | 29 +- board/toradex/apalis_t30/apalis_t30.c | 19 +- common/cmd_i2c.c | 376 +++++++++++++++++++++---- drivers/i2c/Makefile | 2 + drivers/i2c/i2c-emul-uclass.c | 14 + drivers/i2c/i2c-uclass.c | 411 ++++++++++++++++++++++++++++ drivers/i2c/sandbox_i2c.c | 142 ++++++++++ drivers/i2c/tegra_i2c.c | 363 ++++++++---------------- drivers/misc/Makefile | 4 + drivers/misc/i2c_eeprom.c | 51 ++++ drivers/misc/i2c_eeprom_emul.c | 108 ++++++++ drivers/power/tps6586x.c | 27 +- include/config_fallbacks.h | 6 + include/configs/apalis_t30.h | 3 - include/configs/beaver.h | 3 - include/configs/cardhu.h | 5 - include/configs/colibri_t30.h | 3 - include/configs/dalmore.h | 5 - include/configs/jetson-tk1.h | 5 - include/configs/norrin.h | 5 - include/configs/sandbox.h | 6 + include/configs/seaboard.h | 3 - include/configs/tec-ng.h | 5 - include/configs/tegra-common.h | 1 + include/configs/tegra114-common.h | 3 - include/configs/tegra124-common.h | 3 - include/configs/tegra20-common.h | 3 - include/configs/tegra30-common.h | 3 - include/configs/trimslice.h | 3 - include/configs/venice2.h | 5 - include/configs/whistler.h | 3 - include/dm/uclass-id.h | 4 + include/i2c.h | 348 +++++++++++++++++++++++ include/i2c_eeprom.h | 19 ++ include/tps6586x.h | 4 +- test/dm/Makefile | 1 + test/dm/i2c.c | 112 ++++++++ test/dm/test.dts | 17 ++ 49 files changed, 1805 insertions(+), 430 deletions(-) create mode 100644 arch/sandbox/include/asm/test.h create mode 100644 drivers/i2c/i2c-emul-uclass.c create mode 100644 drivers/i2c/i2c-uclass.c create mode 100644 drivers/i2c/sandbox_i2c.c create mode 100644 drivers/misc/i2c_eeprom.c create mode 100644 drivers/misc/i2c_eeprom_emul.c create mode 100644 include/i2c_eeprom.h create mode 100644 test/dm/i2c.c
This comment will touch driver model, i2c and the pmic.
I have ported the s3c24x0_i2c driver to new dm i2c api - will send after dm i2c tree merge.
I tested this on Trats2(Exynos4412) and Arndale(Exynos5250) and the second one also with the High Speed I2C on port 0.
It works fine for me. I have also rebased my pmic framework on it - which was quite backward, because of various other side tasks like this one.
Using dm i2c framework for PMIC actually forces the previous assumed pmic hierarchy, which had look like this: - udev PMIC(parent:root, uclass: PMIC) - udev REGULATOR(parent: PMIC, uclass REGULATOR)
then both devices can share some data parent<->child
Let's assume that we have some pmic device(more than one functionality in a package) - it uses the same address on I2C0 for both PMIC and REGULATOR drivers.
So we have one physically device and two drivers (two uclass devices): - PMIC driver for some not standard settings by raw I/O, - REGULATOR driver for some defined regulator ops
Single device, single fdt node, and two (or more) uclasses.
With the present dm/i2c assumptions it should looks like: - udev PMIC(parent:I2C0, uclass: PMIC) - udev REGULATOR(parent: I2C0, uclass REGULATOR)
This allows use I2C by the regulator and pmic if the second one is probed - i2c device will be created.
But in the device-tree we have only one device node like: i2c0@.. { pmic@.. { }; };
and for this node we can bind only the ONE driver.
The short flow for e.g.i2c0 is: 1. i2c_post_bind() - called after bind i2c0 i2c uclass driver 2. dm_scan_fdt_node() - scan each i2c0 subnode, e.g. mentioned "pmic" 3. lists_bind_fdt() - scan U-Boot drivers linked list 4. device_bind() - bind the compatible driver 4. Have I2C0 bus and pmic i2c chip(bus child)
And the child_pre_probe() implementation of i2c driver, takes care of i2c chip which was bind as a I2C bus child.
Let's consider this two cases: case 1: # FDT pmic subnode without defined regulators # requires only PMIC uclass driver # situation is clear
case 2: # FDT pmic subnode with defined regulators # requires PMIC uclass driver and REGULATOR uclass driver to bind # situation is unclear
And now, where is the problem? To bind regulator - device_bind() should be called with I2C bus udevice as a parent. Doing this, we lose the PMIC <-> REGULATOR relationship, and those sub-devices can't share any data.
This quite good solution allows to acces the I2C bus also by the regulator, because the device_bind() will be called with PMIC's "of_offset" as an argument.
When bind and probe the regulator driver? In the first pmic framework patchset, there was a function pmic_init_dm - and this was for manually bind and probe the drivers - then it was working well. Now driver-model can bind the i2c devices automatically - so the pmic_init_dm() function can be limited to probe the pmic drivers only.
Testing: For my tests I solved this by manually bind REGULATOR driver in PMIC driver, with the udev I2C bus as a parent for the REGULATOR.
This is very unclear because, the true parent(PMIC) changes it's child(REGULATOR) parent field to the bus udevice which is -> I2C bus. So we have: #|_bus_parent ###|_bus_child ---- parent: #parent_bus #####|_bus_child_child ---- parent: #parent_bus (wrong!)
And now, the REGULATOR don't know that PMIC is the parent. So the things are going to be complicated.
What do you think about extend the struct udevice by the bus? E.g. : struct udevice { struct driver *driver; const char *name; void *platdata; int of_offset; const struct udevice_id *of_id; struct udevice *parent; + struct udevice *bus; void *priv; ...
In such device structure, the true parent device which represents a physical device like PMIC(in this case), could bind few and different uclass devices.
To get this more simply, the mentioned PMIC which can consists of few subsystems, like: - regulators - charger - mfd - rtc - adc, and some more as a parent - should be able to bind a proper class driver for each functionality - add a number of childs.
The another solution is to add new subnodes with different compatibles, but it also requires some combinations - and still the parent is not a bus.
I think that separating a bus from parent<->child relationship make the driver model more flexible.
Please comment, Best regards,
Hi Przemyslaw,
On 3 December 2014 at 09:13, Przemyslaw Marczak p.marczak@samsung.com wrote:
Hello all,
On 11/24/2014 07:57 PM, Simon Glass wrote:
This series adds I2C support to driver model. It has become apparent that this is a high priority as it is widely used. It follows along to some extent from the SPI conversion.
Several changes are made from the original I2C implementations.
Firstly it is not necessary to specify the chip address with every call, since each chip knows its own address - it is stored in struct dm_i2c_chip which is attached to each chip on the I2C bus. However, this information *is* passed to the driver since I presume most drivers need it and it would be cumbersome to look up in every call.
Secondly there is no concept of a 'current' I2C bus so all associated logic is removed. With driver model i2c_set_bus_num() and i2c_get_bus_num() are not available. Since the chip device specifies both the bus and the chip address, there is no need for this concept. It also causes problems when one driver changes the current bus and forgets to change it back.
Thirdly initialisation is handled by driver model's normal probe() method on each device so there should be no need for i2c_init_all(), i2c_init(), i2c_init_board(), i2c_board_late_init() and board_i2c_init().
I2C muxes are not yet supported. To support these we will need to maintain state of the current mux settings to avoid resetting every mux every time. Probably we need to add a sandbox I2C mux driver to permit testing of this. This can probably be done later.
Platform data is not yet supported either, only device tree. The U_BOOT_I2C_MKENT_COMPLETE() and U_BOOT_I2C_ADAP_COMPLETE() macros are not used. Also struct i2c_adapter is not defined anymore. This will need to be addressed, perhaps as part of converting over a board that does not use device tree, assuming that we want to support this.
The following I2C CONFIGs are no-longer needed when driver model is used:
CONFIG_SYS_I2C_INIT_BOARD - each I2C bus is inited in its probe() method CONFIG_I2C_MULTI_BUS - we always support multi-bus with driver model CONFIG_SYS_MAX_I2C_BUS - the device tree aliases define available buses CONFIG_SYS_I2C_SPEED - the device tree specifies the speed for each bus CONFIG_SYS_I2C - this is the 'new old' API, now deprecated
There are a few SPI patches included here due to a dependency on a new device binding function.
v3 changes the uclass to driver interface significantly. It is now a list of messages to be processed by the driver. This matches the Linux user space API which may be a benefit. It does introduce one complication, which is that the protocol does not support separate chip offset and data. I have enhanced it to do so.
This series is available at u-boot-dm/i2c-working2.
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
- Adjust for slightly altered I2C uclass API
- Add new 'i2c flags' command to get/set chip flags
- Update for new uclass <=> driver interface
- Update emulator for new uclass interface
- Update for new uclass <=> emulateor interface
- Change driver to support new message-based I2C uclass API
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
- Change alen to int so that it can be -1 (this was a bug)
- Call the deblock() method for 'i2c reset'
- Update commit message for EEPROM driver
- Add a test for automatic binding of generic I2C devices
- Add a new asm/test.h header for tests in sandbox
- Adjust tegra_i2c_child_pre_probe() to permit generic I2C devices
- Correct the compatible strings for I2C buses
- Don't init if the speed is 0, since this breaks the controller
- Expand coverage to all Tegra boards
Simon Glass (10): dm: i2c: Add a uclass for I2C dm: i2c: Implement driver model support in the i2c command dm: i2c: Add I2C emulation driver for sandbox dm: i2c: Add a sandbox I2C driver dm: i2c: Add an I2C EEPROM simulator dm: i2c: config: Enable I2C for sandbox using driver model dm: i2c: dts: Add an I2C bus for sandbox dm: Add a simple EEPROM driver dm: i2c: Add tests for I2C dm: i2c: tegra: Convert to driver model
arch/arm/cpu/tegra20-common/pmu.c | 21 +- arch/arm/dts/tegra124-jetson-tk1.dts | 1 - arch/arm/dts/tegra124-norrin.dts | 1 - arch/arm/dts/tegra30-tec-ng.dts | 4 + arch/arm/include/asm/arch-tegra/tegra_i2c.h | 2 +- arch/sandbox/dts/sandbox.dts | 17 ++ arch/sandbox/include/asm/test.h | 15 + board/avionic-design/common/tamonten-ng.c | 12 +- board/nvidia/cardhu/cardhu.c | 13 +- board/nvidia/common/board.c | 4 - board/nvidia/dalmore/dalmore.c | 21 +- board/nvidia/whistler/whistler.c | 29 +- board/toradex/apalis_t30/apalis_t30.c | 19 +- common/cmd_i2c.c | 376 +++++++++++++++++++++---- drivers/i2c/Makefile | 2 + drivers/i2c/i2c-emul-uclass.c | 14 + drivers/i2c/i2c-uclass.c | 411 ++++++++++++++++++++++++++++ drivers/i2c/sandbox_i2c.c | 142 ++++++++++ drivers/i2c/tegra_i2c.c | 363 ++++++++---------------- drivers/misc/Makefile | 4 + drivers/misc/i2c_eeprom.c | 51 ++++ drivers/misc/i2c_eeprom_emul.c | 108 ++++++++ drivers/power/tps6586x.c | 27 +- include/config_fallbacks.h | 6 + include/configs/apalis_t30.h | 3 - include/configs/beaver.h | 3 - include/configs/cardhu.h | 5 - include/configs/colibri_t30.h | 3 - include/configs/dalmore.h | 5 - include/configs/jetson-tk1.h | 5 - include/configs/norrin.h | 5 - include/configs/sandbox.h | 6 + include/configs/seaboard.h | 3 - include/configs/tec-ng.h | 5 - include/configs/tegra-common.h | 1 + include/configs/tegra114-common.h | 3 - include/configs/tegra124-common.h | 3 - include/configs/tegra20-common.h | 3 - include/configs/tegra30-common.h | 3 - include/configs/trimslice.h | 3 - include/configs/venice2.h | 5 - include/configs/whistler.h | 3 - include/dm/uclass-id.h | 4 + include/i2c.h | 348 +++++++++++++++++++++++ include/i2c_eeprom.h | 19 ++ include/tps6586x.h | 4 +- test/dm/Makefile | 1 + test/dm/i2c.c | 112 ++++++++ test/dm/test.dts | 17 ++ 49 files changed, 1805 insertions(+), 430 deletions(-) create mode 100644 arch/sandbox/include/asm/test.h create mode 100644 drivers/i2c/i2c-emul-uclass.c create mode 100644 drivers/i2c/i2c-uclass.c create mode 100644 drivers/i2c/sandbox_i2c.c create mode 100644 drivers/misc/i2c_eeprom.c create mode 100644 drivers/misc/i2c_eeprom_emul.c create mode 100644 include/i2c_eeprom.h create mode 100644 test/dm/i2c.c
This comment will touch driver model, i2c and the pmic.
I have ported the s3c24x0_i2c driver to new dm i2c api - will send after dm i2c tree merge.
I tested this on Trats2(Exynos4412) and Arndale(Exynos5250) and the second one also with the High Speed I2C on port 0.
It works fine for me. I have also rebased my pmic framework on it - which was quite backward, because of various other side tasks like this one.
Using dm i2c framework for PMIC actually forces the previous assumed pmic hierarchy, which had look like this:
- udev PMIC(parent:root, uclass: PMIC)
- udev REGULATOR(parent: PMIC, uclass REGULATOR)
then both devices can share some data parent<->child
Let's assume that we have some pmic device(more than one functionality in a package) - it uses the same address on I2C0 for both PMIC and REGULATOR drivers.
So we have one physically device and two drivers (two uclass devices):
- PMIC driver for some not standard settings by raw I/O,
- REGULATOR driver for some defined regulator ops
Single device, single fdt node, and two (or more) uclasses.
With the present dm/i2c assumptions it should looks like:
- udev PMIC(parent:I2C0, uclass: PMIC)
- udev REGULATOR(parent: I2C0, uclass REGULATOR)
This allows use I2C by the regulator and pmic if the second one is probed - i2c device will be created.
But in the device-tree we have only one device node like: i2c0@.. { pmic@.. { }; };
and for this node we can bind only the ONE driver.
The short flow for e.g.i2c0 is:
- i2c_post_bind() - called after bind i2c0 i2c uclass driver
- dm_scan_fdt_node() - scan each i2c0 subnode, e.g. mentioned "pmic"
- lists_bind_fdt() - scan U-Boot drivers linked list
- device_bind() - bind the compatible driver
- Have I2C0 bus and pmic i2c chip(bus child)
And the child_pre_probe() implementation of i2c driver, takes care of i2c chip which was bind as a I2C bus child.
Let's consider this two cases: case 1: # FDT pmic subnode without defined regulators # requires only PMIC uclass driver # situation is clear
case 2: # FDT pmic subnode with defined regulators # requires PMIC uclass driver and REGULATOR uclass driver to bind # situation is unclear
And now, where is the problem? To bind regulator - device_bind() should be called with I2C bus udevice as a parent. Doing this, we lose the PMIC <-> REGULATOR relationship, and those sub-devices can't share any data.
This quite good solution allows to acces the I2C bus also by the regulator, because the device_bind() will be called with PMIC's "of_offset" as an argument.
When bind and probe the regulator driver? In the first pmic framework patchset, there was a function pmic_init_dm - and this was for manually bind and probe the drivers - then it was working well. Now driver-model can bind the i2c devices automatically - so the pmic_init_dm() function can be limited to probe the pmic drivers only.
Testing: For my tests I solved this by manually bind REGULATOR driver in PMIC driver, with the udev I2C bus as a parent for the REGULATOR.
This is very unclear because, the true parent(PMIC) changes it's child(REGULATOR) parent field to the bus udevice which is -> I2C bus. So we have: #|_bus_parent ###|_bus_child ---- parent: #parent_bus #####|_bus_child_child ---- parent: #parent_bus (wrong!)
And now, the REGULATOR don't know that PMIC is the parent. So the things are going to be complicated.
What do you think about extend the struct udevice by the bus? E.g. : struct udevice { struct driver *driver; const char *name; void *platdata; int of_offset; const struct udevice_id *of_id; struct udevice *parent;
struct udevice *bus; void *priv;...
In such device structure, the true parent device which represents a physical device like PMIC(in this case), could bind few and different uclass devices.
To get this more simply, the mentioned PMIC which can consists of few subsystems, like:
- regulators
- charger
- mfd
- rtc
- adc, and some more
as a parent - should be able to bind a proper class driver for each functionality - add a number of childs.
The another solution is to add new subnodes with different compatibles, but it also requires some combinations - and still the parent is not a bus.
I think that separating a bus from parent<->child relationship make the driver model more flexible.
This (a separate bus link) has been discussed with respect to USB but until USB is done we won't know what the requirements is there.
How about you have:
PMIC | -- REGULATOR -- GPIO -- BATTERY
or similar. The PMIC itself should be like a multi-function device (MFD) with child devices. It is the child devices that actually implement functionality - the PMIC is just the gateway.
With respect to I2C, it feels dodgy to have child devices talking directly to the parent's bus. They should talk to the parent.
If you implemented some sort of register access mechanism in the PMIC driver, then its children could use that. Then the PMIC children don't need to know about exactly how the PMIC is accessed, just that they can read and write registers. You could implement reg_read() and reg_write() in the PMIC uclass.
Regards, Simon
Hello,
On 12/04/2014 03:00 AM, Simon Glass wrote:
Hi Przemyslaw,
On 3 December 2014 at 09:13, Przemyslaw Marczak p.marczak@samsung.com wrote:
Hello all,
On 11/24/2014 07:57 PM, Simon Glass wrote:
This series adds I2C support to driver model. It has become apparent that this is a high priority as it is widely used. It follows along to some extent from the SPI conversion.
Several changes are made from the original I2C implementations.
Firstly it is not necessary to specify the chip address with every call, since each chip knows its own address - it is stored in struct dm_i2c_chip which is attached to each chip on the I2C bus. However, this information *is* passed to the driver since I presume most drivers need it and it would be cumbersome to look up in every call.
Secondly there is no concept of a 'current' I2C bus so all associated logic is removed. With driver model i2c_set_bus_num() and i2c_get_bus_num() are not available. Since the chip device specifies both the bus and the chip address, there is no need for this concept. It also causes problems when one driver changes the current bus and forgets to change it back.
Thirdly initialisation is handled by driver model's normal probe() method on each device so there should be no need for i2c_init_all(), i2c_init(), i2c_init_board(), i2c_board_late_init() and board_i2c_init().
I2C muxes are not yet supported. To support these we will need to maintain state of the current mux settings to avoid resetting every mux every time. Probably we need to add a sandbox I2C mux driver to permit testing of this. This can probably be done later.
Platform data is not yet supported either, only device tree. The U_BOOT_I2C_MKENT_COMPLETE() and U_BOOT_I2C_ADAP_COMPLETE() macros are not used. Also struct i2c_adapter is not defined anymore. This will need to be addressed, perhaps as part of converting over a board that does not use device tree, assuming that we want to support this.
The following I2C CONFIGs are no-longer needed when driver model is used:
CONFIG_SYS_I2C_INIT_BOARD - each I2C bus is inited in its probe()method CONFIG_I2C_MULTI_BUS - we always support multi-bus with driver model CONFIG_SYS_MAX_I2C_BUS - the device tree aliases define available buses CONFIG_SYS_I2C_SPEED - the device tree specifies the speed for each bus CONFIG_SYS_I2C - this is the 'new old' API, now deprecated
There are a few SPI patches included here due to a dependency on a new device binding function.
v3 changes the uclass to driver interface significantly. It is now a list of messages to be processed by the driver. This matches the Linux user space API which may be a benefit. It does introduce one complication, which is that the protocol does not support separate chip offset and data. I have enhanced it to do so.
This series is available at u-boot-dm/i2c-working2.
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
- Adjust for slightly altered I2C uclass API
- Add new 'i2c flags' command to get/set chip flags
- Update for new uclass <=> driver interface
- Update emulator for new uclass interface
- Update for new uclass <=> emulateor interface
- Change driver to support new message-based I2C uclass API
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
- Change alen to int so that it can be -1 (this was a bug)
- Call the deblock() method for 'i2c reset'
- Update commit message for EEPROM driver
- Add a test for automatic binding of generic I2C devices
- Add a new asm/test.h header for tests in sandbox
- Adjust tegra_i2c_child_pre_probe() to permit generic I2C devices
- Correct the compatible strings for I2C buses
- Don't init if the speed is 0, since this breaks the controller
- Expand coverage to all Tegra boards
Simon Glass (10): dm: i2c: Add a uclass for I2C dm: i2c: Implement driver model support in the i2c command dm: i2c: Add I2C emulation driver for sandbox dm: i2c: Add a sandbox I2C driver dm: i2c: Add an I2C EEPROM simulator dm: i2c: config: Enable I2C for sandbox using driver model dm: i2c: dts: Add an I2C bus for sandbox dm: Add a simple EEPROM driver dm: i2c: Add tests for I2C dm: i2c: tegra: Convert to driver model
arch/arm/cpu/tegra20-common/pmu.c | 21 +- arch/arm/dts/tegra124-jetson-tk1.dts | 1 - arch/arm/dts/tegra124-norrin.dts | 1 - arch/arm/dts/tegra30-tec-ng.dts | 4 + arch/arm/include/asm/arch-tegra/tegra_i2c.h | 2 +- arch/sandbox/dts/sandbox.dts | 17 ++ arch/sandbox/include/asm/test.h | 15 + board/avionic-design/common/tamonten-ng.c | 12 +- board/nvidia/cardhu/cardhu.c | 13 +- board/nvidia/common/board.c | 4 - board/nvidia/dalmore/dalmore.c | 21 +- board/nvidia/whistler/whistler.c | 29 +- board/toradex/apalis_t30/apalis_t30.c | 19 +- common/cmd_i2c.c | 376 +++++++++++++++++++++---- drivers/i2c/Makefile | 2 + drivers/i2c/i2c-emul-uclass.c | 14 + drivers/i2c/i2c-uclass.c | 411 ++++++++++++++++++++++++++++ drivers/i2c/sandbox_i2c.c | 142 ++++++++++ drivers/i2c/tegra_i2c.c | 363 ++++++++---------------- drivers/misc/Makefile | 4 + drivers/misc/i2c_eeprom.c | 51 ++++ drivers/misc/i2c_eeprom_emul.c | 108 ++++++++ drivers/power/tps6586x.c | 27 +- include/config_fallbacks.h | 6 + include/configs/apalis_t30.h | 3 - include/configs/beaver.h | 3 - include/configs/cardhu.h | 5 - include/configs/colibri_t30.h | 3 - include/configs/dalmore.h | 5 - include/configs/jetson-tk1.h | 5 - include/configs/norrin.h | 5 - include/configs/sandbox.h | 6 + include/configs/seaboard.h | 3 - include/configs/tec-ng.h | 5 - include/configs/tegra-common.h | 1 + include/configs/tegra114-common.h | 3 - include/configs/tegra124-common.h | 3 - include/configs/tegra20-common.h | 3 - include/configs/tegra30-common.h | 3 - include/configs/trimslice.h | 3 - include/configs/venice2.h | 5 - include/configs/whistler.h | 3 - include/dm/uclass-id.h | 4 + include/i2c.h | 348 +++++++++++++++++++++++ include/i2c_eeprom.h | 19 ++ include/tps6586x.h | 4 +- test/dm/Makefile | 1 + test/dm/i2c.c | 112 ++++++++ test/dm/test.dts | 17 ++ 49 files changed, 1805 insertions(+), 430 deletions(-) create mode 100644 arch/sandbox/include/asm/test.h create mode 100644 drivers/i2c/i2c-emul-uclass.c create mode 100644 drivers/i2c/i2c-uclass.c create mode 100644 drivers/i2c/sandbox_i2c.c create mode 100644 drivers/misc/i2c_eeprom.c create mode 100644 drivers/misc/i2c_eeprom_emul.c create mode 100644 include/i2c_eeprom.h create mode 100644 test/dm/i2c.c
This comment will touch driver model, i2c and the pmic.
I have ported the s3c24x0_i2c driver to new dm i2c api - will send after dm i2c tree merge.
I tested this on Trats2(Exynos4412) and Arndale(Exynos5250) and the second one also with the High Speed I2C on port 0.
It works fine for me. I have also rebased my pmic framework on it - which was quite backward, because of various other side tasks like this one.
Using dm i2c framework for PMIC actually forces the previous assumed pmic hierarchy, which had look like this:
- udev PMIC(parent:root, uclass: PMIC)
- udev REGULATOR(parent: PMIC, uclass REGULATOR)
then both devices can share some data parent<->child
Let's assume that we have some pmic device(more than one functionality in a package) - it uses the same address on I2C0 for both PMIC and REGULATOR drivers.
So we have one physically device and two drivers (two uclass devices):
- PMIC driver for some not standard settings by raw I/O,
- REGULATOR driver for some defined regulator ops
Single device, single fdt node, and two (or more) uclasses.
With the present dm/i2c assumptions it should looks like:
- udev PMIC(parent:I2C0, uclass: PMIC)
- udev REGULATOR(parent: I2C0, uclass REGULATOR)
This allows use I2C by the regulator and pmic if the second one is probed - i2c device will be created.
But in the device-tree we have only one device node like: i2c0@.. { pmic@.. { }; };
and for this node we can bind only the ONE driver.
The short flow for e.g.i2c0 is:
- i2c_post_bind() - called after bind i2c0 i2c uclass driver
- dm_scan_fdt_node() - scan each i2c0 subnode, e.g. mentioned "pmic"
- lists_bind_fdt() - scan U-Boot drivers linked list
- device_bind() - bind the compatible driver
- Have I2C0 bus and pmic i2c chip(bus child)
And the child_pre_probe() implementation of i2c driver, takes care of i2c chip which was bind as a I2C bus child.
Let's consider this two cases: case 1: # FDT pmic subnode without defined regulators # requires only PMIC uclass driver # situation is clear
case 2: # FDT pmic subnode with defined regulators # requires PMIC uclass driver and REGULATOR uclass driver to bind # situation is unclear
And now, where is the problem? To bind regulator - device_bind() should be called with I2C bus udevice as a parent. Doing this, we lose the PMIC <-> REGULATOR relationship, and those sub-devices can't share any data.
This quite good solution allows to acces the I2C bus also by the regulator, because the device_bind() will be called with PMIC's "of_offset" as an argument.
When bind and probe the regulator driver? In the first pmic framework patchset, there was a function pmic_init_dm - and this was for manually bind and probe the drivers - then it was working well. Now driver-model can bind the i2c devices automatically - so the pmic_init_dm() function can be limited to probe the pmic drivers only.
Testing: For my tests I solved this by manually bind REGULATOR driver in PMIC driver, with the udev I2C bus as a parent for the REGULATOR.
This is very unclear because, the true parent(PMIC) changes it's child(REGULATOR) parent field to the bus udevice which is -> I2C bus. So we have: #|_bus_parent ###|_bus_child ---- parent: #parent_bus #####|_bus_child_child ---- parent: #parent_bus (wrong!)
And now, the REGULATOR don't know that PMIC is the parent. So the things are going to be complicated.
What do you think about extend the struct udevice by the bus? E.g. : struct udevice { struct driver *driver; const char *name; void *platdata; int of_offset; const struct udevice_id *of_id; struct udevice *parent;
struct udevice *bus; void *priv;...
In such device structure, the true parent device which represents a physical device like PMIC(in this case), could bind few and different uclass devices.
To get this more simply, the mentioned PMIC which can consists of few subsystems, like:
- regulators
- charger
- mfd
- rtc
- adc, and some more
as a parent - should be able to bind a proper class driver for each functionality - add a number of childs.
The another solution is to add new subnodes with different compatibles, but it also requires some combinations - and still the parent is not a bus.
I think that separating a bus from parent<->child relationship make the driver model more flexible.
This (a separate bus link) has been discussed with respect to USB but until USB is done we won't know what the requirements is there.
How about you have:
PMIC | -- REGULATOR -- GPIO -- BATTERY
or similar. The PMIC itself should be like a multi-function device (MFD) with child devices. It is the child devices that actually implement functionality - the PMIC is just the gateway.
Yes, before the dm i2c it was something like that, the pmic platdata was shared - and each device could do r/w operations thanks to that. But now it looks like the pmic platdata (actually the previous struct pmic), can be removed from the new framework, because all required data are provided by spi/i2c devices.
With respect to I2C, it feels dodgy to have child devices talking directly to the parent's bus. They should talk to the parent.
Yes, that's truth.
If you implemented some sort of register access mechanism in the PMIC driver, then its children could use that. Then the PMIC children don't need to know about exactly how the PMIC is accessed, just that they can read and write registers. You could implement reg_read() and reg_write() in the PMIC uclass.
Regards, Simon
So in this case, the pmic_read/write could use dev->parent as i2c device(the pmic gateway) and do the check: # pmic_read(dev, reg) { # struct udevice *i2c_dev; # # if (dev->parent != root) # i2c_dev = dev->parent; # else # i2c_dev = dev; # }
Or the second case, the pmic_childs will pass the parent as a device, with just determine the proper register.
I have some other work to do now. And I will back to this probably on Thursday.
Best regards,
Hi Prazemyslaw,
On 5 December 2014 at 06:09, Przemyslaw Marczak p.marczak@samsung.com wrote:
Hello,
On 12/04/2014 03:00 AM, Simon Glass wrote:
Hi Przemyslaw,
On 3 December 2014 at 09:13, Przemyslaw Marczak p.marczak@samsung.com wrote:
Hello all,
On 11/24/2014 07:57 PM, Simon Glass wrote:
This series adds I2C support to driver model. It has become apparent that this is a high priority as it is widely used. It follows along to some extent from the SPI conversion.
Several changes are made from the original I2C implementations.
Firstly it is not necessary to specify the chip address with every call, since each chip knows its own address - it is stored in struct dm_i2c_chip which is attached to each chip on the I2C bus. However, this information *is* passed to the driver since I presume most drivers need it and it would be cumbersome to look up in every call.
Secondly there is no concept of a 'current' I2C bus so all associated logic is removed. With driver model i2c_set_bus_num() and i2c_get_bus_num() are not available. Since the chip device specifies both the bus and the chip address, there is no need for this concept. It also causes problems when one driver changes the current bus and forgets to change it back.
Thirdly initialisation is handled by driver model's normal probe() method on each device so there should be no need for i2c_init_all(), i2c_init(), i2c_init_board(), i2c_board_late_init() and board_i2c_init().
I2C muxes are not yet supported. To support these we will need to maintain state of the current mux settings to avoid resetting every mux every time. Probably we need to add a sandbox I2C mux driver to permit testing of this. This can probably be done later.
Platform data is not yet supported either, only device tree. The U_BOOT_I2C_MKENT_COMPLETE() and U_BOOT_I2C_ADAP_COMPLETE() macros are not used. Also struct i2c_adapter is not defined anymore. This will need to be addressed, perhaps as part of converting over a board that does not use device tree, assuming that we want to support this.
The following I2C CONFIGs are no-longer needed when driver model is used:
CONFIG_SYS_I2C_INIT_BOARD - each I2C bus is inited in its probe()method CONFIG_I2C_MULTI_BUS - we always support multi-bus with driver model CONFIG_SYS_MAX_I2C_BUS - the device tree aliases define available buses CONFIG_SYS_I2C_SPEED - the device tree specifies the speed for each bus CONFIG_SYS_I2C - this is the 'new old' API, now deprecated
There are a few SPI patches included here due to a dependency on a new device binding function.
v3 changes the uclass to driver interface significantly. It is now a list of messages to be processed by the driver. This matches the Linux user space API which may be a benefit. It does introduce one complication, which is that the protocol does not support separate chip offset and data. I have enhanced it to do so.
This series is available at u-boot-dm/i2c-working2.
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
- Adjust for slightly altered I2C uclass API
- Add new 'i2c flags' command to get/set chip flags
- Update for new uclass <=> driver interface
- Update emulator for new uclass interface
- Update for new uclass <=> emulateor interface
- Change driver to support new message-based I2C uclass API
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
- Change alen to int so that it can be -1 (this was a bug)
- Call the deblock() method for 'i2c reset'
- Update commit message for EEPROM driver
- Add a test for automatic binding of generic I2C devices
- Add a new asm/test.h header for tests in sandbox
- Adjust tegra_i2c_child_pre_probe() to permit generic I2C devices
- Correct the compatible strings for I2C buses
- Don't init if the speed is 0, since this breaks the controller
- Expand coverage to all Tegra boards
Simon Glass (10): dm: i2c: Add a uclass for I2C dm: i2c: Implement driver model support in the i2c command dm: i2c: Add I2C emulation driver for sandbox dm: i2c: Add a sandbox I2C driver dm: i2c: Add an I2C EEPROM simulator dm: i2c: config: Enable I2C for sandbox using driver model dm: i2c: dts: Add an I2C bus for sandbox dm: Add a simple EEPROM driver dm: i2c: Add tests for I2C dm: i2c: tegra: Convert to driver model
arch/arm/cpu/tegra20-common/pmu.c | 21 +- arch/arm/dts/tegra124-jetson-tk1.dts | 1 - arch/arm/dts/tegra124-norrin.dts | 1 - arch/arm/dts/tegra30-tec-ng.dts | 4 + arch/arm/include/asm/arch-tegra/tegra_i2c.h | 2 +- arch/sandbox/dts/sandbox.dts | 17 ++ arch/sandbox/include/asm/test.h | 15 + board/avionic-design/common/tamonten-ng.c | 12 +- board/nvidia/cardhu/cardhu.c | 13 +- board/nvidia/common/board.c | 4 - board/nvidia/dalmore/dalmore.c | 21 +- board/nvidia/whistler/whistler.c | 29 +- board/toradex/apalis_t30/apalis_t30.c | 19 +- common/cmd_i2c.c | 376 +++++++++++++++++++++---- drivers/i2c/Makefile | 2 + drivers/i2c/i2c-emul-uclass.c | 14 + drivers/i2c/i2c-uclass.c | 411 ++++++++++++++++++++++++++++ drivers/i2c/sandbox_i2c.c | 142 ++++++++++ drivers/i2c/tegra_i2c.c | 363 ++++++++---------------- drivers/misc/Makefile | 4 + drivers/misc/i2c_eeprom.c | 51 ++++ drivers/misc/i2c_eeprom_emul.c | 108 ++++++++ drivers/power/tps6586x.c | 27 +- include/config_fallbacks.h | 6 + include/configs/apalis_t30.h | 3 - include/configs/beaver.h | 3 - include/configs/cardhu.h | 5 - include/configs/colibri_t30.h | 3 - include/configs/dalmore.h | 5 - include/configs/jetson-tk1.h | 5 - include/configs/norrin.h | 5 - include/configs/sandbox.h | 6 + include/configs/seaboard.h | 3 - include/configs/tec-ng.h | 5 - include/configs/tegra-common.h | 1 + include/configs/tegra114-common.h | 3 - include/configs/tegra124-common.h | 3 - include/configs/tegra20-common.h | 3 - include/configs/tegra30-common.h | 3 - include/configs/trimslice.h | 3 - include/configs/venice2.h | 5 - include/configs/whistler.h | 3 - include/dm/uclass-id.h | 4 + include/i2c.h | 348 +++++++++++++++++++++++ include/i2c_eeprom.h | 19 ++ include/tps6586x.h | 4 +- test/dm/Makefile | 1 + test/dm/i2c.c | 112 ++++++++ test/dm/test.dts | 17 ++ 49 files changed, 1805 insertions(+), 430 deletions(-) create mode 100644 arch/sandbox/include/asm/test.h create mode 100644 drivers/i2c/i2c-emul-uclass.c create mode 100644 drivers/i2c/i2c-uclass.c create mode 100644 drivers/i2c/sandbox_i2c.c create mode 100644 drivers/misc/i2c_eeprom.c create mode 100644 drivers/misc/i2c_eeprom_emul.c create mode 100644 include/i2c_eeprom.h create mode 100644 test/dm/i2c.c
This comment will touch driver model, i2c and the pmic.
I have ported the s3c24x0_i2c driver to new dm i2c api - will send after dm i2c tree merge.
I tested this on Trats2(Exynos4412) and Arndale(Exynos5250) and the second one also with the High Speed I2C on port 0.
It works fine for me. I have also rebased my pmic framework on it - which was quite backward, because of various other side tasks like this one.
Using dm i2c framework for PMIC actually forces the previous assumed pmic hierarchy, which had look like this:
- udev PMIC(parent:root, uclass: PMIC)
- udev REGULATOR(parent: PMIC, uclass REGULATOR)
then both devices can share some data parent<->child
Let's assume that we have some pmic device(more than one functionality in a package) - it uses the same address on I2C0 for both PMIC and REGULATOR drivers.
So we have one physically device and two drivers (two uclass devices):
- PMIC driver for some not standard settings by raw I/O,
- REGULATOR driver for some defined regulator ops
Single device, single fdt node, and two (or more) uclasses.
With the present dm/i2c assumptions it should looks like:
- udev PMIC(parent:I2C0, uclass: PMIC)
- udev REGULATOR(parent: I2C0, uclass REGULATOR)
This allows use I2C by the regulator and pmic if the second one is probed
i2c device will be created.
But in the device-tree we have only one device node like: i2c0@.. { pmic@.. { }; };
and for this node we can bind only the ONE driver.
The short flow for e.g.i2c0 is:
- i2c_post_bind() - called after bind i2c0 i2c uclass driver
- dm_scan_fdt_node() - scan each i2c0 subnode, e.g. mentioned "pmic"
- lists_bind_fdt() - scan U-Boot drivers linked list
- device_bind() - bind the compatible driver
- Have I2C0 bus and pmic i2c chip(bus child)
And the child_pre_probe() implementation of i2c driver, takes care of i2c chip which was bind as a I2C bus child.
Let's consider this two cases: case 1: # FDT pmic subnode without defined regulators # requires only PMIC uclass driver # situation is clear
case 2: # FDT pmic subnode with defined regulators # requires PMIC uclass driver and REGULATOR uclass driver to bind # situation is unclear
And now, where is the problem? To bind regulator - device_bind() should be called with I2C bus udevice as a parent. Doing this, we lose the PMIC <-> REGULATOR relationship, and those sub-devices can't share any data.
This quite good solution allows to acces the I2C bus also by the regulator, because the device_bind() will be called with PMIC's "of_offset" as an argument.
When bind and probe the regulator driver? In the first pmic framework patchset, there was a function pmic_init_dm - and this was for manually bind and probe the drivers - then it was working well. Now driver-model can bind the i2c devices automatically - so the pmic_init_dm() function can be limited to probe the pmic drivers only.
Testing: For my tests I solved this by manually bind REGULATOR driver in PMIC driver, with the udev I2C bus as a parent for the REGULATOR.
This is very unclear because, the true parent(PMIC) changes it's child(REGULATOR) parent field to the bus udevice which is -> I2C bus. So we have: #|_bus_parent ###|_bus_child ---- parent: #parent_bus #####|_bus_child_child ---- parent: #parent_bus (wrong!)
And now, the REGULATOR don't know that PMIC is the parent. So the things are going to be complicated.
What do you think about extend the struct udevice by the bus? E.g. : struct udevice { struct driver *driver; const char *name; void *platdata; int of_offset; const struct udevice_id *of_id; struct udevice *parent;
struct udevice *bus; void *priv;...
In such device structure, the true parent device which represents a physical device like PMIC(in this case), could bind few and different uclass devices.
To get this more simply, the mentioned PMIC which can consists of few subsystems, like:
- regulators
- charger
- mfd
- rtc
- adc, and some more
as a parent - should be able to bind a proper class driver for each functionality - add a number of childs.
The another solution is to add new subnodes with different compatibles, but it also requires some combinations - and still the parent is not a bus.
I think that separating a bus from parent<->child relationship make the driver model more flexible.
This (a separate bus link) has been discussed with respect to USB but until USB is done we won't know what the requirements is there.
How about you have:
PMIC | -- REGULATOR -- GPIO -- BATTERY
or similar. The PMIC itself should be like a multi-function device (MFD) with child devices. It is the child devices that actually implement functionality - the PMIC is just the gateway.
Yes, before the dm i2c it was something like that, the pmic platdata was shared - and each device could do r/w operations thanks to that. But now it looks like the pmic platdata (actually the previous struct pmic), can be removed from the new framework, because all required data are provided by spi/i2c devices.
Sounds plausible.
With respect to I2C, it feels dodgy to have child devices talking directly to the parent's bus. They should talk to the parent.
Yes, that's truth.
If you implemented some sort of register access mechanism in the PMIC driver, then its children could use that. Then the PMIC children don't need to know about exactly how the PMIC is accessed, just that they can read and write registers. You could implement reg_read() and reg_write() in the PMIC uclass.
Regards, Simon
So in this case, the pmic_read/write could use dev->parent as i2c device(the pmic gateway) and do the check: # pmic_read(dev, reg) { # struct udevice *i2c_dev; # # if (dev->parent != root) # i2c_dev = dev->parent; # else # i2c_dev = dev; # }
I don't like that very much. It would be better to require a PMIC device to be passed in, then have children use dev_get_parent() to find it.
Or the second case, the pmic_childs will pass the parent as a device, with just determine the proper register.
Yes
I have some other work to do now. And I will back to this probably on Thursday.
OK sounds good.
Regards, Simon
participants (5)
-
Heiko Schocher -
Masahiro YAMADA -
Masahiro Yamada
-
Przemyslaw Marczak -
Simon Glass