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@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_GPIOhelpEnable 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@denx.de.
- Changes for multibus/multiadapter I2C support.
- (C) Copyright 2001, 2002
- Wolfgang Denk, DENX Software Engineering, wd@denx.de.
- SPDX-License-Identifier: GPL-2.0+
- This has been changed substantially by Gerald Van Baren, Custom IDEAS,
- vanbaren@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);elseret = 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,