From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 31 Mar 2015 17:58:00 +0200 Subject: [U-Boot] [PATCH V2 3/3] dm: i2c: add i2c-gpio driver In-Reply-To: References: <1425983444-18565-1-git-send-email-p.marczak@samsung.com> <1427477624-23127-1-git-send-email-p.marczak@samsung.com> <1427477624-23127-4-git-send-email-p.marczak@samsung.com> Message-ID: <551AC408.3020700@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, On 03/28/2015 04:08 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 27 March 2015 at 11:33, Przemyslaw Marczak 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 >> CC: Masahiro Yamada >> Cc: Lukasz Majewski >> Cc: Mike Frysinger >> Cc: Simon Glass >> Cc: Heiko Schocher >> >> 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 entry > > Sorry 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 = , ; >> + >> +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 at 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 at 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 >> + * Add driver-model support as a separated driver file >> + * >> + * (C) Copyright 2009 >> + * Heiko Schocher, DENX Software Engineering, hs at denx.de. >> + * Changes for multibus/multiadapter I2C support. >> + * >> + * (C) Copyright 2001, 2002 >> + * Wolfgang Denk, DENX Software Engineering, wd at denx.de. >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + * >> + * This has been changed substantially by Gerald Van Baren, Custom IDEAS, >> + * vanbaren at cideas.com. It was heavily influenced by LiMon, written by >> + * Neil Russell. >> + */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#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...) >> +#endif > > I 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 >> + * 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 (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.1 >> > > Regards, > Simon > Thank you for the review. Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com