[U-Boot] [PATCH V2 3/3] dm: i2c: add i2c-gpio driver

[Przemyslaw Marczak <p.marczak@samsung.com>]

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@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@samsung.com>
>> CC: Masahiro Yamada <yamada.masahiro@socionext.com>
>> Cc: Lukasz Majewski <l.majewski@samsung.com>
>> Cc: Mike Frysinger <vapier@gentoo.org>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Heiko Schocher <hs@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@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