[RFC] i2c-designware patches

[RFC] i2c-designware patches

[Shinya Kuribayashi]

Hi Baruch and lists,

Here're various improvements / bug-fixing patches for DW I2C driver.
I'm working with v2.6.27-based kernel, but they must work fine with the
latest mainline kernel.

It's stil in RFC, and I'm working with it for some time.  Any comments
or suggestions are highly appreciated.  Then I'll respin and give it a
test, thanks.

Baruch, I'd say the base driver is in good shape enogh, so I'm having
a fun with modifing the driver.  Thanks for the initial work.

P.S. ARM and MIPS lists are Cc:ed as I believe there must be potential
users of this driver.


Shinya Kuribayashi (16):
      i2c-designware: Consolidate to use 32-bit word accesses
      i2c-designware: Don't use the IC_CLR_INTR register to clear interrupts
      i2c-designware: Use platform_get_irq helper
      i2c-designware: i2c_dw_read: Take "struct dw_i2c_dev" pointer
      i2c-designware: i2c_dw_xfer_msg: Take "struct dw_i2c_dev" pointer
      i2c-designware: Remove an useless local variable "num"
      i2c-designware: Set a clock name to DesignWare I2C clock source
      i2c-designware: Improve _HCNT/_LCNT calculation
      i2c-designware: i2c_dw_xfer_msg: Fix an i2c_msg search bug
      i2c-designware: Do dw_i2c_pump_msg's jobs in the interrutp handler
      i2c-designware: Set Tx/Rx FIFO threshold levels
      i2c-designware: Divide i2c_dw_xfer_msg into two functions
      i2c-designware: i2c_dw_xfer_msg: Introduce a local "buf" pointer
      i2c-designware: Deferred FIFO-data-counting variables initialization
      i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error
      i2c-designware: Add I2C_FUNC_SMBUS_* bits

 drivers/i2c/busses/i2c-designware.c |  367 +++++++++++++++++++++++++----------
 1 files changed, 264 insertions(+), 103 deletions(-)

-- 
Shinya Kuribayashi
NEC Electronics

[PATCH 01/16] i2c-designware: Consolidate to use 32-bit word accesses

[Shinya Kuribayashi]

This driver looks originally meant for armel machines where readw()/
writew() works perfectly fine.  But that doens't work for big-endian
systems.

This patch converts all 8/16-bit-aware usages to 32-bit variants, so
that the driver works for MIPS big-endian machines, too.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   76 +++++++++++++++++-----------------
 1 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index b444762..a4f928e 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -162,14 +162,14 @@ struct dw_i2c_dev {
 	struct i2c_msg		*msgs;
 	int			msgs_num;
 	int			msg_write_idx;
-	u16			tx_buf_len;
+	u32			tx_buf_len;
 	u8			*tx_buf;
 	int			msg_read_idx;
-	u16			rx_buf_len;
+	u32			rx_buf_len;
 	u8			*rx_buf;
 	int			msg_err;
 	unsigned int		status;
-	u16			abort_source;
+	u32			abort_source;
 	int			irq;
 	struct i2c_adapter	adapter;
 	unsigned int		tx_fifo_depth;
@@ -187,25 +187,25 @@ struct dw_i2c_dev {
 static void i2c_dw_init(struct dw_i2c_dev *dev)
 {
 	u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
-	u16 ic_con;
+	u32 ic_con;
 
 	/* Disable the adapter */
-	writeb(0, dev->base + DW_IC_ENABLE);
+	writel(0, dev->base + DW_IC_ENABLE);
 
 	/* set standard and fast speed deviders for high/low periods */
-	writew((input_clock_khz * 40 / 10000)+1, /* std speed high, 4us */
+	writel((input_clock_khz * 40 / 10000)+1, /* std speed high, 4us */
 			dev->base + DW_IC_SS_SCL_HCNT);
-	writew((input_clock_khz * 47 / 10000)+1, /* std speed low, 4.7us */
+	writel((input_clock_khz * 47 / 10000)+1, /* std speed low, 4.7us */
 			dev->base + DW_IC_SS_SCL_LCNT);
-	writew((input_clock_khz *  6 / 10000)+1, /* fast speed high, 0.6us */
+	writel((input_clock_khz *  6 / 10000)+1, /* fast speed high, 0.6us */
 			dev->base + DW_IC_FS_SCL_HCNT);
-	writew((input_clock_khz * 13 / 10000)+1, /* fast speed low, 1.3us */
+	writel((input_clock_khz * 13 / 10000)+1, /* fast speed low, 1.3us */
 			dev->base + DW_IC_FS_SCL_LCNT);
 
 	/* configure the i2c master */
 	ic_con = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
 		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
-	writew(ic_con, dev->base + DW_IC_CON);
+	writel(ic_con, dev->base + DW_IC_CON);
 }
 
 /*
@@ -215,7 +215,7 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 {
 	int timeout = TIMEOUT;
 
-	while (readb(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
+	while (readl(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
 		if (timeout <= 0) {
 			dev_warn(dev->dev, "timeout waiting for bus ready\n");
 			return -ETIMEDOUT;
@@ -239,29 +239,29 @@ i2c_dw_xfer_msg(struct i2c_adapter *adap)
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msgs = dev->msgs;
 	int num = dev->msgs_num;
-	u16 ic_con, intr_mask;
-	int tx_limit = dev->tx_fifo_depth - readb(dev->base + DW_IC_TXFLR);
-	int rx_limit = dev->rx_fifo_depth - readb(dev->base + DW_IC_RXFLR);
-	u16 addr = msgs[dev->msg_write_idx].addr;
-	u16 buf_len = dev->tx_buf_len;
+	u32 ic_con, intr_mask;
+	int tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
+	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
+	u32 addr = msgs[dev->msg_write_idx].addr;
+	u32 buf_len = dev->tx_buf_len;
 
 	if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
 		/* Disable the adapter */
-		writeb(0, dev->base + DW_IC_ENABLE);
+		writel(0, dev->base + DW_IC_ENABLE);
 
 		/* set the slave (target) address */
-		writew(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
+		writel(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
 
 		/* if the slave address is ten bit address, enable 10BITADDR */
-		ic_con = readw(dev->base + DW_IC_CON);
+		ic_con = readl(dev->base + DW_IC_CON);
 		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
 			ic_con |= DW_IC_CON_10BITADDR_MASTER;
 		else
 			ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
-		writew(ic_con, dev->base + DW_IC_CON);
+		writel(ic_con, dev->base + DW_IC_CON);
 
 		/* Enable the adapter */
-		writeb(1, dev->base + DW_IC_ENABLE);
+		writel(1, dev->base + DW_IC_ENABLE);
 	}
 
 	for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
@@ -287,10 +287,10 @@ i2c_dw_xfer_msg(struct i2c_adapter *adap)
 
 		while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
 			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
-				writew(0x100, dev->base + DW_IC_DATA_CMD);
+				writel(0x100, dev->base + DW_IC_DATA_CMD);
 				rx_limit--;
 			} else
-				writew(*(dev->tx_buf++),
+				writel(*(dev->tx_buf++),
 						dev->base + DW_IC_DATA_CMD);
 			tx_limit--; buf_len--;
 		}
@@ -302,7 +302,7 @@ i2c_dw_xfer_msg(struct i2c_adapter *adap)
 		dev->status |= STATUS_WRITE_IN_PROGRESS;
 	} else
 		dev->status &= ~STATUS_WRITE_IN_PROGRESS;
-	writew(intr_mask, dev->base + DW_IC_INTR_MASK);
+	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
 
 	dev->tx_buf_len = buf_len;
 }
@@ -313,11 +313,11 @@ i2c_dw_read(struct i2c_adapter *adap)
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msgs = dev->msgs;
 	int num = dev->msgs_num;
-	u16 addr = msgs[dev->msg_read_idx].addr;
-	int rx_valid = readw(dev->base + DW_IC_RXFLR);
+	u32 addr = msgs[dev->msg_read_idx].addr;
+	int rx_valid = readl(dev->base + DW_IC_RXFLR);
 
 	for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
-		u16 len;
+		u32 len;
 		u8 *buf;
 
 		if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
@@ -336,7 +336,7 @@ i2c_dw_read(struct i2c_adapter *adap)
 		}
 
 		for (; len > 0 && rx_valid > 0; len--, rx_valid--)
-			*buf++ = readb(dev->base + DW_IC_DATA_CMD);
+			*buf++ = readl(dev->base + DW_IC_DATA_CMD);
 
 		if (len > 0) {
 			dev->status |= STATUS_READ_IN_PROGRESS;
@@ -398,7 +398,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		do {
 			i2c_dw_read(adap);
 		} while (dev->status & STATUS_READ_IN_PROGRESS);
-		writeb(0, dev->base + DW_IC_ENABLE);
+		writel(0, dev->base + DW_IC_ENABLE);
 		ret = num;
 		goto done;
 	}
@@ -428,7 +428,7 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
 static void dw_i2c_pump_msg(unsigned long data)
 {
 	struct dw_i2c_dev *dev = (struct dw_i2c_dev *) data;
-	u16 intr_mask;
+	u32 intr_mask;
 
 	i2c_dw_read(&dev->adapter);
 	i2c_dw_xfer_msg(&dev->adapter);
@@ -436,7 +436,7 @@ static void dw_i2c_pump_msg(unsigned long data)
 	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
 	if (dev->status & STATUS_WRITE_IN_PROGRESS)
 		intr_mask |= DW_IC_INTR_TX_EMPTY;
-	writew(intr_mask, dev->base + DW_IC_INTR_MASK);
+	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
 }
 
 /*
@@ -446,19 +446,19 @@ static void dw_i2c_pump_msg(unsigned long data)
 static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 {
 	struct dw_i2c_dev *dev = dev_id;
-	u16 stat;
+	u32 stat;
 
-	stat = readw(dev->base + DW_IC_INTR_STAT);
+	stat = readl(dev->base + DW_IC_INTR_STAT);
 	dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
 	if (stat & DW_IC_INTR_TX_ABRT) {
-		dev->abort_source = readw(dev->base + DW_IC_TX_ABRT_SOURCE);
+		dev->abort_source = readl(dev->base + DW_IC_TX_ABRT_SOURCE);
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
 		dev->status = STATUS_IDLE;
 	} else if (stat & DW_IC_INTR_TX_EMPTY)
 		tasklet_schedule(&dev->pump_msg);
 
-	readb(dev->base + DW_IC_CLR_INTR);	/* clear interrupts */
-	writew(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */
+	readl(dev->base + DW_IC_CLR_INTR);	/* clear interrupts */
+	writel(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */
 	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
 		complete(&dev->cmd_complete);
 
@@ -531,7 +531,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	}
 	i2c_dw_init(dev);
 
-	writew(0, dev->base + DW_IC_INTR_MASK); /* disable IRQ */
+	writel(0, dev->base + DW_IC_INTR_MASK); /* disable IRQ */
 	r = request_irq(dev->irq, i2c_dw_isr, 0, pdev->name, dev);
 	if (r) {
 		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
@@ -587,7 +587,7 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
 	clk_put(dev->clk);
 	dev->clk = NULL;
 
-	writeb(0, dev->base + DW_IC_ENABLE);
+	writel(0, dev->base + DW_IC_ENABLE);
 	free_irq(dev->irq, dev);
 	kfree(dev);
 
-- 
1.6.5

[PATCH 02/16] i2c-designware: Don't use the IC_CLR_INTR register to clear interrupts

[Shinya Kuribayashi]

We're strongly discouraged from using the IC_CLR_INTR register because
it clears all software-clearable interrupts asserted at the moment.

stat = read(IC_INTR_STAT);
  :
  :  <=== Interrupts asserted during this period will be lost
  :
read(IC_CLR_INTR);

Use the separately-prepared IC_CLR_* registers, instead.

At the same time, this patch adds all remaining interrupt definitions
available in the DesignWare I2C hardware.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   77 +++++++++++++++++++++++++++++++++--
 1 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index a4f928e..efddae1 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -49,7 +49,18 @@
 #define DW_IC_FS_SCL_LCNT	0x20
 #define DW_IC_INTR_STAT		0x2c
 #define DW_IC_INTR_MASK		0x30
+#define DW_IC_RAW_INTR_STAT	0x34
 #define DW_IC_CLR_INTR		0x40
+#define DW_IC_CLR_RX_UNDER	0x44
+#define DW_IC_CLR_RX_OVER	0x48
+#define DW_IC_CLR_TX_OVER	0x4c
+#define DW_IC_CLR_RD_REQ	0x50
+#define DW_IC_CLR_TX_ABRT	0x54
+#define DW_IC_CLR_RX_DONE	0x58
+#define DW_IC_CLR_ACTIVITY	0x5c
+#define DW_IC_CLR_STOP_DET	0x60
+#define DW_IC_CLR_START_DET	0x64
+#define DW_IC_CLR_GEN_CALL	0x68
 #define DW_IC_ENABLE		0x6c
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
@@ -64,9 +75,18 @@
 #define DW_IC_CON_RESTART_EN		0x20
 #define DW_IC_CON_SLAVE_DISABLE		0x40
 
-#define DW_IC_INTR_TX_EMPTY	0x10
-#define DW_IC_INTR_TX_ABRT	0x40
+#define DW_IC_INTR_RX_UNDER	0x001
+#define DW_IC_INTR_RX_OVER	0x002
+#define DW_IC_INTR_RX_FULL	0x004
+#define DW_IC_INTR_TX_OVER	0x008
+#define DW_IC_INTR_TX_EMPTY	0x010
+#define DW_IC_INTR_RD_REQ	0x020
+#define DW_IC_INTR_TX_ABRT	0x040
+#define DW_IC_INTR_RX_DONE	0x080
+#define DW_IC_INTR_ACTIVITY	0x100
 #define DW_IC_INTR_STOP_DET	0x200
+#define DW_IC_INTR_START_DET	0x400
+#define DW_IC_INTR_GEN_CALL	0x800
 
 #define DW_IC_STATUS_ACTIVITY	0x1
 
@@ -439,6 +459,55 @@ static void dw_i2c_pump_msg(unsigned long data)
 	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
 }
 
+static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
+{
+	u32 stat;
+
+	/*
+	 * The IC_INTR_STAT register just indicates "enabled" interrupts.
+	 * Ths unmasked raw version of interrupt status bits are available
+	 * in the IC_RAW_INTR_STAT register.
+	 *
+	 * That is,
+	 *   stat = readl(IC_INTR_STAT);
+	 * equals to,
+	 *   stat = readl(IC_RAW_INTR_STAT) & readl(IC_INTR_MASK);
+	 *
+	 * The raw version might be useful for debugging purposes.
+	 */
+	stat = readl(dev->base + DW_IC_INTR_STAT);
+
+	/*
+	 * Do not use the IC_CLR_INTR register to clear interrupts, or
+	 * you'll miss some interrupts, triggered during the period from
+	 * reading IC_INTR_STAT to reading IC_CLR_INTR.
+	 *
+	 * Use the separately-prepared IC_CLR_* registers instead.
+	 */
+	if (stat & DW_IC_INTR_RX_UNDER)
+		readl(dev->base + DW_IC_CLR_RX_UNDER);
+	if (stat & DW_IC_INTR_RX_OVER)
+		readl(dev->base + DW_IC_CLR_RX_OVER);
+	if (stat & DW_IC_INTR_TX_OVER)
+		readl(dev->base + DW_IC_CLR_TX_OVER);
+	if (stat & DW_IC_INTR_RD_REQ)
+		readl(dev->base + DW_IC_CLR_RD_REQ);
+	if (stat & DW_IC_INTR_TX_ABRT)
+		readl(dev->base + DW_IC_CLR_TX_ABRT);
+	if (stat & DW_IC_INTR_RX_DONE)
+		readl(dev->base + DW_IC_CLR_RX_DONE);
+	if (stat & DW_IC_INTR_ACTIVITY)
+		readl(dev->base + DW_IC_CLR_ACTIVITY);
+	if (stat & DW_IC_INTR_STOP_DET)
+		readl(dev->base + DW_IC_CLR_STOP_DET);
+	if (stat & DW_IC_INTR_START_DET)
+		readl(dev->base + DW_IC_CLR_START_DET);
+	if (stat & DW_IC_INTR_GEN_CALL)
+		readl(dev->base + DW_IC_CLR_GEN_CALL);
+
+	return stat;
+}
+
 /*
  * Interrupt service routine. This gets called whenever an I2C interrupt
  * occurs.
@@ -448,8 +517,9 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	struct dw_i2c_dev *dev = dev_id;
 	u32 stat;
 
-	stat = readl(dev->base + DW_IC_INTR_STAT);
+	stat = i2c_dw_read_clear_intrbits(dev);
 	dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
+
 	if (stat & DW_IC_INTR_TX_ABRT) {
 		dev->abort_source = readl(dev->base + DW_IC_TX_ABRT_SOURCE);
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
@@ -457,7 +527,6 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	} else if (stat & DW_IC_INTR_TX_EMPTY)
 		tasklet_schedule(&dev->pump_msg);
 
-	readl(dev->base + DW_IC_CLR_INTR);	/* clear interrupts */
 	writel(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */
 	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
 		complete(&dev->cmd_complete);
-- 
1.6.5

[PATCH 03/16] i2c-designware: Use platform_get_irq helper

[Shinya Kuribayashi]

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index efddae1..0164092 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -543,8 +543,8 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev;
 	struct i2c_adapter *adap;
-	struct resource *mem, *irq, *ioarea;
-	int r;
+	struct resource *mem, *ioarea;
+	int irq, r;
 
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -553,10 +553,10 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!irq) {
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
 		dev_err(&pdev->dev, "no irq resource?\n");
-		return -EINVAL;
+		return irq; /* -ENXIO */
 	}
 
 	ioarea = request_mem_region(mem->start, resource_size(mem),
@@ -576,7 +576,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	tasklet_init(&dev->pump_msg, dw_i2c_pump_msg, (unsigned long) dev);
 	mutex_init(&dev->lock);
 	dev->dev = get_device(&pdev->dev);
-	dev->irq = irq->start;
+	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
 	dev->clk = clk_get(&pdev->dev, NULL);
-- 
1.6.5

[PATCH 04/16] i2c-designware: i2c_dw_read: Take "struct dw_i2c_dev" pointer

[Shinya Kuribayashi]

There's no need to interface using with "struct i2c_adapter" pointer.
Let's use a local "struct dw_i2c_dev" pointer, instead.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 0164092..c6a35bf 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -328,9 +328,8 @@ i2c_dw_xfer_msg(struct i2c_adapter *adap)
 }
 
 static void
-i2c_dw_read(struct i2c_adapter *adap)
+i2c_dw_read(struct dw_i2c_dev *dev)
 {
-	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msgs = dev->msgs;
 	int num = dev->msgs_num;
 	u32 addr = msgs[dev->msg_read_idx].addr;
@@ -416,7 +415,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (likely(!dev->cmd_err)) {
 		/* read rx fifo, and disable the adapter */
 		do {
-			i2c_dw_read(adap);
+			i2c_dw_read(dev);
 		} while (dev->status & STATUS_READ_IN_PROGRESS);
 		writel(0, dev->base + DW_IC_ENABLE);
 		ret = num;
@@ -450,7 +449,7 @@ static void dw_i2c_pump_msg(unsigned long data)
 	struct dw_i2c_dev *dev = (struct dw_i2c_dev *) data;
 	u32 intr_mask;
 
-	i2c_dw_read(&dev->adapter);
+	i2c_dw_read(dev);
 	i2c_dw_xfer_msg(&dev->adapter);
 
 	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
-- 
1.6.5

[PATCH 05/16] i2c-designware: i2c_dw_xfer_msg: Take "struct dw_i2c_dev" pointer

[Shinya Kuribayashi]

There's no need to interface using with "struct i2c_adapter" pointer.
Let's use a local "struct dw_i2c_dev" pointer, instead.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index c6a35bf..205f691 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -254,9 +254,8 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
  * that is longer than the size of the TX FIFO.
  */
 static void
-i2c_dw_xfer_msg(struct i2c_adapter *adap)
+i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 {
-	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msgs = dev->msgs;
 	int num = dev->msgs_num;
 	u32 ic_con, intr_mask;
@@ -394,7 +393,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		goto done;
 
 	/* start the transfers */
-	i2c_dw_xfer_msg(adap);
+	i2c_dw_xfer_msg(dev);
 
 	/* wait for tx to complete */
 	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
@@ -450,7 +449,7 @@ static void dw_i2c_pump_msg(unsigned long data)
 	u32 intr_mask;
 
 	i2c_dw_read(dev);
-	i2c_dw_xfer_msg(&dev->adapter);
+	i2c_dw_xfer_msg(dev);
 
 	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
 	if (dev->status & STATUS_WRITE_IN_PROGRESS)
-- 
1.6.5

[PATCH 06/16] i2c-designware: Remove an useless local variable "num"

[Shinya Kuribayashi]

We couldn't know the original intent for this variable, but at this
point it's useless.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 205f691..9d81775 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -257,7 +257,6 @@ static void
 i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
-	int num = dev->msgs_num;
 	u32 ic_con, intr_mask;
 	int tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
 	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
@@ -283,7 +282,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 		writel(1, dev->base + DW_IC_ENABLE);
 	}
 
-	for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
+	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		/* if target address has changed, we need to
 		 * reprogram the target address in the i2c
 		 * adapter when we are done with this transfer
@@ -330,11 +329,10 @@ static void
 i2c_dw_read(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
-	int num = dev->msgs_num;
 	u32 addr = msgs[dev->msg_read_idx].addr;
 	int rx_valid = readl(dev->base + DW_IC_RXFLR);
 
-	for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
+	for (; dev->msg_read_idx < dev->msgs_num; dev->msg_read_idx++) {
 		u32 len;
 		u8 *buf;
 
-- 
1.6.5

[PATCH 07/16] i2c-designware: Set a clock name to DesignWare I2C clock source

[Shinya Kuribayashi]

This driver is originally prepared for the ARM kernel where rich and
well-maintained "clkdev" clock framework is available, and clock name
might not be strictly required.  ARM's clkdev does slightly fuzzy
matching where it basically gives preference to "struct device" mathing
over "clock id".  As long as used for ARM machines, there's no problem.

However, all users of this driver necessarily don't have the same clk
framework with ARM's, as the clk I/F implementation varies depending on
ARCHs and machines.

This patch adds a clock name so that other users with simple/minimum/
limited clk support could make use of the driver.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 9d81775..0f8bd4c 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -575,7 +575,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
-	dev->clk = clk_get(&pdev->dev, NULL);
+	dev->clk = clk_get(&pdev->dev, "dw_i2c_clk");
 	if (IS_ERR(dev->clk)) {
 		r = -ENODEV;
 		goto err_free_mem;
-- 
1.6.5

[PATCH 07/16] i2c-designware: Set a clock name to DesignWare I2C clock source

[Mark Brown]

On Tue, Oct 13, 2009 at 11:50:49AM +0900, Shinya Kuribayashi wrote:
> This driver is originally prepared for the ARM kernel where rich and
> well-maintained "clkdev" clock framework is available, and clock name
> might not be strictly required.  ARM's clkdev does slightly fuzzy
> matching where it basically gives preference to "struct device" mathing
> over "clock id".  As long as used for ARM machines, there's no problem.

> However, all users of this driver necessarily don't have the same clk
> framework with ARM's, as the clk I/F implementation varies depending on
> ARCHs and machines.

> This patch adds a clock name so that other users with simple/minimum/
> limited clk support could make use of the driver.

This seems like something that it'd be better to fix in the relevant
clock frameworks in order to try to keep the API consistently usable
between platforms.  The clkdev matching library that most of the ARM
platforms use should be generic enough to be usable elsewhere, there
is regular discussion of moving it somewhere more generic.

[PATCH 07/16] i2c-designware: Set a clock name to DesignWare I2C clock source

[Shinya Kuribayashi]

Mark Brown wrote:
> On Tue, Oct 13, 2009 at 11:50:49AM +0900, Shinya Kuribayashi wrote:
>> This driver is originally prepared for the ARM kernel where rich and
>> well-maintained "clkdev" clock framework is available, and clock name
>> might not be strictly required.  ARM's clkdev does slightly fuzzy
>> matching where it basically gives preference to "struct device" mathing
>> over "clock id".  As long as used for ARM machines, there's no problem.
> 
>> However, all users of this driver necessarily don't have the same clk
>> framework with ARM's, as the clk I/F implementation varies depending on
>> ARCHs and machines.
> 
>> This patch adds a clock name so that other users with simple/minimum/
>> limited clk support could make use of the driver.
> 
> This seems like something that it'd be better to fix in the relevant
> clock frameworks in order to try to keep the API consistently usable
> between platforms.  The clkdev matching library that most of the ARM
> platforms use should be generic enough to be usable elsewhere, there
> is regular discussion of moving it somewhere more generic.

Before fixing the driver, I've checked various discussion archives on
clock framework, git log histories, and source codes of almost all ARM
common/mach/plat and SH variants, so I think I know enough backgrounds
on this.

-- 
Shinya Kuribayashi
NEC Electronics

[PATCH 07/16] i2c-designware: Set a clock name to DesignWare I2C clock source

[Ben Dooks]

On Tue, Oct 13, 2009 at 11:50:49AM +0900, Shinya Kuribayashi wrote:
> This driver is originally prepared for the ARM kernel where rich and
> well-maintained "clkdev" clock framework is available, and clock name
> might not be strictly required.  ARM's clkdev does slightly fuzzy
> matching where it basically gives preference to "struct device" mathing
> over "clock id".  As long as used for ARM machines, there's no problem.
> 
> However, all users of this driver necessarily don't have the same clk
> framework with ARM's, as the clk I/F implementation varies depending on
> ARCHs and machines.
> 
> This patch adds a clock name so that other users with simple/minimum/
> limited clk support could make use of the driver.

You're meant to match with both the device and name, I belive the
clkdev specification has always said this. I'm of the opinion if
the clkdev implementation isn't getting this right, then it is the
clkdev that should be fixed, not this driver.
 
> Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
> ---
> drivers/i2c/busses/i2c-designware.c |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
> index 9d81775..0f8bd4c 100644
> --- a/drivers/i2c/busses/i2c-designware.c
> +++ b/drivers/i2c/busses/i2c-designware.c
> @@ -575,7 +575,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
> 	dev->irq = irq;
> 	platform_set_drvdata(pdev, dev);
> 
> -	dev->clk = clk_get(&pdev->dev, NULL);
> +	dev->clk = clk_get(&pdev->dev, "dw_i2c_clk");
> 	if (IS_ERR(dev->clk)) {
> 		r = -ENODEV;
> 		goto err_free_mem;

-- 
Ben (ben at fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

[PATCH 07/16] i2c-designware: Set a clock name to DesignWare I2C clock source

[Shinya Kuribayashi]

Ben Dooks wrote:
> On Tue, Oct 13, 2009 at 11:50:49AM +0900, Shinya Kuribayashi wrote:
>> This driver is originally prepared for the ARM kernel where rich and
>> well-maintained "clkdev" clock framework is available, and clock name
>> might not be strictly required.  ARM's clkdev does slightly fuzzy
>> matching where it basically gives preference to "struct device" mathing
>> over "clock id".  As long as used for ARM machines, there's no problem.
>>
>> However, all users of this driver necessarily don't have the same clk
>> framework with ARM's, as the clk I/F implementation varies depending on
>> ARCHs and machines.
>>
>> This patch adds a clock name so that other users with simple/minimum/
>> limited clk support could make use of the driver.
> 
> You're meant to match with both the device and name, I belive the
> clkdev specification has always said this. I'm of the opinion if
> the clkdev implementation isn't getting this right, then it is the
> clkdev that should be fixed, not this driver.

Thanks both, Mark and Ben.  I do see your point, so would like to
fix my clk (not clkdev :-D) implementation accordingly, or keep the
fix local.  

# As you can imagine, my local clk implementation is based on
  "clk_id" name matching, which looks simple for me.

---

By the way, my original intention of the patch was slightly different;
I didn't intend to modify the driver for my clk framework.  Let me
explain a bit here.

The point is, we need to consider clk_get() implementation and
clk_get() user, separately.

* clk_get() implementation varies depending on ARCHs and machines.
  For whatever historical reasons, this is the present situation.

* And clk_get() is expected to pick up a clock source with:

 1) dev_id + clk_id ... strict matching condition (default)
 2) dev_id only ... fuzzy extension1 (clk_id can be regarded as wildcard)
 3) clk_id only ... fuzzy extension2 (dev_id can be regarded as wildcard)

  2) and 3) might be sort of ARM's clkdev extensions
  (I think it's useful), but that's not my point.
  I'll leave that alone for now.

  My point is that clk framework basically requires case 1).

* Then the driver is expected to supply necessary information to
  clk_get(), regardless of clk_get() implementation, or should I say,
  the driver should always supply both "dev_id" and "clk_id" whether
  they are used or not.

  This makes drivers much independent from the clk framework, IMHO.

* From the point of drivers, we're never interested in clk_get()
  implementation, and which matching pattern 1) - 3) is taken.

  Supplying only "dev_id" could be regarded as misuse of clk_get(),
  and will increase dependence on clk framework implementation.

* All other clk_get() users with one ID supplied, should be fixed as
  well.

I intended only to fix the driver in the right direction, so would
like to avoid discussion on clk framework implementation here.

Anyway, I'm going to drop this fix next time.  Thanks!
-- 
Shinya Kuribayashi
NEC Electronics

[PATCH 07/16] i2c-designware: Set a clock name to DesignWare I2C clock source

[Mark Brown]

On Wed, Oct 14, 2009 at 01:19:23PM +0900, Shinya Kuribayashi wrote:

> * And clk_get() is expected to pick up a clock source with:

> 1) dev_id + clk_id ... strict matching condition (default)
> 2) dev_id only ... fuzzy extension1 (clk_id can be regarded as wildcard)
> 3) clk_id only ... fuzzy extension2 (dev_id can be regarded as wildcard)

>  2) and 3) might be sort of ARM's clkdev extensions
>  (I think it's useful), but that's not my point.
>  I'll leave that alone for now.

Part of the reason I was querying this is that you were using the clock
name "dw_i2c_clk" which looks like a globally namespaced name which
would be used with option 3 above.  Several clock frameworks just
completely ignore dev_id which creates a lot of problems down the line.
If it had just been i2c_clk I'd have been less likely to follow up.

Option 3 has never been the intended use of the clock API and is
basically a bug in any clock implementation that requires it for most
drivers but it does need to be used in a few limited cases where the
relevant kernel subsystem doesn't provide a struct device (cpufreq is
the major offender here).

> * From the point of drivers, we're never interested in clk_get()
>  implementation, and which matching pattern 1) - 3) is taken.

>  Supplying only "dev_id" could be regarded as misuse of clk_get(),

As far as I can tell from what rmk is saying it seems that the use of a
NULL string is actually an intended usage for devices where the clocks
aren't otherwise named.  I had the concerns when I first saw this usage
but it does seem to be an expected and intended use.

> and will increase dependence on clk framework implementation.

To a certain extent this is good in that it applies pressure to improve
the clock API implementations concerned.

[PATCH 07/16] i2c-designware: Set a clock name to DesignWare I2C clock source

[Russell King - ARM Linux]

On Wed, Oct 14, 2009 at 01:19:23PM +0900, Shinya Kuribayashi wrote:
> # As you can imagine, my local clk implementation is based on
>  "clk_id" name matching, which looks simple for me.

... but totally wrong, and will eventually bite you.  Please use clkdev
and use the device names for the primary matching.  clkdev is what I see
as the de-facto implementation that should eventually become totally
arch independent (it is today, but lives in arch/arm for no real reason
other than my lazyness.)

> * And clk_get() is expected to pick up a clock source with:
>
> 1) dev_id + clk_id ... strict matching condition (default)
> 2) dev_id only ... fuzzy extension1 (clk_id can be regarded as wildcard)
> 3) clk_id only ... fuzzy extension2 (dev_id can be regarded as wildcard)
>
>  2) and 3) might be sort of ARM's clkdev extensions
>  (I think it's useful), but that's not my point.
>  I'll leave that alone for now.
>
>  My point is that clk framework basically requires case 1).

Incorrect.  There is no requirement for strict matching.  Think about
the situation where you have multiple different devices, and the SoC
designer has decided to tie all those clock signals to one clock source,
and this is the only clock in the system.

You might decide that your clk_get() implementation can just return that
single clock irrespective of the parameters passed.  That's a totally
legal clk API implementation.

Also, I refer you to the description of clk_get():

 * clk_get - lookup and obtain a reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock comsumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
 * valid IS_ERR() condition containing errno.  The implementation
 * uses @dev and @id to determine the clock consumer, and thereby
 * the clock producer.  (IOW, @id may be identical strings, but
 * clk_get may return different clock producers depending on @dev.)

Note the comment about identical IDs returning different producers.

> * Then the driver is expected to supply necessary information to
>  clk_get(), regardless of clk_get() implementation, or should I say,
>  the driver should always supply both "dev_id" and "clk_id" whether
>  they are used or not.

Nope.  On several devices which only consume one clock signal, we
explicitly pass a NULL ID to absolutely prevent people going down
the road of matching only by "clk id" - it's been shown to be the
wrong approach, and leads people right down the path of passing
clock strings and pointers into drivers.

That's precisely what the clk API was designed to prevent - and
becomes all together unnecessary when the clk API is implemented
as I designed it - to match primerily by device ID.

> I intended only to fix the driver in the right direction, so would
> like to avoid discussion on clk framework implementation here.

I would strongly advise you to ensure that your implementation conforms
to the intentions of the clk API as I outline above to avoid problems
in the future.

[PATCH 07/16] i2c-designware: Set a clock name to DesignWare I2C clock source

[Shinya Kuribayashi]

Ben, Mark and Russell,

Russell King - ARM Linux wrote:
[ big snip ]
> I would strongly advise you to ensure that your implementation conforms
> to the intentions of the clk API

Thanks for kind explanations.  I'd like to give the clkdev a try.
-- 
Shinya Kuribayashi
NEC Electronics

[PATCH 08/16] i2c-designware: Improve _HCNT/_LCNT calculation

[Shinya Kuribayashi]

* Calculate with accurate conditional expressions from DW manuals.
* Round ic_clk by adding 0.5 as it's important at high ic_clk rate.
* Take into account "tHD;STA" issue for _HCNT calculation.
* Take into account "tf" for _LCNT calculation.
* Add "cond" and "offset" fot further correction requirements.

For _HCNT calculation, there's one issue needs to be carefully
considered; DesignWare I2C core doesn't seem to have solid strategy
to meet the tHD;STA timing spec.  If you configure _HCNT based on the
tHIGH timing spec, it easily results in violation of the tHD;STA spec.

After many trials, we came to the conclustion that the tHD;STA period
is proportional to (_HCNT + 3), and this should be selected by default.

As for _LCNT calculation, DW I2C core has one characteristic behavior;
he starts counting the SCL CNTs for the LOW period of the SCL clock
(tLOW) as soon as it pulls the SCL line.  At that time, he doesn't take
into account the fall time of SCL signal (tf), IOW, he starts counting
CNTs without confirming the SCL input voltage has dropped to below VIL.

This characteristics becomes a problem on some platforms where tf is
considerably long, and results in violation of the tLOW timing spec.

To make the driver configurable as much as possible for various cases,
we'd have separated arguments "tf" and "offset", and for safety default
values should be 0.3 us and 0, respectively.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   92 +++++++++++++++++++++++++++++++---
 1 files changed, 83 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 0f8bd4c..26efd3a 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -196,6 +196,60 @@ struct dw_i2c_dev {
 	unsigned int		rx_fifo_depth;
 };
 
+static u32
+i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
+{
+	/*
+	 * DesignWare I2C core doesn't seem to have solid strategy to meet
+	 * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
+	 * will result in violation of the tHD;STA spec.
+	 */
+	if (cond)
+		/*
+		 * Conditional expression:
+		 *
+		 *   IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH
+		 *
+		 * This is based on the DW manuals, and representing an
+		 * ideal configuration.  The resulting I2C bus speed will
+		 * be faster than any of the others.
+		 *
+		 * If your hardware is free from tHD;STA issue, try this one.
+		 */
+		return (ic_clk * tSYMBOL + 5000) / 10000 - 8 + offset;
+	else
+		/*
+		 * Conditional expression:
+		 *
+		 *   IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)
+		 *
+		 * This is just experimental rule; the tHD;STA period turned
+		 * out to be proportinal to (_HCNT + 3).  With this setting,
+		 * we could meet both tHIGH and tHD;STA timing specs.
+		 *
+		 * If unsure, you'd better to take this alternative.
+		 *
+		 * The reason why we need to take into account "tf" here,
+		 * is the same as described in i2c_dw_scl_lcnt().
+		 */
+		return (ic_clk * (tSYMBOL + tf) + 5000) / 10000 - 3 + offset;
+}
+
+static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
+{
+	/*
+	 * Conditional expression:
+	 *
+	 *   IC_[FS]S_SCL_LCNT + 1 >= IC_CLK * (tLOW + tf)
+	 *
+	 * DW I2C core starts counting the SCL CNTs for the LOW period
+	 * of the SCL clock (tLOW) as soon as it pulls the SCL line.
+	 * We need to take into account tf to meet the tLOW timing spec.
+	 * Default tf value should be 0.3 us, for safety.
+	 */
+	return ((ic_clk * (tLOW + tf) + 5000) / 10000) - 1 + offset;
+}
+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data
@@ -207,20 +261,40 @@ struct dw_i2c_dev {
 static void i2c_dw_init(struct dw_i2c_dev *dev)
 {
 	u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
-	u32 ic_con;
+	u32 ic_con, hcnt, lcnt;
 
 	/* Disable the adapter */
 	writel(0, dev->base + DW_IC_ENABLE);
 
 	/* set standard and fast speed deviders for high/low periods */
-	writel((input_clock_khz * 40 / 10000)+1, /* std speed high, 4us */
-			dev->base + DW_IC_SS_SCL_HCNT);
-	writel((input_clock_khz * 47 / 10000)+1, /* std speed low, 4.7us */
-			dev->base + DW_IC_SS_SCL_LCNT);
-	writel((input_clock_khz *  6 / 10000)+1, /* fast speed high, 0.6us */
-			dev->base + DW_IC_FS_SCL_HCNT);
-	writel((input_clock_khz * 13 / 10000)+1, /* fast speed low, 1.3us */
-			dev->base + DW_IC_FS_SCL_LCNT);
+
+	/* Standard-mode */
+	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+				40,	/* tHD;STA = tHIGH = 4.0 us */
+				3,	/* tf = 0.3 us */
+				0,	/* 0: DW default, 1: Ideal */
+				0);	/* No offset */
+	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+				47,	/* tLOW = 4.7 us */
+				3,	/* tf = 0.3 us */
+				0);	/* No offset */
+	writel(hcnt, dev->base + DW_IC_SS_SCL_HCNT);
+	writel(lcnt, dev->base + DW_IC_SS_SCL_LCNT);
+	dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
+
+	/* Fast-mode */
+	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+				6,	/* tHD;STA = tHIGH = 0.6 us */
+				3,	/* tf = 0.3 us */
+				0,	/* 0: DW default, 1: Ideal */
+				0);	/* No offset */
+	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+				13,	/* tLOW = 1.3 us */
+				3,	/* tf = 0.3 us */
+				0);	/* No offset */
+	writel(hcnt, dev->base + DW_IC_FS_SCL_HCNT);
+	writel(lcnt, dev->base + DW_IC_FS_SCL_LCNT);
+	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
 	/* configure the i2c master */
 	ic_con = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
-- 
1.6.5

[PATCH 09/16] i2c-designware: i2c_dw_xfer_msg: Fix an i2c_msg search bug

[Shinya Kuribayashi]

In case an i2c_msg, which is currently work-in-progress, has more bytes
to be written, we need to set STATUS_WRITE_IN_PROGRESS _and_ exit from
the msg_write_idx-search loop.  Otherwise, we'll overtake the current
index without waiting for transmission completed.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 26efd3a..6bfdc5f 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -386,17 +386,22 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 						dev->base + DW_IC_DATA_CMD);
 			tx_limit--; buf_len--;
 		}
+
+		dev->tx_buf_len = buf_len;
+
+		/* more bytes to be written? */
+		if (buf_len > 0) {
+			dev->status |= STATUS_WRITE_IN_PROGRESS;
+			break;
+		} else {
+			dev->status &= ~STATUS_WRITE_IN_PROGRESS;
+		}
 	}
 
 	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
-	if (buf_len > 0) { /* more bytes to be written */
+	if (buf_len > 0)
 		intr_mask |= DW_IC_INTR_TX_EMPTY;
-		dev->status |= STATUS_WRITE_IN_PROGRESS;
-	} else
-		dev->status &= ~STATUS_WRITE_IN_PROGRESS;
 	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
-
-	dev->tx_buf_len = buf_len;
 }
 
 static void
-- 
1.6.5

[PATCH 10/16] i2c-designware: Do dw_i2c_pump_msg's jobs in the interrutp handler

[Shinya Kuribayashi]

Symptom:
--------
When we're going to send/receive the data with bigger size than the
Tx FIFO length, the I2C transaction will be divided into several
separated transactions, limited by the Tx FIFO length.

Details:
--------
As a hardware feature, DesignWare I2C core emits a STOP condition
whenever Tx FIFO becomes empty (strictly speaking, whenever the last
byte in the Tx FIFO has been sent out), even if we have more bytes to
be written.  Then, once a new transmit data is written to the Tx FIFO,
DW I2C core will initiate a new I2C transaction, which leads to another
START condition.

This explains how the transaction in question goes, and implies that
current tasklet-based dw_i2c_pump_msg() couldn't meet the timing
constraint required for avoiding the Tx FIFO underrun.

To avoid this scenario, we must keep providing new transmit data out
within a given time period.  In case of Fast-mode + 32-byte Tx FIFO,
for instance, it takes about 22.5[us] to process one byte, and 720[us]
in total.

This patch removes the existing tasklet-based "pump" system, then put
its jobs into the interrupt handler.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   42 ++++++++++++++--------------------
 1 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 6bfdc5f..52a69a2 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -149,7 +149,6 @@ static char *abort_sources[] = {
  * @dev: driver model device node
  * @base: IO registers pointer
  * @cmd_complete: tx completion indicator
- * @pump_msg: continue in progress transfers
  * @lock: protect this struct and IO registers
  * @clk: input reference clock
  * @cmd_err: run time hadware error code
@@ -175,7 +174,6 @@ struct dw_i2c_dev {
 	struct device		*dev;
 	void __iomem		*base;
 	struct completion	cmd_complete;
-	struct tasklet_struct	pump_msg;
 	struct mutex		lock;
 	struct clk		*clk;
 	int			cmd_err;
@@ -324,7 +322,7 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 /*
  * Initiate low level master read/write transaction.
  * This function is called from i2c_dw_xfer when starting a transfer.
- * This function is also called from dw_i2c_pump_msg to continue a transfer
+ * This function is also called from i2c_dw_isr to continue a transfer
  * that is longer than the size of the TX FIFO.
  */
 static void
@@ -489,10 +487,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	/* no error */
 	if (likely(!dev->cmd_err)) {
-		/* read rx fifo, and disable the adapter */
-		do {
-			i2c_dw_read(dev);
-		} while (dev->status & STATUS_READ_IN_PROGRESS);
+		/* Disable the adapter */
 		writel(0, dev->base + DW_IC_ENABLE);
 		ret = num;
 		goto done;
@@ -520,20 +515,6 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
 	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
 }
 
-static void dw_i2c_pump_msg(unsigned long data)
-{
-	struct dw_i2c_dev *dev = (struct dw_i2c_dev *) data;
-	u32 intr_mask;
-
-	i2c_dw_read(dev);
-	i2c_dw_xfer_msg(dev);
-
-	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
-	if (dev->status & STATUS_WRITE_IN_PROGRESS)
-		intr_mask |= DW_IC_INTR_TX_EMPTY;
-	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
-}
-
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
 {
 	u32 stat;
@@ -599,10 +580,22 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 		dev->abort_source = readl(dev->base + DW_IC_TX_ABRT_SOURCE);
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
 		dev->status = STATUS_IDLE;
-	} else if (stat & DW_IC_INTR_TX_EMPTY)
-		tasklet_schedule(&dev->pump_msg);
+	}
+
+	if (stat & DW_IC_INTR_TX_EMPTY) {
+		/* Allocate room for data reception prior to xfer_msg() */
+		i2c_dw_read(dev);
+		i2c_dw_xfer_msg(dev);
+	}
+
+	/*
+	 * No need to modify or disable the interrupt mask here, as
+	 * i2c_dw_xfer_msg() above will do that job according to the
+	 * state machine.
+	 *
+	 * This comment can be removed once approved by I2C list.
+	 */
 
-	writel(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */
 	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
 		complete(&dev->cmd_complete);
 
@@ -648,7 +641,6 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	}
 
 	init_completion(&dev->cmd_complete);
-	tasklet_init(&dev->pump_msg, dw_i2c_pump_msg, (unsigned long) dev);
 	mutex_init(&dev->lock);
 	dev->dev = get_device(&pdev->dev);
 	dev->irq = irq;
-- 
1.6.5

[PATCH 11/16] i2c-designware: Set Tx/Rx FIFO threshold levels

[Shinya Kuribayashi]

As a hardware feature, DesignWare I2C core emits a STOP condition
whenever Tx FIFO becomes empty (strictly speaking, whenever the last
byte in the Tx FIFO has been sent out), even if we have more bytes to
be written.

In other words, we must never make "Tx FIFO underrun" happen during
a transaction, except for the last byte.  For the safety's sake, we'd
make TX_EMPTY interrupt get triggered every time one byte is processed.

Rx FIFO threshold needs to be set as well according to such tx policy.
In addition, this patch also modifies the interrupt settings properly
(enable RX_FULL mask, and hook it in the handler)

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 52a69a2..d9b5790 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -50,6 +50,8 @@
 #define DW_IC_INTR_STAT		0x2c
 #define DW_IC_INTR_MASK		0x30
 #define DW_IC_RAW_INTR_STAT	0x34
+#define DW_IC_RX_TL		0x38
+#define DW_IC_TX_TL		0x3c
 #define DW_IC_CLR_INTR		0x40
 #define DW_IC_CLR_RX_UNDER	0x44
 #define DW_IC_CLR_RX_OVER	0x48
@@ -294,6 +296,10 @@ static void i2c_dw_init(struct dw_i2c_dev *dev)
 	writel(lcnt, dev->base + DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure Tx/Rx FIFO threshold levels */
+	writel(dev->tx_fifo_depth - 1, dev->base + DW_IC_TX_TL);
+	writel(0, dev->base + DW_IC_RX_TL);
+
 	/* configure the i2c master */
 	ic_con = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
 		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
@@ -396,7 +402,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 		}
 	}
 
-	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
+	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT | DW_IC_INTR_RX_FULL;
 	if (buf_len > 0)
 		intr_mask |= DW_IC_INTR_TX_EMPTY;
 	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
@@ -582,11 +588,12 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 		dev->status = STATUS_IDLE;
 	}
 
-	if (stat & DW_IC_INTR_TX_EMPTY) {
-		/* Allocate room for data reception prior to xfer_msg() */
+	/* Allocate room for data reception prior to i2c_dw_xfer_msg(). */
+	if (stat & DW_IC_INTR_RX_FULL)
 		i2c_dw_read(dev);
+
+	if (stat & DW_IC_INTR_TX_EMPTY)
 		i2c_dw_xfer_msg(dev);
-	}
 
 	/*
 	 * No need to modify or disable the interrupt mask here, as
-- 
1.6.5

[PATCH 12/16] i2c-designware: Divide i2c_dw_xfer_msg into two functions

[Shinya Kuribayashi]

We have some steps at the top of i2c_dw_xfer_msg() to set up the slave
address and enable the DW I2C core.  It's executed only when we don't
have STATUS_WRITE_IN_PROGRESS.

Then we need to make sure that STATUS_WRITE_IN_PROGRESS only indicates
that we have a pending i2c_msg to process.  In other words, even if
STATUS_WRITE_IN_PROGRESS is not set, that doesn't mean we're at initial
state in the I2C transaction.

Since i2c_dw_xfer_msg() will be invoked again and again during the
transaction, those init steps have a possibility to be re-processed
needlessly.  For example, this issue easily takes place when processing
a combined transaction with a certain condition (the number of tx bytes
in the first i2c_msg == Tx FIFO depth).

Consequently we should not use STATUS_WRITE_IN_PROGRESS to determine
where we're at in the I2C transaction, then let's separate those
initialization steps from i2c_dw_xfer_msg().

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   45 +++++++++++++++++++---------------
 1 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index d9b5790..de006f0 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -325,6 +325,29 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 	return 0;
 }
 
+static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
+{
+	struct i2c_msg *msgs = dev->msgs;
+	u32 ic_con;
+
+	/* Disable the adapter */
+	writel(0, dev->base + DW_IC_ENABLE);
+
+	/* set the slave (target) address */
+	writel(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
+
+	/* if the slave address is ten bit address, enable 10BITADDR */
+	ic_con = readl(dev->base + DW_IC_CON);
+	if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
+		ic_con |= DW_IC_CON_10BITADDR_MASTER;
+	else
+		ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
+	writel(ic_con, dev->base + DW_IC_CON);
+
+	/* Enable the adapter */
+	writel(1, dev->base + DW_IC_ENABLE);
+}
+
 /*
  * Initiate low level master read/write transaction.
  * This function is called from i2c_dw_xfer when starting a transfer.
@@ -335,31 +358,12 @@ static void
 i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
-	u32 ic_con, intr_mask;
+	u32 intr_mask;
 	int tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
 	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
 	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
 
-	if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
-		/* Disable the adapter */
-		writel(0, dev->base + DW_IC_ENABLE);
-
-		/* set the slave (target) address */
-		writel(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
-
-		/* if the slave address is ten bit address, enable 10BITADDR */
-		ic_con = readl(dev->base + DW_IC_CON);
-		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
-			ic_con |= DW_IC_CON_10BITADDR_MASTER;
-		else
-			ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
-		writel(ic_con, dev->base + DW_IC_CON);
-
-		/* Enable the adapter */
-		writel(1, dev->base + DW_IC_ENABLE);
-	}
-
 	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		/* if target address has changed, we need to
 		 * reprogram the target address in the i2c
@@ -474,6 +478,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		goto done;
 
 	/* start the transfers */
+	i2c_dw_xfer_init(dev);
 	i2c_dw_xfer_msg(dev);
 
 	/* wait for tx to complete */
-- 
1.6.5

[PATCH 13/16] i2c-designware: i2c_dw_xfer_msg: Introduce a local "buf" pointer

[Shinya Kuribayashi]

While we have "buf_len" local variable for dev->tx_buf_len, we don't
have such local variable for dev->tx_buf pointer.  While "buf_len" is
restored at first then updated when we're going to process a new i2c_msg
(in WRITE_IN_PROGRESS case), ->tx_buf is never done so.

Such inconsistency makes the code slightly hard to follow.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---

 Furthermore, even with this change, i2c_dw_xfer_msg() is still
 inconsistent with i2c_dw_read().  I don't have preference around
 here, but would like to sort out.  Any suggestions are welcome.

 drivers/i2c/busses/i2c-designware.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index de006f0..b3152c2 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -363,6 +363,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
 	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
+	u8 *buf = dev->tx_buf;;
 
 	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		/* if target address has changed, we need to
@@ -381,7 +382,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 
 		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
 			/* new i2c_msg */
-			dev->tx_buf = msgs[dev->msg_write_idx].buf;
+			buf	= msgs[dev->msg_write_idx].buf;
 			buf_len = msgs[dev->msg_write_idx].len;
 		}
 
@@ -390,12 +391,12 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 				writel(0x100, dev->base + DW_IC_DATA_CMD);
 				rx_limit--;
 			} else
-				writel(*(dev->tx_buf++),
-						dev->base + DW_IC_DATA_CMD);
+				writel(*buf++, dev->base + DW_IC_DATA_CMD);
 			tx_limit--; buf_len--;
 		}
 
 		dev->tx_buf_len = buf_len;
+		dev->tx_buf = buf;
 
 		/* more bytes to be written? */
 		if (buf_len > 0) {
-- 
1.6.5

[PATCH 14/16] i2c-designware: Deferred FIFO-data-counting variables initialization

[Shinya Kuribayashi]

As the driver and hardware always process the given data in parallel,
it would be better to initialize those {tx,rx}_limit or rx_valid
variables just prior to be used.

This will help to send/receive data as much as possible.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index b3152c2..f7ea032 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -359,8 +359,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
 	u32 intr_mask;
-	int tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
-	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
+	int tx_limit, rx_limit;
 	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
 	u8 *buf = dev->tx_buf;;
@@ -386,6 +385,9 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			buf_len = msgs[dev->msg_write_idx].len;
 		}
 
+		tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
+		rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
+
 		while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
 			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
 				writel(0x100, dev->base + DW_IC_DATA_CMD);
@@ -418,7 +420,7 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
 	u32 addr = msgs[dev->msg_read_idx].addr;
-	int rx_valid = readl(dev->base + DW_IC_RXFLR);
+	int rx_valid;
 
 	for (; dev->msg_read_idx < dev->msgs_num; dev->msg_read_idx++) {
 		u32 len;
@@ -439,6 +441,8 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			buf = dev->rx_buf;
 		}
 
+		rx_valid = readl(dev->base + DW_IC_RXFLR);
+
 		for (; len > 0 && rx_valid > 0; len--, rx_valid--)
 			*buf++ = readl(dev->base + DW_IC_DATA_CMD);
 
-- 
1.6.5

[PATCH 15/16] i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error

[Shinya Kuribayashi]

As wait_for_completion_interruptible_timeout() will be invoked after
the first call to i2c_dw_xfer_msg() is made whether or not an error is
detected in it, we need to mark ->cmd_complete as completed to avoid a
needless HZ timeout.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---

 Or we could change the i2c_dw_xfer_msg() prototype from "void" to
 "int".  Which is preffered?

 drivers/i2c/busses/i2c-designware.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index f7ea032..6f85e28 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -376,6 +376,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			dev_err(dev->dev,
 				"%s: invalid message length\n", __func__);
 			dev->msg_err = -EINVAL;
+			complete(&dev->cmd_complete);
 			return;
 		}
 
-- 
1.6.5

[PATCH 15/16] i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error

[Shinya Kuribayashi]

Shinya Kuribayashi wrote:
> As wait_for_completion_interruptible_timeout() will be invoked after
> the first call to i2c_dw_xfer_msg() is made whether or not an error is
> detected in it, we need to mark ->cmd_complete as completed to avoid a
> needless HZ timeout.

> diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
> index f7ea032..6f85e28 100644
> --- a/drivers/i2c/busses/i2c-designware.c
> +++ b/drivers/i2c/busses/i2c-designware.c
> @@ -376,6 +376,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>  			dev_err(dev->dev,
>  				"%s: invalid message length\n", __func__);
>  			dev->msg_err = -EINVAL;
> +			complete(&dev->cmd_complete);
>  			return;
>  		}

It turned out to be incomplete, I confimred with I2C_FUNC_SMBUS_QUICK
flag for testing.

In [PATCH 10/16] (Do dw_i2c_pump_msg's jobs in the interrutp handler),
I deleted the following line from the interrupt handler,
>-	writel(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */

so some interrupt bits, namely RX_FULL and somtimes TX_EMPTY, are now
kept _opened_ by default.  In error handling procedures, we need to
disable them or shutdown DW I2C core properly, otherwise those
interrupts continue to be asserted.

In addition, I noticed that we need similar treatments in target addr
inconsistency checking path.

Here's problematic parts.  I'll sort out these bits, later.
---
 drivers/i2c/busses/i2c-designware.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 060f2dd..70aaaec 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -370,13 +370,21 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 		 * reprogram the target address in the i2c
 		 * adapter when we are done with this transfer
 		 */
-		if (msgs[dev->msg_write_idx].addr != addr)
+		if (msgs[dev->msg_write_idx].addr != addr) {
+			dev_err(dev->dev,
+				"%s: invalid message address\n", __func__);
+			dev->msg_err = -EINVAL;
+			writel(0, dev->base + DW_IC_ENABLE);
+			complete(&dev->cmd_complete);
 			return;
+		}
 
 		if (msgs[dev->msg_write_idx].len == 0) {
 			dev_err(dev->dev,
 				"%s: invalid message length\n", __func__);
 			dev->msg_err = -EINVAL;
+			writel(0, dev->base + DW_IC_ENABLE);
+			complete(&dev->cmd_complete);
 			return;
 		}
 
@@ -430,8 +438,14 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			continue;
 
 		/* different i2c client, reprogram the i2c adapter */
-		if (msgs[dev->msg_read_idx].addr != addr)
+		if (msgs[dev->msg_read_idx].addr != addr) {
+			dev_err(dev->dev,
+				"%s: invalid message address\n", __func__);
+			dev->msg_err = -EINVAL;
+			writel(0, dev->base + DW_IC_ENABLE);
+			complete(&dev->cmd_complete);
 			return;
+		}
 
 		if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
 			len = msgs[dev->msg_read_idx].len;
-- 
Shinya Kuribayashi
NEC Electronics

[PATCH 16/16] i2c-designware: Add I2C_FUNC_SMBUS_* bits

[Shinya Kuribayashi]

This will ease our testing a bit with i2c-tools.  Note that DW I2C core
doesn't support I2C_FUNC_SMBUS_QUICK, as it's not capable of slave-
addressing-only I2C transactions.

Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
---
 drivers/i2c/busses/i2c-designware.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
index 6f85e28..80c8b8a 100644
--- a/drivers/i2c/busses/i2c-designware.c
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -529,7 +529,14 @@ done:
 
 static u32 i2c_dw_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
+	return	I2C_FUNC_I2C |
+		I2C_FUNC_10BIT_ADDR |
+		I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_BLOCK_DATA |
+		I2C_FUNC_SMBUS_I2C_BLOCK |
+		I2C_FUNC_SMBUS_I2C_BLOCK_2;
 }
 
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
-- 
1.6.5

[PATCH 16/16] i2c-designware: Add I2C_FUNC_SMBUS_* bits

[Baruch Siach]

Hi Shinya,

On Tue, Oct 13, 2009 at 11:54:21AM +0900, Shinya Kuribayashi wrote:
> This will ease our testing a bit with i2c-tools.  Note that DW I2C core
> doesn't support I2C_FUNC_SMBUS_QUICK, as it's not capable of slave-
> addressing-only I2C transactions.

Is this supposed to be applied to mainline?

baruch

> 
> Signed-off-by: Shinya Kuribayashi <shinya.kuribayashi@necel.com>
> ---
> drivers/i2c/busses/i2c-designware.c |    9 ++++++++-
> 1 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
> index 6f85e28..80c8b8a 100644
> --- a/drivers/i2c/busses/i2c-designware.c
> +++ b/drivers/i2c/busses/i2c-designware.c
> @@ -529,7 +529,14 @@ done:
> 
> static u32 i2c_dw_func(struct i2c_adapter *adap)
> {
> -	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
> +	return	I2C_FUNC_I2C |
> +		I2C_FUNC_10BIT_ADDR |
> +		I2C_FUNC_SMBUS_BYTE |
> +		I2C_FUNC_SMBUS_BYTE_DATA |
> +		I2C_FUNC_SMBUS_WORD_DATA |
> +		I2C_FUNC_SMBUS_BLOCK_DATA |
> +		I2C_FUNC_SMBUS_I2C_BLOCK |
> +		I2C_FUNC_SMBUS_I2C_BLOCK_2;
> }
> 
> static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
> -- 
> 1.6.5
> 

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 16/16] i2c-designware: Add I2C_FUNC_SMBUS_* bits

[Shinya Kuribayashi]

Hi Baruch,

Baruch Siach wrote:
> On Tue, Oct 13, 2009 at 11:54:21AM +0900, Shinya Kuribayashi wrote:
>> This will ease our testing a bit with i2c-tools.  Note that DW I2C core
>> doesn't support I2C_FUNC_SMBUS_QUICK, as it's not capable of slave-
>> addressing-only I2C transactions.
> 
> Is this supposed to be applied to mainline?

Yes, I hope so.  But I have to admit I blindly added several flags for
my testing, and should have audited them before submitting patches.

>> @@ -529,7 +529,14 @@ done:
>>
>> static u32 i2c_dw_func(struct i2c_adapter *adap)
>> {
>> -	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
>> +	return	I2C_FUNC_I2C |
>> +		I2C_FUNC_10BIT_ADDR |
>> +		I2C_FUNC_SMBUS_BYTE |
>> +		I2C_FUNC_SMBUS_BYTE_DATA |
>> +		I2C_FUNC_SMBUS_WORD_DATA |
>> +		I2C_FUNC_SMBUS_BLOCK_DATA |
>> +		I2C_FUNC_SMBUS_I2C_BLOCK |
>> +		I2C_FUNC_SMBUS_I2C_BLOCK_2;
>> }

As far as I confirmed the requirements for having I2C_FUNC_SMBUS_*
from drivers/i2c/,

>> +		I2C_FUNC_SMBUS_BLOCK_DATA |

>> +		I2C_FUNC_SMBUS_I2C_BLOCK_2;

should be removed.  About the former, we have not implemented proper
I2C_M_RECV_LEN handling yet [ I'm not sure what it's for ... ], and
the latter doesn't seem to be used anywhere in the kernel.
As for the rest, BYTE/WORD/I2C_BLOCK transaction works for me.

So the resulting func() would be,

static u32 i2c_dw_func(struct i2c_adapter *adap)
{
	return	I2C_FUNC_I2C |
		I2C_FUNC_10BIT_ADDR |
		I2C_FUNC_SMBUS_BYTE |
		I2C_FUNC_SMBUS_BYTE_DATA |
		I2C_FUNC_SMBUS_WORD_DATA |
		I2C_FUNC_SMBUS_I2C_BLOCK;
}

and will be fixed up in the next patchset.
-- 
Shinya Kuribayashi
NEC Electronics

[RFC] i2c-designware patches

[Baruch Siach]

Hi Shinya,

On Tue, Oct 13, 2009 at 11:44:04AM +0900, Shinya Kuribayashi wrote:
> Here're various improvements / bug-fixing patches for DW I2C driver.
> I'm working with v2.6.27-based kernel, but they must work fine with the
> latest mainline kernel.
> 
> It's stil in RFC, and I'm working with it for some time.  Any comments
> or suggestions are highly appreciated.  Then I'll respin and give it a
> test, thanks.
> 
> Baruch, I'd say the base driver is in good shape enogh, so I'm having
> a fun with modifing the driver.  Thanks for the initial work.

Thanks. It is good to see that my work is actually useful for someone. I'll go 
over this series in the coming days. Unfortunately I don't have the hardware 
for testing anymore, but I'll try at least to compile test these patches.

By the way, what is the chip you work with?

baruch

> P.S. ARM and MIPS lists are Cc:ed as I believe there must be potential
> users of this driver.
> 
> 
> Shinya Kuribayashi (16):
>      i2c-designware: Consolidate to use 32-bit word accesses
>      i2c-designware: Don't use the IC_CLR_INTR register to clear interrupts
>      i2c-designware: Use platform_get_irq helper
>      i2c-designware: i2c_dw_read: Take "struct dw_i2c_dev" pointer
>      i2c-designware: i2c_dw_xfer_msg: Take "struct dw_i2c_dev" pointer
>      i2c-designware: Remove an useless local variable "num"
>      i2c-designware: Set a clock name to DesignWare I2C clock source
>      i2c-designware: Improve _HCNT/_LCNT calculation
>      i2c-designware: i2c_dw_xfer_msg: Fix an i2c_msg search bug
>      i2c-designware: Do dw_i2c_pump_msg's jobs in the interrutp handler
>      i2c-designware: Set Tx/Rx FIFO threshold levels
>      i2c-designware: Divide i2c_dw_xfer_msg into two functions
>      i2c-designware: i2c_dw_xfer_msg: Introduce a local "buf" pointer
>      i2c-designware: Deferred FIFO-data-counting variables initialization
>      i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error
>      i2c-designware: Add I2C_FUNC_SMBUS_* bits
> 
> drivers/i2c/busses/i2c-designware.c |  367 +++++++++++++++++++++++++----------
> 1 files changed, 264 insertions(+), 103 deletions(-)
> 
> -- 
> Shinya Kuribayashi
> NEC Electronics

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[RFC] i2c-designware patches

[Shinya Kuribayashi]

Baruch Siach wrote:
> On Tue, Oct 13, 2009 at 11:44:04AM +0900, Shinya Kuribayashi wrote:
>> Here're various improvements / bug-fixing patches for DW I2C driver.
>> I'm working with v2.6.27-based kernel, but they must work fine with the
>> latest mainline kernel.
>>
>> It's stil in RFC, and I'm working with it for some time.  Any comments
>> or suggestions are highly appreciated.  Then I'll respin and give it a
>> test, thanks.
>>
>> Baruch, I'd say the base driver is in good shape enogh, so I'm having
>> a fun with modifing the driver.  Thanks for the initial work.
> 
> Thanks. It is good to see that my work is actually useful for someone. I'll go 
> over this series in the coming days. Unfortunately I don't have the hardware 
> for testing anymore, but I'll try at least to compile test these patches.

No problem, I could do real tests on real hardwares, so just give me a
feedback.  Grammar corrections or rewording is also appriciated.

> By the way, what is the chip you work with?

I'd like to avoid specific comment on this.  I'm working with our big-
endian MIPS SoCs which are not in mainline.

-- 
Shinya Kuribayashi
NEC Electronics

[RFC] i2c-designware patches

[Baruch Siach]

Hi Shinya,

On Tue, Oct 13, 2009 at 11:44:04AM +0900, Shinya Kuribayashi wrote:
> Here're various improvements / bug-fixing patches for DW I2C driver.
> I'm working with v2.6.27-based kernel, but they must work fine with the
> latest mainline kernel.
> 
> It's stil in RFC, and I'm working with it for some time.  Any comments
> or suggestions are highly appreciated.  Then I'll respin and give it a
> test, thanks.
> 
> Baruch, I'd say the base driver is in good shape enogh, so I'm having
> a fun with modifing the driver.  Thanks for the initial work.
> 
> P.S. ARM and MIPS lists are Cc:ed as I believe there must be potential
> users of this driver.
> 
> 
> Shinya Kuribayashi (16):
>      i2c-designware: Consolidate to use 32-bit word accesses
>      i2c-designware: Don't use the IC_CLR_INTR register to clear interrupts
>      i2c-designware: Use platform_get_irq helper
>      i2c-designware: i2c_dw_read: Take "struct dw_i2c_dev" pointer
>      i2c-designware: i2c_dw_xfer_msg: Take "struct dw_i2c_dev" pointer
>      i2c-designware: Remove an useless local variable "num"
>      i2c-designware: Set a clock name to DesignWare I2C clock source
>      i2c-designware: Improve _HCNT/_LCNT calculation
>      i2c-designware: i2c_dw_xfer_msg: Fix an i2c_msg search bug
>      i2c-designware: Do dw_i2c_pump_msg's jobs in the interrutp handler
>      i2c-designware: Set Tx/Rx FIFO threshold levels
>      i2c-designware: Divide i2c_dw_xfer_msg into two functions
>      i2c-designware: i2c_dw_xfer_msg: Introduce a local "buf" pointer
>      i2c-designware: Deferred FIFO-data-counting variables initialization
>      i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error
>      i2c-designware: Add I2C_FUNC_SMBUS_* bits

Great job.

Your contribution to the implementation of transactions that are longer than 
the hardware buffers is very important. I wrote this code as a theoretical 
exercise, as I never had access to an i2c slave which requires long 
transactions. So I'm not surprised to find out that this code was broken.

For all these patches except no. 7 (Set a clock name to DesignWare I2C clock 
source) and no. 16 (Add I2C_FUNC_SMBUS_* bits) you have my

Acked-by: Baruch Siach <baruch@tkos.co.il>

Thanks,
baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[RFC] i2c-designware patches

[Shinya Kuribayashi]

Hi Baruch,

Baruch Siach wrote:
> Your contribution to the implementation of transactions that are longer than 
> the hardware buffers is very important. I wrote this code as a theoretical 
> exercise, as I never had access to an i2c slave which requires long 
> transactions. So I'm not surprised to find out that this code was broken.

Aa far as looking through the source code, I know that it's broken
without giving it a try with real DW I2C hardware ;-), but that'
not a big problem.

As said in the covermail, your initial work, namely i2c_dw_xfer(),
i2c_dw_xfer_msg(), and i2c_dw_read() is well-designed, this _is_
very important.  That makes further enhancements of DW I2C
transactions much easier, and makes me decide to post the patches.

> For all these patches except no. 7 (Set a clock name to DesignWare I2C clock 
> source) and no. 16 (Add I2C_FUNC_SMBUS_* bits) you have my
> 
> Acked-by: Baruch Siach <baruch@tkos.co.il>

Thanks, will do.

I added more 4-5 patches fixing transmit abort handling, and misc
updates all over the patchset (typos, comments, better logics).
They will come up, hopefully, next week.

  Shinya