Hello Balbi,

On 28 December 2012 04:27, Felipe Balbi [off-list ref] wrote:

Hi,

On Tue, Dec 25, 2012 at 04:55:54PM +0530, Naveen Krishna Chatradhi wrote:

quoted

Adds support for High Speed I2C driver found in Exynos5 and later
SoCs from Samsung. This driver currently supports Auto mode.

Driver only supports Device Tree method.

Signed-off-by: Taekgyun Ko <redacted>
Signed-off-by: Naveen Krishna Chatradhi <redacted>
---
Changes since v1:
Fixed the comments from Felipe Balbi and Thomas Abraham.

 drivers/i2c/busses/Kconfig       |    7 +
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-exynos5.c |  652 ++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-exynos5.h |  102 ++++++
 4 files changed, 762 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-exynos5.c
 create mode 100644 drivers/i2c/busses/i2c-exynos5.h

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index bdca511..4caea76 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig

@@ -618,6 +618,13 @@ config I2C_S3C2410
        Say Y here to include support for I2C controller in the
        Samsung SoCs.

+config I2C_EXYNOS5
+     tristate "Exynos5 high-speed I2C driver"
+     depends on ARCH_EXYNOS5
+     help
+       Say Y here to include support for High-speed I2C controller in the
+       Exynos5 based Samsung SoCs.
+
 config I2C_S6000
      tristate "S6000 I2C support"
      depends on XTENSA_VARIANT_S6000

diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 6181f3f..4b1548c 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile

@@ -61,6 +61,7 @@ obj-$(CONFIG_I2C_PUV3)              += i2c-puv3.o
 obj-$(CONFIG_I2C_PXA)                += i2c-pxa.o
 obj-$(CONFIG_I2C_PXA_PCI)    += i2c-pxa-pci.o
 obj-$(CONFIG_I2C_S3C2410)    += i2c-s3c2410.o
+obj-$(CONFIG_I2C_EXYNOS5)    += i2c-exynos5.o
 obj-$(CONFIG_I2C_S6000)              += i2c-s6000.o
 obj-$(CONFIG_I2C_SH7760)     += i2c-sh7760.o
 obj-$(CONFIG_I2C_SH_MOBILE)  += i2c-sh_mobile.o

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
new file mode 100644
index 0000000..7614f60
--- /dev/null
+++ b/drivers/i2c/busses/i2c-exynos5.c

@@ -0,0 +1,652 @@
+/* linux/drivers/i2c/busses/i2c-exynos5.c

no need for the full path. Generally this would look like:

* i2c-exynos5.c - Samsung Exynos 5 I2C Controller Driver

no strong feelings however.

quoted

+ * Copyright (C) 2012 Samsung Electronics Co., Ltd.
+ *
+ * High speed I2C controller driver
+ * for Exynos5 and later SoCs from Samsung.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/time.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/clk.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
+#include <linux/of_i2c.h>
+
+#include "i2c-exynos5.h"

it doesn't look like this header is even needed. All those macros could
be defined here in the C-source file which is the only user.

quoted

+#define EXYNOS5_I2C_TIMEOUT (msecs_to_jiffies(1000))
+
+/* timeout for pm runtime autosuspend */
+#define EXYNOS5_I2C_PM_TIMEOUT               1000    /* ms */
+
+struct exynos5_i2c {
+     struct i2c_adapter      adap;
+     unsigned int            suspended:1;
+
+     struct i2c_msg          *msg;
+     unsigned int            msg_idx;
+     struct completion       msg_complete;
+     unsigned int            msg_ptr;
+
+     unsigned int            irq;
+
+     void __iomem            *regs;
+     struct clk              *clk;
+     struct device           *dev;
+     int                     gpios[2];
+
+     int                     bus_num;
+     int                     speed_mode;
+};
+
+static const struct of_device_id exynos5_i2c_match[] = {
+     { .compatible = "samsung,exynos5-hsi2c" },
+     {},
+};
+MODULE_DEVICE_TABLE(of, exynos5_i2c_match);
+
+/* TODO: Should go to debugfs */
+static inline void dump_i2c_register(struct exynos5_i2c *i2c)
+{
+     dev_vdbg(i2c->dev, "Register dump(%d) :\n %x\n %x\n %x\n %x\n"
+             " %x\n %x\n %x\n %x\n %x\n"
+             " %x\n %x\n %x\n %x\n %x\n"
+             " %x\n %x\n %x\n %x\n %x\n"
+             " %x\n %x\n %x\n %x\n %x\n",
+             i2c->suspended,
+             readl(i2c->regs + HSI2C_CTL),
+             readl(i2c->regs + HSI2C_FIFO_CTL),
+             readl(i2c->regs + HSI2C_TRAILIG_CTL),
+             readl(i2c->regs + HSI2C_CLK_CTL),
+             readl(i2c->regs + HSI2C_CLK_SLOT),
+             readl(i2c->regs + HSI2C_INT_ENABLE),
+             readl(i2c->regs + HSI2C_INT_STATUS),
+             readl(i2c->regs + HSI2C_ERR_STATUS),
+             readl(i2c->regs + HSI2C_FIFO_STATUS),
+             readl(i2c->regs + HSI2C_TX_DATA),
+             readl(i2c->regs + HSI2C_RX_DATA),
+             readl(i2c->regs + HSI2C_CONF),
+             readl(i2c->regs + HSI2C_AUTO_CONF),
+             readl(i2c->regs + HSI2C_TIMEOUT),
+             readl(i2c->regs + HSI2C_MANUAL_CMD),
+             readl(i2c->regs + HSI2C_TRANS_STATUS),
+             readl(i2c->regs + HSI2C_TIMING_HS1),
+             readl(i2c->regs + HSI2C_TIMING_HS2),
+             readl(i2c->regs + HSI2C_TIMING_HS3),
+             readl(i2c->regs + HSI2C_TIMING_FS1),
+             readl(i2c->regs + HSI2C_TIMING_FS2),
+             readl(i2c->regs + HSI2C_TIMING_FS3),
+             readl(i2c->regs + HSI2C_TIMING_SLA),
+             readl(i2c->regs + HSI2C_ADDR));
+}

NAK, please add debugfs support already. This will be here for a long
time otherwise

Actually patch 2/2 does the same, posted them separately for RFC

quoted

+static inline void exynos5_i2c_stop(struct exynos5_i2c *i2c, int ret)

no need for inline, GCC will handle that. Also, this prototype looks
awkward, you're passing caller's return code to be checked here... it's
weird, to say the least.

quoted

+{
+     dev_dbg(i2c->dev, "STOP\n");

I would call this dev_vdbg()

quoted

+
+     i2c->msg_idx++;
+     if (ret)
+             i2c->msg_idx = ret;
+
+     /* Disable interrrupts */
+     writel(0, i2c->regs + HSI2C_INT_ENABLE);
+     complete(&i2c->msg_complete);
+}
+
+static void exynos5_i2c_en_timeout(struct exynos5_i2c *i2c)
+{
+     unsigned long i2c_timeout = readl(i2c->regs + HSI2C_TIMEOUT);
+
+     /* Clear to enable Timeout */
+     i2c_timeout &= ~HSI2C_TIMEOUT_EN;
+     writel(i2c_timeout, i2c->regs + HSI2C_TIMEOUT);
+}
+
+static void exynos5_i2c_master_run(struct exynos5_i2c *i2c)
+{
+     /* Start data transfer in Master mode */
+     u32 i2c_auto_conf = readl(i2c->regs + HSI2C_AUTO_CONF);
+     i2c_auto_conf |= HSI2C_MASTER_RUN;
+     writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONF);
+}
+
+/* exynos5_i2c_set_bus
+ *
+ * get the i2c bus for a master/slave transaction
+ */

multiline comment style is wrong.

quoted

+static int exynos5_i2c_set_bus(struct exynos5_i2c *i2c, int master)

this function is only called with master set to 1, do you even need that
argument ? Do you have some out-of-tree code for supporting slave mode ?

Please remove 'slave' support completely as that's not supported in
mainline as of today, if you need slave support, then add it to the
framework first.

Will remove slave stuff, I don't plans for the as of now.

quoted

+{
+     unsigned long t_status;
+     int timeout = 400;
+
+     while (timeout-- > 0) {
+             t_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
+
+             if (master) {
+                     if (!(t_status & HSI2C_MASTER_BUSY))
+                             return 0;
+             } else {
+                     if (!(t_status & HSI2C_SLAVE_BUSY))
+                             return 0;
+             }
+
+             msleep(20);
+     }
+
+     return -ETIMEDOUT;
+}
+
+/* exynos5_i2c_irq
+ *
+ * top level IRQ servicing routine
+ */

multiline comment style is wrong.

quoted

+static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
+{
+     struct exynos5_i2c *i2c = dev_id;
+     unsigned long t_stat;
+     unsigned char byte;
+
+     t_stat = readl(i2c->regs + HSI2C_TRANS_STATUS);
+
+     if (t_stat & HSI2C_TRANS_ABORT) {
+             /* deal with arbitration loss */
+             dev_err(i2c->dev, "deal with arbitration loss\n");
+             goto out;
+     }
+     if (i2c->msg->flags & I2C_M_RD) {
+             if (t_stat & HSI2C_TRANS_DONE) {
+                     dev_dbg(i2c->dev, "Device found.");
+                     while ((readl(i2c->regs + HSI2C_FIFO_STATUS) &
+                                     HSI2C_RX_FIFO_EMPTY) == 0) {
+                             byte = readl(i2c->regs + HSI2C_RX_DATA);
+                             dev_dbg(i2c->dev, "read rx_data = %x", byte);
+                             i2c->msg->buf[i2c->msg_ptr++] = byte;
+                     }
+             } else if (t_stat & HSI2C_NO_DEV) {
+                     dev_dbg(i2c->dev, "No device found.");
+                     exynos5_i2c_stop(i2c, -ENXIO);
+             } else if (t_stat & HSI2C_NO_DEV_ACK &&
+                             !(i2c->msg->flags & I2C_M_IGNORE_NAK)) {
+                     dev_dbg(i2c->dev, "No device Ack.");
+                     exynos5_i2c_stop(i2c, -ENXIO);
+             }
+     } else {
+             byte = i2c->msg->buf[i2c->msg_ptr++];
+             dev_dbg(i2c->dev, "write tx_data = %x ", byte);
+             writel(byte, i2c->regs + HSI2C_TX_DATA);
+     }
+
+     if (i2c->msg_ptr >= i2c->msg->len)
+             exynos5_i2c_stop(i2c, 0);
+
+ out:
+     /* Set those bits to clear them */
+     writel(readl(i2c->regs + HSI2C_INT_STATUS),
+                             i2c->regs + HSI2C_INT_STATUS);
+
+     return IRQ_HANDLED;
+}
+
+static void exynos5_i2c_message_start(struct exynos5_i2c *i2c,
+                                   struct i2c_msg *msgs)
+{
+     unsigned long usi_ctl = HSI2C_FUNC_MODE_I2C | HSI2C_MASTER;
+     unsigned long i2c_auto_conf;
+     unsigned long i2c_addr = ((msgs->addr & 0x7f) << 10);
+     unsigned long usi_int_en = 0;
+
+     exynos5_i2c_en_timeout(i2c);
+
+     if (msgs->flags & I2C_M_RD) {
+             usi_ctl &= ~HSI2C_TXCHON;
+             usi_ctl |= HSI2C_RXCHON;
+
+             i2c_auto_conf |= HSI2C_READ_WRITE;
+
+             usi_int_en |= (HSI2C_INT_RX_ALMOSTFULL_EN |
+                     HSI2C_INT_TRAILING_EN);
+     } else {
+             usi_ctl &= ~HSI2C_RXCHON;
+             usi_ctl |= HSI2C_TXCHON;
+
+             i2c_auto_conf &= ~HSI2C_READ_WRITE;
+
+             usi_int_en |= HSI2C_INT_TX_ALMOSTEMPTY_EN;
+     }
+
+     writel(i2c_addr, i2c->regs + HSI2C_ADDR);
+     writel(usi_ctl, i2c->regs + HSI2C_CTL);
+
+     i2c_auto_conf |= i2c->msg->len;
+     i2c_auto_conf |= HSI2C_STOP_AFTER_TRANS;
+     writel(i2c_auto_conf, i2c->regs + HSI2C_AUTO_CONF);
+
+     exynos5_i2c_master_run(i2c);
+
+     /* Enable appropriate interrupts */
+     writel(usi_int_en, i2c->regs + HSI2C_INT_ENABLE);
+}
+
+static int exynos5_i2c_doxfer(struct exynos5_i2c *i2c, struct i2c_msg *msgs)
+{
+     unsigned long timeout;
+
+     if (i2c->suspended) {
+             dev_err(i2c->dev, "HS-I2C is not initialzed.\n");
+             return -EIO;
+     }
+
+     if (exynos5_i2c_set_bus(i2c, 1)) {
+             dev_err(i2c->dev, "cannot get bus, Master busy.\n");
+             return -EAGAIN;
+     }
+
+     i2c->msg = msgs;
+     i2c->msg_ptr = 0;
+     i2c->msg_idx = 0;
+
+     INIT_COMPLETION(i2c->msg_complete);
+
+     exynos5_i2c_message_start(i2c, msgs);
+
+     timeout = wait_for_completion_timeout(&i2c->msg_complete,
+             EXYNOS5_I2C_TIMEOUT);
+
+     if (timeout == 0)
+             dev_dbg(i2c->dev, "timeout\n");
+     else if (i2c->msg_idx != msgs->len)
+             dev_dbg(i2c->dev, "incomplete xfer (%d)\n", i2c->msg_idx);
+
+     return i2c->msg_idx;
+}
+
+static int exynos5_i2c_xfer(struct i2c_adapter *adap,
+                     struct i2c_msg *msgs, int num)
+{
+     struct exynos5_i2c *i2c = (struct exynos5_i2c *)adap->algo_data;
+     int retry, i;
+     int ret;
+
+     ret = pm_runtime_get_sync(i2c->dev);
+     if (IS_ERR_VALUE(ret))
+             goto out;
+
+     clk_prepare_enable(i2c->clk);
+
+     for (retry = 0; retry < adap->retries; retry++) {
+             for (i = 0; i < num; i++) {
+                     ret = exynos5_i2c_doxfer(i2c, msgs);
+                     msgs++;
+
+                     if (ret == -EAGAIN)
+                             break;
+             }
+             if (i == num) {
+                     clk_disable_unprepare(i2c->clk);
+                     ret = i2c->msg_idx;
+                     goto out;
+             }

why don't you just move this to the out label ?

Moving this to label, din't look so clean to me..

quoted

+             dev_dbg(i2c->dev, "retrying transfer (%d)\n", retry);
+
+             udelay(100);
+     }
+
+     ret = -EREMOTEIO;
+     clk_disable_unprepare(i2c->clk);
+ out:
+     pm_runtime_mark_last_busy(i2c->dev);
+     pm_runtime_put_autosuspend(i2c->dev);
+     return ret;
+}
+
+static u32 exynos5_i2c_func(struct i2c_adapter *adap)
+{
+     return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm exynos5_i2c_algorithm = {
+     .master_xfer            = exynos5_i2c_xfer,
+     .functionality          = exynos5_i2c_func,
+};
+
+static int exynos5_i2c_set_timing(struct exynos5_i2c *i2c, int speed_mode)
+{
+     unsigned long i2c_timing_s1;
+     unsigned long i2c_timing_s2;
+     unsigned long i2c_timing_s3;
+     unsigned long i2c_timing_sla;
+     unsigned int op_clk;
+     unsigned int clkin = clk_get_rate(i2c->clk);
+     unsigned int n_clkdiv;
+     unsigned int t_start_su, t_start_hd;
+     unsigned int t_stop_su;
+     unsigned int t_data_su, t_data_hd;
+     unsigned int t_scl_l, t_scl_h;
+     unsigned int t_sr_release;
+     unsigned int t_ftl_cycle;
+     unsigned int i = 0, utemp0 = 0, utemp1 = 0, utemp2 = 0;
+
+     if (speed_mode == HSI2C_HIGH_SPD)
+             op_clk = HSI2C_HS_TX_CLOCK;
+     else
+             op_clk = HSI2C_FS_TX_CLOCK;
+
+     /* FPCLK / FI2C =
+      * (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2) + 8 + 2 * FLT_CYCLE
+      * uTemp0 = (CLK_DIV + 1) * (TSCLK_L + TSCLK_H + 2)
+      * uTemp1 = (TSCLK_L + TSCLK_H + 2)
+      * uTemp2 = TSCLK_L + TSCLK_H
+     */

wrong indentation

(back to my vacations now)

Thanks for your time. (i was wondering if anyone would care during
this vacation season.)

--
balbi

Will post the v3, fixing all comments except the (moving to out label) one.

-- 
Shine bright,
(: Nav :)