[U-Boot] [PATCH V2 3/3] dm: i2c: add i2c-gpio driver
Przemyslaw Marczak
p.marczak at samsung.com
Tue Mar 31 17:58:00 CEST 2015
Hello Simon,
On 03/28/2015 04:08 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 27 March 2015 at 11:33, Przemyslaw Marczak <p.marczak at 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.marczak at samsung.com>
>> CC: Masahiro Yamada <yamada.masahiro at socionext.com>
>> Cc: Lukasz Majewski <l.majewski at samsung.com>
>> Cc: Mike Frysinger <vapier at gentoo.org>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Heiko Schocher <hs at 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 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 = <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 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 <p.marczak at samsung.com>
>> + * 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 <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...)
>> +#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
>> + * <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.1
>>
>
> Regards,
> Simon
>
Thank you for the review.
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
More information about the U-Boot
mailing list