Hello Simon, On 03/28/2015 04:08 PM, Simon Glass wrote:
Hi Przemyslaw,On 27 March 2015 at 11:33, Przemyslaw Marczak <p.marc...@samsung.com> wrote:This commit adds driver model support to software emulated i2c bus driver. The device-tree node is compatible with the kernel i2c-gpio driver. It means, that boards device-tree "i2c-gpio" node, should be the same as in the kernel. For operating, it requires proper compatible and gpio pins dts description. Added: - Config: CONFIG_DM_I2C_GPIO - File: drivers/i2c/i2c-gpio.c - File: doc/device-tree-bindings/i2c/i2c-gpio.txt Driver base code is taken from: drivers/i2c/soft-i2c.c, changes: - update comments style, - move preprocesor macros into functions, - add device-tree support, - add driver model i2c support. - add Kconfig entry Signed-off-by: Przemyslaw Marczak <p.marc...@samsung.com> CC: Masahiro Yamada <yamada.masah...@socionext.com> Cc: Lukasz Majewski <l.majew...@samsung.com> Cc: Mike Frysinger <vap...@gentoo.org> Cc: Simon Glass <s...@chromium.org> Cc: Heiko Schocher <h...@denx.de> Changes V2: - new file for software i2c driver: i2c-gpio.c - update driver naming: use of "i2c-gpio" - add full compatibility with the kernels device-tree "i2c-gpio" node - fix Kconfig entrySorry I still have a few more comments.
OK, this is the purpose of this list:)
--- doc/device-tree-bindings/i2c/i2c-gpio.txt | 37 ++++ drivers/i2c/Kconfig | 9 + drivers/i2c/Makefile | 1 + drivers/i2c/i2c-gpio.c | 353 ++++++++++++++++++++++++++++++ 4 files changed, 400 insertions(+) create mode 100644 doc/device-tree-bindings/i2c/i2c-gpio.txt create mode 100644 drivers/i2c/i2c-gpio.c diff --git a/doc/device-tree-bindings/i2c/i2c-gpio.txt b/doc/device-tree-bindings/i2c/i2c-gpio.txt new file mode 100644 index 0000000..3978381 --- /dev/null +++ b/doc/device-tree-bindings/i2c/i2c-gpio.txt @@ -0,0 +1,37 @@ +I2C gpio device binding +======================= + +Driver: +- drivers/i2c/i2c-gpio.c + +Software i2c device-tree node properties: +Required: +* #address-cells = <1>; +* #size-cells = <0>; +* compatible = "i2c-gpio"; +* gpios = <sda ...>, <scl ...>; + +Optional: +* i2c-gpio,delay-us = <5>; + The resulting transfer speed can be adjusted by setting the delay[us] + between gpio-toggle operations. Speed [Hz] = 1us / 4 * udelay, + It not defined, then default is 5us (~50KHz). + +Example: + +i2c-gpio@1 { + #address-cells = <1>; + #size-cells = <0>; + + compatible = "i2c-gpio"; + gpios = <&gpd1 0 GPIO_ACTIVE_HIGH>, /* SDA */ + <&gpd1 1 GPIO_ACTIVE_HIGH>; /* CLK */ + + i2c-gpio,delay-us = <5>; + + some_device@5 { + compatible = "some_device"; + reg = <0x5>; + ... + }; +}; diff --git a/drivers/i2c/Kconfig b/drivers/i2c/Kconfig index 979522f..22e4a7c 100644 --- a/drivers/i2c/Kconfig +++ b/drivers/i2c/Kconfig @@ -19,6 +19,15 @@ config DM_I2C_COMPAT to convert all code for a board in a single commit. It should not be enabled for any board in an official release. +config DM_I2C_GPIO + bool "Enable Driver Model for software emulated I2C bus driver" + depends on DM_I2C && DM_GPIO + help + Enable the i2c bus driver emulation by using the GPIO.by using GPIOs.+ The bus gpio configuration is given by the device-tree.GPIO device tree (no hypen)+ Kernel style device-tree node for i2c-gpio is supported.Kernel-style device tree bindings are supported
Thanks, will fix the all above.
+ Binding info: doc/device-tree-bindings/i2c/i2c-gpio.txt + config SYS_I2C_UNIPHIER bool "UniPhier I2C driver" depends on ARCH_UNIPHIER && DM_I2C diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile index 774bc94..d9e9f3a 100644 --- a/drivers/i2c/Makefile +++ b/drivers/i2c/Makefile @@ -6,6 +6,7 @@ # obj-$(CONFIG_DM_I2C) += i2c-uclass.o obj-$(CONFIG_DM_I2C_COMPAT) += i2c-uclass-compat.o +obj-$(CONFIG_DM_I2C_GPIO) += i2c-gpio.o obj-$(CONFIG_SYS_I2C_ADI) += adi_i2c.o obj-$(CONFIG_I2C_MV) += mv_i2c.o diff --git a/drivers/i2c/i2c-gpio.c b/drivers/i2c/i2c-gpio.c new file mode 100644 index 0000000..8e9ed6b --- /dev/null +++ b/drivers/i2c/i2c-gpio.c @@ -0,0 +1,353 @@ +/* + * (C) Copyright 2015, Samsung Electronics + * Przemyslaw Marczak <p.marc...@samsung.com> + * Add driver-model support as a separated driver file + * + * (C) Copyright 2009 + * Heiko Schocher, DENX Software Engineering, h...@denx.de. + * Changes for multibus/multiadapter I2C support. + * + * (C) Copyright 2001, 2002 + * Wolfgang Denk, DENX Software Engineering, w...@denx.de. + * + * SPDX-License-Identifier: GPL-2.0+ + * + * This has been changed substantially by Gerald Van Baren, Custom IDEAS, + * vanba...@cideas.com. It was heavily influenced by LiMon, written by + * Neil Russell. + */ +#include <common.h> +#include <errno.h> +#include <dm.h> +#include <i2c.h> +#include <div64.h> +#include <asm/gpio.h> + +#define DEFAULT_UDELAY 5 +#define RETRIES 0 +#define I2C_ACK 0 +#define I2C_NOACK 1 + +#ifdef DEBUG_I2C +#define PRINTD(fmt, args...) do { \ + printf (fmt, ##args); \ + } while (0) +#else +#define PRINTD(fmt, args...) +#endifI don't see any point in this - how about just using debug() instead?
Right, will change to debug().
+ +DECLARE_GLOBAL_DATA_PTR; + +enum { + PIN_SDA = 0, + PIN_SCL,PIN_COUNT
Ok
+}; +Document these members - speed is in HzHz, udelay is the delay for what?
The delay was described in the binding file, I will also add the description beside the structure. And I will remove the speed, since uclass keeps an info about this.
+struct i2c_gpio_bus { + unsigned int speed; + unsigned long udelay; + struct gpio_desc gpios[2]; /* sda, scl */gpios[PIN_COUNT]
Ok.
+}; + +/* Local functions */ +static void send_reset(struct gpio_desc *, struct gpio_desc *, int); +static void send_start(struct gpio_desc *, struct gpio_desc *, int); +static void send_stop(struct gpio_desc *, struct gpio_desc *, int); +static void send_ack(struct gpio_desc *, struct gpio_desc *, int, int); +static int write_byte(struct gpio_desc *, struct gpio_desc *, int, uchar); +static uchar read_byte(struct gpio_desc *, struct gpio_desc *, int, int);If you move send_reset() down a bit can you drop these declarations?+ +static int I2C_READ(struct gpio_desc *sda)Lower case - how about gpio_i2c_read()? Same for others.
Ok, will clean this functions.
+{ + return dm_gpio_get_value(sda); +} + +static void I2C_SDA(struct gpio_desc *sda, int bit) +{ + if (bit) { + dm_gpio_set_dir_flags(sda, GPIOD_IS_IN);I assume the polarity is never set, so this should be OK.
Yes, this works fine.
+ } else { + dm_gpio_set_dir_flags(sda, GPIOD_IS_OUT); + dm_gpio_set_value(sda, 0); + } +} + +static void I2C_SCL(struct gpio_desc *scl, int bit) +{ + dm_gpio_set_dir_flags(scl, GPIOD_IS_OUT); + dm_gpio_set_value(scl, bit); +} + +static void I2C_DELAY(unsigned long us) +{ + udelay(us); /* 1/4 I2C clock duration */ +} + +/** + * Send a reset sequence consisting of 9 clocks with the data signal high + * to clock any confused device back into an idle state. Also send a + * <stop> at the end of the sequence for belts & suspenders. + */ +static void send_reset(struct gpio_desc *scl, struct gpio_desc *sda, int delay) +{ + int j; + + I2C_SCL(scl, 1); + I2C_SDA(sda, 1); + + for (j = 0; j < 9; j++) { + I2C_SCL(scl, 0); + I2C_DELAY(delay); + I2C_DELAY(delay); + I2C_SCL(scl, 1); + I2C_DELAY(delay); + I2C_DELAY(delay);I wonder why we don't do one call with delay * 2?
Right.
+ } + send_stop(scl, sda, delay); +} + +/* START: High -> Low on SDA while SCL is High */ +static void send_start(struct gpio_desc *scl, struct gpio_desc *sda, int delay) +{ + I2C_DELAY(delay); + I2C_SDA(sda, 1); + I2C_DELAY(delay); + I2C_SCL(scl, 1); + I2C_DELAY(delay); + I2C_SDA(sda, 0); + I2C_DELAY(delay); +} + +/* STOP: Low -> High on SDA while SCL is High */ +static void send_stop(struct gpio_desc *scl, struct gpio_desc *sda, int delay) +{ + I2C_SCL(scl, 0); + I2C_DELAY(delay); + I2C_SDA(sda, 0); + I2C_DELAY(delay); + I2C_SCL(scl, 1); + I2C_DELAY(delay); + I2C_SDA(sda, 1); + I2C_DELAY(delay); +} + +/* ack should be I2C_ACK or I2C_NOACK */ +static void send_ack(struct gpio_desc *scl, struct gpio_desc *sda, + int delay, int ack) +{ + I2C_SCL(scl, 0); + I2C_DELAY(delay); + I2C_SDA(sda, ack); + I2C_DELAY(delay); + I2C_SCL(scl, 1); + I2C_DELAY(delay); + I2C_DELAY(delay); + I2C_SCL(scl, 0); + I2C_DELAY(delay); +} + +/* Send 8 bits and look for an acknowledgement */ +static int write_byte(struct gpio_desc *scl, struct gpio_desc *sda, + int delay, uchar data) +{ + int j; + int nack; + + for (j = 0; j < 8; j++) { + I2C_SCL(scl, 0); + I2C_DELAY(delay); + I2C_SDA(sda, data & 0x80); + I2C_DELAY(delay); + I2C_SCL(scl, 1); + I2C_DELAY(delay); + I2C_DELAY(delay);This sequence of 7 calls appears a lot in this code. Could it go in a function and be called from various places?
Ok, I will clean this.
+ + data <<= 1; + } + + /* Look for an <ACK>(negative logic) and return it */ + I2C_SCL(scl, 0); + I2C_DELAY(delay); + I2C_SDA(sda, 1); + I2C_DELAY(delay); + I2C_SCL(scl, 1); + I2C_DELAY(delay); + I2C_DELAY(delay); + nack = I2C_READ(sda); + I2C_SCL(scl, 0); + I2C_DELAY(delay); + + return nack; /* not a nack is an ack */ +} + +/** + * if ack == I2C_ACK, ACK the byte so can continue reading, else + * send I2C_NOACK to end the read. + */ +static uchar read_byte(struct gpio_desc *scl, struct gpio_desc *sda, + int delay, int ack) +{ + int data; + int j; + + /* Read 8 bits, MSB first */ + I2C_SDA(sda, 1); + data = 0; + for (j = 0; j < 8; j++) { + I2C_SCL(scl, 0); + I2C_DELAY(delay); + I2C_SCL(scl, 1); + I2C_DELAY(delay); + data <<= 1; + data |= I2C_READ(sda); + I2C_DELAY(delay); + } + send_ack(scl, sda, delay, ack); + + return data; +} + +static int i2c_write_data(struct i2c_gpio_bus *bus, uchar chip, + uchar *buffer, int len, bool end_with_repeated_start) +{ + struct gpio_desc *scl = &bus->gpios[PIN_SCL]; + struct gpio_desc *sda = &bus->gpios[PIN_SDA]; + unsigned int delay = bus->udelay; + int failures = 0; + + PRINTD("%s: chip %x buffer %p len %d\n", __func__, chip, buffer, len); + + send_start(scl, sda, delay); + if (write_byte(scl, sda, delay, chip << 1)) { + send_stop(scl, sda, delay); + PRINTD("i2c_write, no chip responded %02X\n", chip); + return -EIO; + } + + while (len-- > 0) { + if (write_byte(scl, sda, delay, *buffer++)) + failures++; + } + + send_stop(scl, sda, delay); + + return failures; +} + +static int i2c_read_data(struct i2c_gpio_bus *bus, uchar chip, + uchar *buffer, int len) +{ + struct gpio_desc *scl = &bus->gpios[PIN_SCL]; + struct gpio_desc *sda = &bus->gpios[PIN_SDA]; + unsigned int delay = bus->udelay; + + PRINTD("%s: chip %x buffer: %x len %d\n", __func__, chip, buffer, len); + + send_start(scl, sda, delay); + write_byte(scl, sda, delay, (chip << 1) | 1); /* read cycle */ + + while (len-- > 0) + *buffer++ = read_byte(scl, sda, delay, len == 0); + + send_stop(scl, sda, delay); + + return 0; +} + +static int i2c_gpio_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs) +{ + struct i2c_gpio_bus *bus = dev_get_priv(dev); + int ret; + + for (; nmsgs > 0; nmsgs--, msg++) { + bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD); + if (msg->flags & I2C_M_RD) + ret = i2c_read_data(bus, msg->addr, msg->buf, msg->len); + else + ret = i2c_write_data(bus, msg->addr, msg->buf, msg->len, + next_is_read); + + if (ret) + return -EREMOTEIO; + } + + return 0; +} + +static int i2c_gpio_probe(struct udevice *dev, uint chip, uint chip_flags) +{ + struct i2c_gpio_bus *bus = dev_get_priv(dev); + struct gpio_desc *scl = &bus->gpios[PIN_SCL]; + struct gpio_desc *sda = &bus->gpios[PIN_SDA]; + unsigned int delay = bus->udelay; + int ret; + + send_start(scl, sda, delay); + ret = write_byte(scl, sda, delay, (chip << 1) | 0); + send_stop(scl, sda, delay); + + PRINTD("%s: bus: %d (%s) chip: %x flags: %x ret: %d\n", + __func__, dev->seq, dev->name, chip, chip_flags, ret); + + return ret; +} + +static int i2c_gpio_set_bus_speed(struct udevice *dev, unsigned int speed) +{ + struct i2c_gpio_bus *bus = dev_get_priv(dev); + struct gpio_desc *scl = &bus->gpios[PIN_SCL]; + struct gpio_desc *sda = &bus->gpios[PIN_SDA]; + + bus->speed = speed; + bus->udelay = lldiv(1000000, speed << 2);Why lldiv() if the value can never be >100,000? Can we just use normal division?
ok, will change this. And I will remove the speed, since it's the same value as uclass provides in struct dm_i2c_bus.
+ + send_reset(scl, sda, bus->udelay); + + PRINTD("%s: bus: %d (%s) speed: %u Hz (1/4 of period: %lu us)\n", + __func__, dev->seq, dev->name, speed, bus->udelay); + + return 0; +} + +static int i2c_gpio_ofdata_to_platdata(struct udevice *dev) +{ + struct i2c_gpio_bus *bus = dev_get_priv(dev); + const void *blob = gd->fdt_blob; + int node = dev->of_offset; + int ret; + + ret = gpio_request_list_by_name(dev, "gpios", bus->gpios, + ARRAY_SIZE(bus->gpios), 0); + if (ret < 0) + goto error; + + bus->udelay = fdtdec_get_int(blob, node, "i2c-gpio,delay-us", + DEFAULT_UDELAY); + + PRINTD("%s: bus: %d (%s) fdt node ok\n", __func__, dev->seq, dev->name); + + return 0; +error: + error("Can't get %s gpios! Error: %d", dev->name, ret); + return ret; +} + +static const struct dm_i2c_ops i2c_gpio_ops = { + .xfer = i2c_gpio_xfer, + .probe_chip = i2c_gpio_probe, + .set_bus_speed = i2c_gpio_set_bus_speed, +}; + +static const struct udevice_id i2c_gpio_ids[] = { + { .compatible = "i2c-gpio" }, + { } +}; + +U_BOOT_DRIVER(i2c_gpio) = { + .name = "i2c-gpio", + .id = UCLASS_I2C, + .of_match = i2c_gpio_ids, + .ofdata_to_platdata = i2c_gpio_ofdata_to_platdata, + .priv_auto_alloc_size = sizeof(struct i2c_gpio_bus), + .ops = &i2c_gpio_ops, +}; -- 1.9.1Regards, Simon
Thank you for the review. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marc...@samsung.com _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
