All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
@ 2009-05-19  9:23 Baruch Siach
       [not found] ` <1242725005-6431-1-git-send-email-baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Baruch Siach @ 2009-05-19  9:23 UTC (permalink / raw)
  To: linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: Ben Dooks, Baruch Siach


Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/i2c/busses/Kconfig          |    9 +
 drivers/i2c/busses/Makefile         |    1 +
 drivers/i2c/busses/i2c-designware.c |  587 +++++++++++++++++++++++++++++++++++
 3 files changed, 597 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f1c6ca7..cef1567 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -326,6 +326,15 @@ config I2C_DAVINCI
 	  devices such as DaVinci NIC.
 	  For details please see http://www.ti.com/davinci
 
+config I2C_DESIGNWARE
+	tristate "Synopsys DesignWare"
+	help
+	  If you say yes to this option, support will be included for the
+	  Synopsys DesignWare I2C adapter. Only master mode is supported.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-designware.
+
 config I2C_GPIO
 	tristate "GPIO-based bitbanging I2C"
 	depends on GENERIC_GPIO
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 776acb6..c2be11e 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
+obj-$(CONFIG_I2C_DESIGNWARE)	+= i2c-designware.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
 obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
 obj-$(CONFIG_I2C_IBM_IIC)	+= i2c-ibm_iic.o
diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
new file mode 100644
index 0000000..0138fea
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -0,0 +1,587 @@
+/*
+ * Synopsys Designware I2C adapter driver (master only).
+ *
+ * Based on the TI DAVINCI I2C adapter driver.
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software Inc.
+ * Copyright (C) 2009 Provigent Ltd.
+ *
+ * ----------------------------------------------------------------------------
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ * ----------------------------------------------------------------------------
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/sched.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+/*
+ * Registers offset
+ */
+#define DW_IC_CON		0x0
+#define DW_IC_TAR		0x4
+#define DW_IC_DATA_CMD		0x10
+#define DW_IC_SS_SCL_HCNT	0x14
+#define DW_IC_SS_SCL_LCNT	0x18
+#define DW_IC_FS_SCL_HCNT	0x1c
+#define DW_IC_FS_SCL_LCNT	0x20
+#define DW_IC_INTR_STAT		0x2c
+#define DW_IC_INTR_MASK		0x30
+#define DW_IC_CLR_INTR		0x40
+#define DW_IC_ENABLE		0x6c
+#define DW_IC_STATUS		0x70
+#define DW_IC_TXFLR		0x74
+#define DW_IC_RXFLR		0x78
+#define DW_IC_COMP_PARAM_1	0xf4
+#define DW_IC_TX_ABRT_SOURCE	0x80
+
+#define DW_IC_CON_MASTER		0x1
+#define DW_IC_CON_SPEED_STD		0x2
+#define DW_IC_CON_SPEED_FAST		0x4
+#define DW_IC_CON_10BITADDR_MASTER	0x10
+#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_STOP_DET	0x200
+
+#define DW_IC_STATUS_ACTIVITY	0x1
+
+#define DW_IC_ERR_TX_ABRT	0x1
+
+/*
+ * status codes
+ */
+#define STATUS_IDLE			0x0
+#define STATUS_WRITE_IN_PROGRESS	0x1
+#define STATUS_READ_IN_PROGRESS		0x2
+
+/*
+ * hardware abort codes
+ *
+ * only expected abort codes are listed here
+ * refer to the datasheet for the full list
+ */
+#define ABRT_7B_ADDR_NOACK	0
+#define ABRT_10ADDR1_NOACK	1
+#define ABRT_10ADDR2_NOACK	2
+#define ABRT_TXDATA_NOACK	3
+#define ABRT_GCALL_NOACK	4
+#define ABRT_GCALL_READ		5
+#define ABRT_SBYTE_ACKDET	7
+#define ABRT_SBYTE_NORSTRT	9
+#define ABRT_10B_RD_NORSTRT	10
+#define ARB_MASTER_DIS		11
+#define ARB_LOST		12
+
+static char *abort_sources[] = {
+	[ABRT_7B_ADDR_NOACK]	=
+		"slave address not acknowledged (7bit mode)",
+	[ABRT_10ADDR1_NOACK]	=
+		"first address byte not acknowledged (10bit mode)",
+	[ABRT_10ADDR2_NOACK]	=
+		"second address byte not acknowledged (10bit mode)",
+	[ABRT_TXDATA_NOACK]		=
+		"data not acknowledged",
+	[ABRT_GCALL_NOACK]		=
+		"no acknowledgement for a general call",
+	[ABRT_GCALL_READ]		=
+		"read after general call",
+	[ABRT_SBYTE_ACKDET]		=
+		"start byte acknowledged",
+	[ABRT_SBYTE_NORSTRT]	=
+		"trying to send start byte when restart is disabled",
+	[ABRT_10B_RD_NORSTRT]	=
+		"trying to read when restart is disabled (10bit mode)",
+	[ARB_MASTER_DIS]		=
+		"trying to use disabled adapter",
+	[ARB_LOST]			=
+		"lost arbitration",
+};
+
+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;
+	struct i2c_msg		*msgs;
+	int			msgs_num;
+	int			msg_write_idx;
+	int			msg_read_idx;
+	int			msg_err;
+	unsigned int		status;
+	u16			abort_source;
+	int			irq;
+	struct i2c_adapter	adapter;
+	unsigned int		tx_fifo_depth;
+	unsigned int		rx_fifo_depth;
+};
+
+/*
+ * This functions configures I2C and enables I2C.
+ * This function is called during I2C init function.
+ */
+static int i2c_dw_init(struct dw_i2c_dev *dev)
+{
+	u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
+	u16 ic_con;
+
+	/* Disable the adapter */
+	writeb(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 */
+			dev->base + DW_IC_SS_SCL_HCNT);
+	writew((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 */
+			dev->base + DW_IC_FS_SCL_HCNT);
+	writew((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);
+
+	return 0;
+}
+
+/*
+ * Waiting for bus not busy
+ */
+static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
+{
+	unsigned long timeout;
+
+	timeout = jiffies + HZ;
+	while (readb(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(dev->dev,
+				 "timeout waiting for bus ready\n");
+			return -ETIMEDOUT;
+		}
+		schedule_timeout(1);
+	}
+
+	return 0;
+}
+
+/*
+ * 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
+ * that is longer than the size of the TX FIFO.
+ */
+static void
+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_valid_bytes = readb(dev->base + DW_IC_TXFLR);
+	int tx_limit = dev->tx_fifo_depth - tx_valid_bytes;
+	int rx_limit = dev->rx_fifo_depth - readb(dev->base + DW_IC_RXFLR)
+		- tx_valid_bytes;
+	static u16 buf_len = 0;
+	static u16 addr;
+
+	if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
+		/* Disable the adapter */
+		writeb(0, dev->base + DW_IC_ENABLE);
+
+		/* set the slave (target) address */
+		writew(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);
+		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);
+
+		/* Enable the adapter */
+		writeb(1, dev->base + DW_IC_ENABLE);
+
+		addr = msgs[dev->msg_write_idx].addr;
+	}
+
+	for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
+		static u8 *buf;
+
+		/* if target address has changed, we need to
+		 * reprogram the target address in the i2c
+		 * adapter when we are done with this transfer
+		 */
+		if (msgs[dev->msg_write_idx].addr != addr)
+			return;
+
+		if (msgs[dev->msg_write_idx].len == 0) {
+			dev_err(dev->dev,
+				"%s: invalid message length\n", __func__);
+			dev->msg_err = -EINVAL;
+			return;
+		}
+
+		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
+			/* new i2c_msg */
+			buf = msgs[dev->msg_write_idx].buf;
+			buf_len = msgs[dev->msg_write_idx].len;
+		}
+
+		for (; buf_len > 0 && tx_limit > 0 && rx_limit > 0; buf_len--)
+			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
+				writew(0x100, dev->base + DW_IC_DATA_CMD);
+				rx_limit--; tx_limit--;
+			} else {
+				writew(*buf++, dev->base + DW_IC_DATA_CMD);
+				tx_limit--;
+			}
+	}
+
+	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
+	if (buf_len > 0) { /* more bytes to be written */
+		intr_mask |= DW_IC_INTR_TX_EMPTY;
+		dev->status |= STATUS_WRITE_IN_PROGRESS;
+	} else
+		dev->status &= ~STATUS_WRITE_IN_PROGRESS;
+	writew(intr_mask, dev->base + DW_IC_INTR_MASK);
+}
+
+static void
+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);
+
+	for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
+		static int len;
+		static u8 *buf;
+
+		if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
+			continue;
+
+		/* different i2c client, reprogram the i2c adapter */
+		if (msgs[dev->msg_read_idx].addr != addr)
+			return;
+
+		if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
+			len = msgs[dev->msg_read_idx].len;
+			buf = msgs[dev->msg_read_idx].buf;
+		}
+
+		for (; len > 0 && rx_valid > 0; len--, rx_valid--)
+			*buf++ = readb(dev->base + DW_IC_DATA_CMD);
+
+		if (len > 0) {
+			dev->status |= STATUS_READ_IN_PROGRESS;
+			return;
+		} else
+			dev->status &= ~STATUS_READ_IN_PROGRESS;
+	}
+}
+
+/*
+ * Prepare controller for a transaction and call i2c_dw_xfer_msg
+ */
+static int
+i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+	int ret;
+
+	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+
+	mutex_lock(&dev->lock);
+
+	INIT_COMPLETION(dev->cmd_complete);
+	dev->msgs = msgs;
+	dev->msgs_num = num;
+	dev->cmd_err = 0;
+	dev->msg_write_idx = 0;
+	dev->msg_read_idx = 0;
+	dev->msg_err = 0;
+	dev->status = STATUS_IDLE;
+
+	ret = i2c_dw_wait_bus_not_busy(dev);
+	if (ret < 0)
+		goto done;
+
+	/* start the transfers */
+	i2c_dw_xfer_msg(adap);
+
+	/* wait for tx to complete */
+	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
+	if (ret == 0) {
+		dev_err(dev->dev, "controller timed out\n");
+		i2c_dw_init(dev);
+		ret = -ETIMEDOUT;
+		goto done;
+	} else if (ret < 0)
+		goto done;
+
+	if (dev->msg_err) {
+		ret = dev->msg_err;
+		goto done;
+	}
+
+	/* no error */
+	if (likely(!dev->cmd_err)) {
+		/* read rx fifo, and disable the adapter */
+		do {
+			i2c_dw_read(adap);
+		} while (dev->status & STATUS_READ_IN_PROGRESS);
+		writeb(0, dev->base + DW_IC_ENABLE);
+		ret = num;
+		goto done;
+	}
+
+	/* We have an error */
+	if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
+		unsigned long abort_source = dev->abort_source;
+		int i;
+
+		for_each_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) {
+		    dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
+		}
+	}
+	ret = -EIO;
+
+done:
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+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;
+	u16 intr_mask;
+
+	i2c_dw_read(&dev->adapter);
+	i2c_dw_xfer_msg(&dev->adapter);
+
+	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);
+}
+
+/*
+ * Interrupt service routine. This gets called whenever an I2C interrupt
+ * occurs.
+ */
+static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
+{
+	struct dw_i2c_dev *dev = dev_id;
+	u16 stat;
+
+	stat = readw(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->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 */
+	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
+		complete(&dev->cmd_complete);
+
+	return IRQ_HANDLED;
+}
+
+static struct i2c_algorithm i2c_dw_algo = {
+	.master_xfer	= i2c_dw_xfer,
+	.functionality	= i2c_dw_func,
+};
+
+static int dw_i2c_probe(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *dev;
+	struct i2c_adapter *adap;
+	struct resource *mem, *irq, *ioarea;
+	int r;
+
+	/* NOTE: driver uses the static register mapping */
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem) {
+		dev_err(&pdev->dev, "no mem resource?\n");
+		return -ENODEV;
+	}
+
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!irq) {
+		dev_err(&pdev->dev, "no irq resource?\n");
+		return -ENODEV;
+	}
+
+	ioarea = request_mem_region(mem->start, (mem->end - mem->start) + 1,
+				    pdev->name);
+	if (!ioarea) {
+		dev_err(&pdev->dev, "I2C region already claimed\n");
+		return -EBUSY;
+	}
+
+	dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
+	if (!dev) {
+		r = -ENOMEM;
+		goto err_release_region;
+	}
+
+	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->start;
+	platform_set_drvdata(pdev, dev);
+
+	dev->clk = clk_get(&pdev->dev, "I2CCLK");
+	if (IS_ERR(dev->clk)) {
+		r = -ENODEV;
+		goto err_free_mem;
+	}
+	clk_enable(dev->clk);
+
+	dev->base = ioremap(mem->start, mem->end - mem->start + 1);
+	if (dev->base == NULL) {
+		dev_err(&pdev->dev, "failure mapping io resources\n");
+		r = -EBUSY;
+		goto err_unuse_clocks;
+	}
+	dev->tx_fifo_depth =
+		((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x00ff0000) >> 16) + 1;
+	dev->rx_fifo_depth =
+		((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x0000ff00) >> 8) + 1;
+	i2c_dw_init(dev);
+
+	writew(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);
+		goto err_iounmap;
+	}
+
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	strlcpy(adap->name, "Synopsys DesignWare I2C adapter", 
+			sizeof(adap->name));
+	adap->algo = &i2c_dw_algo;
+	adap->dev.parent = &pdev->dev;
+
+	adap->nr = pdev->id;
+	r = i2c_add_numbered_adapter(adap);
+	if (r) {
+		dev_err(&pdev->dev, "failure adding adapter\n");
+		goto err_free_irq;
+	}
+
+	return 0;
+
+err_free_irq:
+	free_irq(dev->irq, dev);
+err_iounmap:
+	iounmap(dev->base);
+err_unuse_clocks:
+	clk_disable(dev->clk);
+	clk_put(dev->clk);
+	dev->clk = NULL;
+err_free_mem:
+	platform_set_drvdata(pdev, NULL);
+	put_device(&pdev->dev);
+	kfree(dev);
+err_release_region:
+	release_mem_region(mem->start, (mem->end - mem->start) + 1);
+
+	return r;
+}
+
+static int dw_i2c_remove(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+	struct resource *mem;
+
+	platform_set_drvdata(pdev, NULL);
+	i2c_del_adapter(&dev->adapter);
+	put_device(&pdev->dev);
+
+	clk_disable(dev->clk);
+	clk_put(dev->clk);
+	dev->clk = NULL;
+
+	writeb(0, dev->base + DW_IC_ENABLE);
+	free_irq(dev->irq, dev);
+	kfree(dev);
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(mem->start, (mem->end - mem->start) + 1);
+	return 0;
+}
+
+/* work with hotplug and coldplug */
+MODULE_ALIAS("platform:i2c_designware");
+
+static struct platform_driver dw_i2c_driver = {
+	.probe		= dw_i2c_probe,
+	.remove		= dw_i2c_remove,
+	.driver		= {
+		.name	= "i2c_designware",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init dw_i2c_init_driver(void)
+{
+	return platform_driver_register(&dw_i2c_driver);
+}
+module_init(dw_i2c_init_driver);
+
+static void __exit dw_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&dw_i2c_driver);
+}
+module_exit(dw_i2c_exit_driver);
+
+MODULE_AUTHOR("Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
+MODULE_LICENSE("GPL");
-- 
1.6.2.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
       [not found] ` <1242725005-6431-1-git-send-email-baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
@ 2009-05-26 21:39   ` Ben Dooks
       [not found]     ` <20090526213917.GH23114-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
  2009-05-27 19:23   ` Linus Walleij
  1 sibling, 1 reply; 12+ messages in thread
From: Ben Dooks @ 2009-05-26 21:39 UTC (permalink / raw)
  To: Baruch Siach; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

On Tue, May 19, 2009 at 12:23:25PM +0300, Baruch Siach wrote:

Please provide an actual description here. A brief subject
isn't enough!
 
> Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig          |    9 +
>  drivers/i2c/busses/Makefile         |    1 +
>  drivers/i2c/busses/i2c-designware.c |  587 +++++++++++++++++++++++++++++++++++
>  3 files changed, 597 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-designware.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index f1c6ca7..cef1567 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -326,6 +326,15 @@ config I2C_DAVINCI
>  	  devices such as DaVinci NIC.
>  	  For details please see http://www.ti.com/davinci
>  
> +config I2C_DESIGNWARE
> +	tristate "Synopsys DesignWare"
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  Synopsys DesignWare I2C adapter. Only master mode is supported.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-designware.
> +
>  config I2C_GPIO
>  	tristate "GPIO-based bitbanging I2C"
>  	depends on GENERIC_GPIO
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 776acb6..c2be11e 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
>  obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
>  obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
>  obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
> +obj-$(CONFIG_I2C_DESIGNWARE)	+= i2c-designware.o
>  obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
>  obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
>  obj-$(CONFIG_I2C_IBM_IIC)	+= i2c-ibm_iic.o
> diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
> new file mode 100644
> index 0000000..0138fea
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-designware.c
> @@ -0,0 +1,587 @@
> +/*
> + * Synopsys Designware I2C adapter driver (master only).
> + *
> + * Based on the TI DAVINCI I2C adapter driver.
> + *
> + * Copyright (C) 2006 Texas Instruments.
> + * Copyright (C) 2007 MontaVista Software Inc.
> + * Copyright (C) 2009 Provigent Ltd.
> + *
> + * ----------------------------------------------------------------------------
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + * ----------------------------------------------------------------------------
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/sched.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +/*
> + * Registers offset
> + */
> +#define DW_IC_CON		0x0
> +#define DW_IC_TAR		0x4
> +#define DW_IC_DATA_CMD		0x10
> +#define DW_IC_SS_SCL_HCNT	0x14
> +#define DW_IC_SS_SCL_LCNT	0x18
> +#define DW_IC_FS_SCL_HCNT	0x1c
> +#define DW_IC_FS_SCL_LCNT	0x20
> +#define DW_IC_INTR_STAT		0x2c
> +#define DW_IC_INTR_MASK		0x30
> +#define DW_IC_CLR_INTR		0x40
> +#define DW_IC_ENABLE		0x6c
> +#define DW_IC_STATUS		0x70
> +#define DW_IC_TXFLR		0x74
> +#define DW_IC_RXFLR		0x78
> +#define DW_IC_COMP_PARAM_1	0xf4
> +#define DW_IC_TX_ABRT_SOURCE	0x80
> +
> +#define DW_IC_CON_MASTER		0x1
> +#define DW_IC_CON_SPEED_STD		0x2
> +#define DW_IC_CON_SPEED_FAST		0x4
> +#define DW_IC_CON_10BITADDR_MASTER	0x10
> +#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_STOP_DET	0x200
> +
> +#define DW_IC_STATUS_ACTIVITY	0x1
> +
> +#define DW_IC_ERR_TX_ABRT	0x1
> +
> +/*
> + * status codes
> + */
> +#define STATUS_IDLE			0x0
> +#define STATUS_WRITE_IN_PROGRESS	0x1
> +#define STATUS_READ_IN_PROGRESS		0x2
> +
> +/*
> + * hardware abort codes
> + *
> + * only expected abort codes are listed here
> + * refer to the datasheet for the full list
> + */
> +#define ABRT_7B_ADDR_NOACK	0
> +#define ABRT_10ADDR1_NOACK	1
> +#define ABRT_10ADDR2_NOACK	2
> +#define ABRT_TXDATA_NOACK	3
> +#define ABRT_GCALL_NOACK	4
> +#define ABRT_GCALL_READ		5
> +#define ABRT_SBYTE_ACKDET	7
> +#define ABRT_SBYTE_NORSTRT	9
> +#define ABRT_10B_RD_NORSTRT	10
> +#define ARB_MASTER_DIS		11
> +#define ARB_LOST		12
> +
> +static char *abort_sources[] = {
> +	[ABRT_7B_ADDR_NOACK]	=
> +		"slave address not acknowledged (7bit mode)",
> +	[ABRT_10ADDR1_NOACK]	=
> +		"first address byte not acknowledged (10bit mode)",
> +	[ABRT_10ADDR2_NOACK]	=
> +		"second address byte not acknowledged (10bit mode)",
> +	[ABRT_TXDATA_NOACK]		=
> +		"data not acknowledged",
> +	[ABRT_GCALL_NOACK]		=
> +		"no acknowledgement for a general call",
> +	[ABRT_GCALL_READ]		=
> +		"read after general call",
> +	[ABRT_SBYTE_ACKDET]		=
> +		"start byte acknowledged",
> +	[ABRT_SBYTE_NORSTRT]	=
> +		"trying to send start byte when restart is disabled",
> +	[ABRT_10B_RD_NORSTRT]	=
> +		"trying to read when restart is disabled (10bit mode)",
> +	[ARB_MASTER_DIS]		=
> +		"trying to use disabled adapter",
> +	[ARB_LOST]			=
> +		"lost arbitration",
> +};
> +
> +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;
> +	struct i2c_msg		*msgs;
> +	int			msgs_num;
> +	int			msg_write_idx;
> +	int			msg_read_idx;
> +	int			msg_err;
> +	unsigned int		status;
> +	u16			abort_source;
> +	int			irq;
> +	struct i2c_adapter	adapter;
> +	unsigned int		tx_fifo_depth;
> +	unsigned int		rx_fifo_depth;
> +};
> +
> +/*
> + * This functions configures I2C and enables I2C.
> + * This function is called during I2C init function.
> + */
> +static int i2c_dw_init(struct dw_i2c_dev *dev)
> +{
> +	u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
> +	u16 ic_con;
> +
> +	/* Disable the adapter */
> +	writeb(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 */
> +			dev->base + DW_IC_SS_SCL_HCNT);
> +	writew((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 */
> +			dev->base + DW_IC_FS_SCL_HCNT);
> +	writew((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);
> +
> +	return 0;
> +}
> +
> +/*
> + * Waiting for bus not busy
> + */
> +static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
> +{
> +	unsigned long timeout;
> +
> +	timeout = jiffies + HZ;
> +	while (readb(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(dev->dev,
> +				 "timeout waiting for bus ready\n");
> +			return -ETIMEDOUT;
> +		}
> +		schedule_timeout(1);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * 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
> + * that is longer than the size of the TX FIFO.
> + */
> +static void
> +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_valid_bytes = readb(dev->base + DW_IC_TXFLR);
> +	int tx_limit = dev->tx_fifo_depth - tx_valid_bytes;
> +	int rx_limit = dev->rx_fifo_depth - readb(dev->base + DW_IC_RXFLR)
> +		- tx_valid_bytes;

hmm, sure you want tx_valid_bytes for rx_limit calculation?

> +	static u16 buf_len = 0;
> +	static u16 addr;
> +
> +	if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> +		/* Disable the adapter */
> +		writeb(0, dev->base + DW_IC_ENABLE);
> +
> +		/* set the slave (target) address */
> +		writew(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);
> +		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);
> +
> +		/* Enable the adapter */
> +		writeb(1, dev->base + DW_IC_ENABLE);
> +
> +		addr = msgs[dev->msg_write_idx].addr;
> +	}
> +
> +	for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
> +		static u8 *buf;
> +
> +		/* if target address has changed, we need to
> +		 * reprogram the target address in the i2c
> +		 * adapter when we are done with this transfer
> +		 */
> +		if (msgs[dev->msg_write_idx].addr != addr)
> +			return;
> +
> +		if (msgs[dev->msg_write_idx].len == 0) {
> +			dev_err(dev->dev,
> +				"%s: invalid message length\n", __func__);
> +			dev->msg_err = -EINVAL;
> +			return;
> +		}
> +
> +		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> +			/* new i2c_msg */
> +			buf = msgs[dev->msg_write_idx].buf;
> +			buf_len = msgs[dev->msg_write_idx].len;
> +		}
> +
> +		for (; buf_len > 0 && tx_limit > 0 && rx_limit > 0; buf_len--)
> +			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
> +				writew(0x100, dev->base + DW_IC_DATA_CMD);
> +				rx_limit--; tx_limit--;
> +			} else {
> +				writew(*buf++, dev->base + DW_IC_DATA_CMD);
> +				tx_limit--;
> +			}
> +	}
> +
> +	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
> +	if (buf_len > 0) { /* more bytes to be written */
> +		intr_mask |= DW_IC_INTR_TX_EMPTY;
> +		dev->status |= STATUS_WRITE_IN_PROGRESS;
> +	} else
> +		dev->status &= ~STATUS_WRITE_IN_PROGRESS;
> +	writew(intr_mask, dev->base + DW_IC_INTR_MASK);
> +}
> +
> +static void
> +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);
> +
> +	for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
> +		static int len;
> +		static u8 *buf;

sure you want this to be static? if so, a note explaining what it
is being used for.

> +
> +		if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
> +			continue;
> +
> +		/* different i2c client, reprogram the i2c adapter */
> +		if (msgs[dev->msg_read_idx].addr != addr)
> +			return;
> +
> +		if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
> +			len = msgs[dev->msg_read_idx].len;
> +			buf = msgs[dev->msg_read_idx].buf;
> +		}
> +
> +		for (; len > 0 && rx_valid > 0; len--, rx_valid--)
> +			*buf++ = readb(dev->base + DW_IC_DATA_CMD);
> +
> +		if (len > 0) {
> +			dev->status |= STATUS_READ_IN_PROGRESS;
> +			return;
> +		} else
> +			dev->status &= ~STATUS_READ_IN_PROGRESS;
> +	}
> +}
> +
> +/*
> + * Prepare controller for a transaction and call i2c_dw_xfer_msg
> + */
> +static int
> +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{
> +	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int ret;
> +
> +	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> +
> +	mutex_lock(&dev->lock);
> +
> +	INIT_COMPLETION(dev->cmd_complete);
> +	dev->msgs = msgs;
> +	dev->msgs_num = num;
> +	dev->cmd_err = 0;
> +	dev->msg_write_idx = 0;
> +	dev->msg_read_idx = 0;
> +	dev->msg_err = 0;
> +	dev->status = STATUS_IDLE;
> +
> +	ret = i2c_dw_wait_bus_not_busy(dev);
> +	if (ret < 0)
> +		goto done;
> +
> +	/* start the transfers */
> +	i2c_dw_xfer_msg(adap);
> +
> +	/* wait for tx to complete */
> +	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
> +	if (ret == 0) {
> +		dev_err(dev->dev, "controller timed out\n");
> +		i2c_dw_init(dev);
> +		ret = -ETIMEDOUT;
> +		goto done;
> +	} else if (ret < 0)
> +		goto done;
> +
> +	if (dev->msg_err) {
> +		ret = dev->msg_err;
> +		goto done;
> +	}
> +
> +	/* no error */
> +	if (likely(!dev->cmd_err)) {
> +		/* read rx fifo, and disable the adapter */
> +		do {
> +			i2c_dw_read(adap);
> +		} while (dev->status & STATUS_READ_IN_PROGRESS);
> +		writeb(0, dev->base + DW_IC_ENABLE);
> +		ret = num;
> +		goto done;
> +	}
> +
> +	/* We have an error */
> +	if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> +		unsigned long abort_source = dev->abort_source;
> +		int i;
> +
> +		for_each_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) {
> +		    dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
> +		}
> +	}
> +	ret = -EIO;
> +
> +done:
> +	mutex_unlock(&dev->lock);
> +
> +	return ret;
> +}
> +
> +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;
> +	u16 intr_mask;
> +
> +	i2c_dw_read(&dev->adapter);
> +	i2c_dw_xfer_msg(&dev->adapter);
> +
> +	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);
> +}
> +
> +/*
> + * Interrupt service routine. This gets called whenever an I2C interrupt
> + * occurs.
> + */
> +static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> +{
> +	struct dw_i2c_dev *dev = dev_id;
> +	u16 stat;
> +
> +	stat = readw(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->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 */
> +	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
> +		complete(&dev->cmd_complete);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct i2c_algorithm i2c_dw_algo = {
> +	.master_xfer	= i2c_dw_xfer,
> +	.functionality	= i2c_dw_func,
> +};
> +
> +static int dw_i2c_probe(struct platform_device *pdev)
> +{
> +	struct dw_i2c_dev *dev;
> +	struct i2c_adapter *adap;
> +	struct resource *mem, *irq, *ioarea;
> +	int r;
> +
> +	/* NOTE: driver uses the static register mapping */
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem) {
> +		dev_err(&pdev->dev, "no mem resource?\n");
> +		return -ENODEV;
> +	}

ENODEV is not a good error return here, it won't get repoted
to the user as the probe takes this as there was nothing there
not a 'it was there, but something went wrong'. try -EINVAL instead.

> +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (!irq) {
> +		dev_err(&pdev->dev, "no irq resource?\n");
> +		return -ENODEV;
> +	}

ditto.

> +	ioarea = request_mem_region(mem->start, (mem->end - mem->start) + 1,
> +				    pdev->name);
> +	if (!ioarea) {
> +		dev_err(&pdev->dev, "I2C region already claimed\n");
> +		return -EBUSY;
> +	}
> +
> +	dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
> +	if (!dev) {
> +		r = -ENOMEM;
> +		goto err_release_region;
> +	}
> +
> +	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->start;
> +	platform_set_drvdata(pdev, dev);
> +
> +	dev->clk = clk_get(&pdev->dev, "I2CCLK");
> +	if (IS_ERR(dev->clk)) {
> +		r = -ENODEV;
> +		goto err_free_mem;
> +	}

and again.

> +	clk_enable(dev->clk);
> +
> +	dev->base = ioremap(mem->start, mem->end - mem->start + 1);
> +	if (dev->base == NULL) {
> +		dev_err(&pdev->dev, "failure mapping io resources\n");
> +		r = -EBUSY;
> +		goto err_unuse_clocks;
> +	}
> +	dev->tx_fifo_depth =
> +		((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x00ff0000) >> 16) + 1;
> +	dev->rx_fifo_depth =
> +		((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x0000ff00) >> 8) + 1;
> +	i2c_dw_init(dev);
> +
> +	writew(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);
> +		goto err_iounmap;
> +	}
> +
> +	adap = &dev->adapter;
> +	i2c_set_adapdata(adap, dev);
> +	adap->owner = THIS_MODULE;
> +	adap->class = I2C_CLASS_HWMON;
> +	strlcpy(adap->name, "Synopsys DesignWare I2C adapter", 
> +			sizeof(adap->name));
> +	adap->algo = &i2c_dw_algo;
> +	adap->dev.parent = &pdev->dev;
> +
> +	adap->nr = pdev->id;
> +	r = i2c_add_numbered_adapter(adap);
> +	if (r) {
> +		dev_err(&pdev->dev, "failure adding adapter\n");
> +		goto err_free_irq;
> +	}
> +
> +	return 0;
> +
> +err_free_irq:
> +	free_irq(dev->irq, dev);
> +err_iounmap:
> +	iounmap(dev->base);
> +err_unuse_clocks:
> +	clk_disable(dev->clk);
> +	clk_put(dev->clk);
> +	dev->clk = NULL;
> +err_free_mem:
> +	platform_set_drvdata(pdev, NULL);
> +	put_device(&pdev->dev);
> +	kfree(dev);
> +err_release_region:
> +	release_mem_region(mem->start, (mem->end - mem->start) + 1);

there's a resource_size() call that does that job for you.

> +	return r;
> +}
> +
> +static int dw_i2c_remove(struct platform_device *pdev)
> +{
> +	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> +	struct resource *mem;
> +
> +	platform_set_drvdata(pdev, NULL);
> +	i2c_del_adapter(&dev->adapter);
> +	put_device(&pdev->dev);
> +
> +	clk_disable(dev->clk);
> +	clk_put(dev->clk);
> +	dev->clk = NULL;
> +
> +	writeb(0, dev->base + DW_IC_ENABLE);
> +	free_irq(dev->irq, dev);
> +	kfree(dev);
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(mem->start, (mem->end - mem->start) + 1);
> +	return 0;
> +}
> +
> +/* work with hotplug and coldplug */
> +MODULE_ALIAS("platform:i2c_designware");
> +
> +static struct platform_driver dw_i2c_driver = {
> +	.probe		= dw_i2c_probe,
> +	.remove		= dw_i2c_remove,
> +	.driver		= {
> +		.name	= "i2c_designware",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init dw_i2c_init_driver(void)
> +{
> +	return platform_driver_register(&dw_i2c_driver);
> +}
> +module_init(dw_i2c_init_driver);
> +
> +static void __exit dw_i2c_exit_driver(void)
> +{
> +	platform_driver_unregister(&dw_i2c_driver);
> +}
> +module_exit(dw_i2c_exit_driver);
> +
> +MODULE_AUTHOR("Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
> +MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
> +MODULE_LICENSE("GPL");

please fix the few problems found and it can be merged.


-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
       [not found]     ` <20090526213917.GH23114-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
@ 2009-05-27  4:26       ` Baruch Siach
       [not found]         ` <1243398392-1516-1-git-send-email-baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Baruch Siach @ 2009-05-27  4:26 UTC (permalink / raw)
  To: linux-i2c-u79uwXL29TY76Z2rM5mHXA; +Cc: Ben Dooks, Baruch Siach

The i2c Linux driver for the DesignWare i2c block of Synopsys, which is meant
for AMBA Peripheral Bus. This i2c block is used on SoC chips like the ARM9
based PVG610.

Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/i2c/busses/Kconfig          |    9 +
 drivers/i2c/busses/Makefile         |    1 +
 drivers/i2c/busses/i2c-designware.c |  587 +++++++++++++++++++++++++++++++++++
 3 files changed, 597 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-designware.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f1c6ca7..cef1567 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -326,6 +326,15 @@ config I2C_DAVINCI
 	  devices such as DaVinci NIC.
 	  For details please see http://www.ti.com/davinci
 
+config I2C_DESIGNWARE
+	tristate "Synopsys DesignWare"
+	help
+	  If you say yes to this option, support will be included for the
+	  Synopsys DesignWare I2C adapter. Only master mode is supported.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-designware.
+
 config I2C_GPIO
 	tristate "GPIO-based bitbanging I2C"
 	depends on GENERIC_GPIO
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 776acb6..c2be11e 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_AU1550)	+= i2c-au1550.o
 obj-$(CONFIG_I2C_BLACKFIN_TWI)	+= i2c-bfin-twi.o
 obj-$(CONFIG_I2C_CPM)		+= i2c-cpm.o
 obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci.o
+obj-$(CONFIG_I2C_DESIGNWARE)	+= i2c-designware.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
 obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
 obj-$(CONFIG_I2C_IBM_IIC)	+= i2c-ibm_iic.o
diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
new file mode 100644
index 0000000..0138fea
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware.c
@@ -0,0 +1,587 @@
+/*
+ * Synopsys Designware I2C adapter driver (master only).
+ *
+ * Based on the TI DAVINCI I2C adapter driver.
+ *
+ * Copyright (C) 2006 Texas Instruments.
+ * Copyright (C) 2007 MontaVista Software Inc.
+ * Copyright (C) 2009 Provigent Ltd.
+ *
+ * ----------------------------------------------------------------------------
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ * ----------------------------------------------------------------------------
+ *
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/sched.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+
+/*
+ * Registers offset
+ */
+#define DW_IC_CON		0x0
+#define DW_IC_TAR		0x4
+#define DW_IC_DATA_CMD		0x10
+#define DW_IC_SS_SCL_HCNT	0x14
+#define DW_IC_SS_SCL_LCNT	0x18
+#define DW_IC_FS_SCL_HCNT	0x1c
+#define DW_IC_FS_SCL_LCNT	0x20
+#define DW_IC_INTR_STAT		0x2c
+#define DW_IC_INTR_MASK		0x30
+#define DW_IC_CLR_INTR		0x40
+#define DW_IC_ENABLE		0x6c
+#define DW_IC_STATUS		0x70
+#define DW_IC_TXFLR		0x74
+#define DW_IC_RXFLR		0x78
+#define DW_IC_COMP_PARAM_1	0xf4
+#define DW_IC_TX_ABRT_SOURCE	0x80
+
+#define DW_IC_CON_MASTER		0x1
+#define DW_IC_CON_SPEED_STD		0x2
+#define DW_IC_CON_SPEED_FAST		0x4
+#define DW_IC_CON_10BITADDR_MASTER	0x10
+#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_STOP_DET	0x200
+
+#define DW_IC_STATUS_ACTIVITY	0x1
+
+#define DW_IC_ERR_TX_ABRT	0x1
+
+/*
+ * status codes
+ */
+#define STATUS_IDLE			0x0
+#define STATUS_WRITE_IN_PROGRESS	0x1
+#define STATUS_READ_IN_PROGRESS		0x2
+
+/*
+ * hardware abort codes
+ *
+ * only expected abort codes are listed here
+ * refer to the datasheet for the full list
+ */
+#define ABRT_7B_ADDR_NOACK	0
+#define ABRT_10ADDR1_NOACK	1
+#define ABRT_10ADDR2_NOACK	2
+#define ABRT_TXDATA_NOACK	3
+#define ABRT_GCALL_NOACK	4
+#define ABRT_GCALL_READ		5
+#define ABRT_SBYTE_ACKDET	7
+#define ABRT_SBYTE_NORSTRT	9
+#define ABRT_10B_RD_NORSTRT	10
+#define ARB_MASTER_DIS		11
+#define ARB_LOST		12
+
+static char *abort_sources[] = {
+	[ABRT_7B_ADDR_NOACK]	=
+		"slave address not acknowledged (7bit mode)",
+	[ABRT_10ADDR1_NOACK]	=
+		"first address byte not acknowledged (10bit mode)",
+	[ABRT_10ADDR2_NOACK]	=
+		"second address byte not acknowledged (10bit mode)",
+	[ABRT_TXDATA_NOACK]		=
+		"data not acknowledged",
+	[ABRT_GCALL_NOACK]		=
+		"no acknowledgement for a general call",
+	[ABRT_GCALL_READ]		=
+		"read after general call",
+	[ABRT_SBYTE_ACKDET]		=
+		"start byte acknowledged",
+	[ABRT_SBYTE_NORSTRT]	=
+		"trying to send start byte when restart is disabled",
+	[ABRT_10B_RD_NORSTRT]	=
+		"trying to read when restart is disabled (10bit mode)",
+	[ARB_MASTER_DIS]		=
+		"trying to use disabled adapter",
+	[ARB_LOST]			=
+		"lost arbitration",
+};
+
+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;
+	struct i2c_msg		*msgs;
+	int			msgs_num;
+	int			msg_write_idx;
+	int			msg_read_idx;
+	int			msg_err;
+	unsigned int		status;
+	u16			abort_source;
+	int			irq;
+	struct i2c_adapter	adapter;
+	unsigned int		tx_fifo_depth;
+	unsigned int		rx_fifo_depth;
+};
+
+/*
+ * This functions configures I2C and enables I2C.
+ * This function is called during I2C init function.
+ */
+static int i2c_dw_init(struct dw_i2c_dev *dev)
+{
+	u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
+	u16 ic_con;
+
+	/* Disable the adapter */
+	writeb(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 */
+			dev->base + DW_IC_SS_SCL_HCNT);
+	writew((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 */
+			dev->base + DW_IC_FS_SCL_HCNT);
+	writew((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);
+
+	return 0;
+}
+
+/*
+ * Waiting for bus not busy
+ */
+static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
+{
+	unsigned long timeout;
+
+	timeout = jiffies + HZ;
+	while (readb(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(dev->dev,
+				 "timeout waiting for bus ready\n");
+			return -ETIMEDOUT;
+		}
+		schedule_timeout(1);
+	}
+
+	return 0;
+}
+
+/*
+ * 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
+ * that is longer than the size of the TX FIFO.
+ */
+static void
+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_valid_bytes = readb(dev->base + DW_IC_TXFLR);
+	int tx_limit = dev->tx_fifo_depth - tx_valid_bytes;
+	int rx_limit = dev->rx_fifo_depth - readb(dev->base + DW_IC_RXFLR)
+		- tx_valid_bytes;
+	static u16 buf_len = 0;
+	static u16 addr;
+
+	if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
+		/* Disable the adapter */
+		writeb(0, dev->base + DW_IC_ENABLE);
+
+		/* set the slave (target) address */
+		writew(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);
+		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);
+
+		/* Enable the adapter */
+		writeb(1, dev->base + DW_IC_ENABLE);
+
+		addr = msgs[dev->msg_write_idx].addr;
+	}
+
+	for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
+		static u8 *buf;
+
+		/* if target address has changed, we need to
+		 * reprogram the target address in the i2c
+		 * adapter when we are done with this transfer
+		 */
+		if (msgs[dev->msg_write_idx].addr != addr)
+			return;
+
+		if (msgs[dev->msg_write_idx].len == 0) {
+			dev_err(dev->dev,
+				"%s: invalid message length\n", __func__);
+			dev->msg_err = -EINVAL;
+			return;
+		}
+
+		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
+			/* new i2c_msg */
+			buf = msgs[dev->msg_write_idx].buf;
+			buf_len = msgs[dev->msg_write_idx].len;
+		}
+
+		for (; buf_len > 0 && tx_limit > 0 && rx_limit > 0; buf_len--)
+			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
+				writew(0x100, dev->base + DW_IC_DATA_CMD);
+				rx_limit--; tx_limit--;
+			} else {
+				writew(*buf++, dev->base + DW_IC_DATA_CMD);
+				tx_limit--;
+			}
+	}
+
+	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
+	if (buf_len > 0) { /* more bytes to be written */
+		intr_mask |= DW_IC_INTR_TX_EMPTY;
+		dev->status |= STATUS_WRITE_IN_PROGRESS;
+	} else
+		dev->status &= ~STATUS_WRITE_IN_PROGRESS;
+	writew(intr_mask, dev->base + DW_IC_INTR_MASK);
+}
+
+static void
+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);
+
+	for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
+		static int len;
+		static u8 *buf;
+
+		if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
+			continue;
+
+		/* different i2c client, reprogram the i2c adapter */
+		if (msgs[dev->msg_read_idx].addr != addr)
+			return;
+
+		if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
+			len = msgs[dev->msg_read_idx].len;
+			buf = msgs[dev->msg_read_idx].buf;
+		}
+
+		for (; len > 0 && rx_valid > 0; len--, rx_valid--)
+			*buf++ = readb(dev->base + DW_IC_DATA_CMD);
+
+		if (len > 0) {
+			dev->status |= STATUS_READ_IN_PROGRESS;
+			return;
+		} else
+			dev->status &= ~STATUS_READ_IN_PROGRESS;
+	}
+}
+
+/*
+ * Prepare controller for a transaction and call i2c_dw_xfer_msg
+ */
+static int
+i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+	int ret;
+
+	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+
+	mutex_lock(&dev->lock);
+
+	INIT_COMPLETION(dev->cmd_complete);
+	dev->msgs = msgs;
+	dev->msgs_num = num;
+	dev->cmd_err = 0;
+	dev->msg_write_idx = 0;
+	dev->msg_read_idx = 0;
+	dev->msg_err = 0;
+	dev->status = STATUS_IDLE;
+
+	ret = i2c_dw_wait_bus_not_busy(dev);
+	if (ret < 0)
+		goto done;
+
+	/* start the transfers */
+	i2c_dw_xfer_msg(adap);
+
+	/* wait for tx to complete */
+	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
+	if (ret == 0) {
+		dev_err(dev->dev, "controller timed out\n");
+		i2c_dw_init(dev);
+		ret = -ETIMEDOUT;
+		goto done;
+	} else if (ret < 0)
+		goto done;
+
+	if (dev->msg_err) {
+		ret = dev->msg_err;
+		goto done;
+	}
+
+	/* no error */
+	if (likely(!dev->cmd_err)) {
+		/* read rx fifo, and disable the adapter */
+		do {
+			i2c_dw_read(adap);
+		} while (dev->status & STATUS_READ_IN_PROGRESS);
+		writeb(0, dev->base + DW_IC_ENABLE);
+		ret = num;
+		goto done;
+	}
+
+	/* We have an error */
+	if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
+		unsigned long abort_source = dev->abort_source;
+		int i;
+
+		for_each_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) {
+		    dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
+		}
+	}
+	ret = -EIO;
+
+done:
+	mutex_unlock(&dev->lock);
+
+	return ret;
+}
+
+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;
+	u16 intr_mask;
+
+	i2c_dw_read(&dev->adapter);
+	i2c_dw_xfer_msg(&dev->adapter);
+
+	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);
+}
+
+/*
+ * Interrupt service routine. This gets called whenever an I2C interrupt
+ * occurs.
+ */
+static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
+{
+	struct dw_i2c_dev *dev = dev_id;
+	u16 stat;
+
+	stat = readw(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->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 */
+	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
+		complete(&dev->cmd_complete);
+
+	return IRQ_HANDLED;
+}
+
+static struct i2c_algorithm i2c_dw_algo = {
+	.master_xfer	= i2c_dw_xfer,
+	.functionality	= i2c_dw_func,
+};
+
+static int dw_i2c_probe(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *dev;
+	struct i2c_adapter *adap;
+	struct resource *mem, *irq, *ioarea;
+	int r;
+
+	/* NOTE: driver uses the static register mapping */
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem) {
+		dev_err(&pdev->dev, "no mem resource?\n");
+		return -ENODEV;
+	}
+
+	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!irq) {
+		dev_err(&pdev->dev, "no irq resource?\n");
+		return -ENODEV;
+	}
+
+	ioarea = request_mem_region(mem->start, (mem->end - mem->start) + 1,
+				    pdev->name);
+	if (!ioarea) {
+		dev_err(&pdev->dev, "I2C region already claimed\n");
+		return -EBUSY;
+	}
+
+	dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
+	if (!dev) {
+		r = -ENOMEM;
+		goto err_release_region;
+	}
+
+	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->start;
+	platform_set_drvdata(pdev, dev);
+
+	dev->clk = clk_get(&pdev->dev, "I2CCLK");
+	if (IS_ERR(dev->clk)) {
+		r = -ENODEV;
+		goto err_free_mem;
+	}
+	clk_enable(dev->clk);
+
+	dev->base = ioremap(mem->start, mem->end - mem->start + 1);
+	if (dev->base == NULL) {
+		dev_err(&pdev->dev, "failure mapping io resources\n");
+		r = -EBUSY;
+		goto err_unuse_clocks;
+	}
+	dev->tx_fifo_depth =
+		((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x00ff0000) >> 16) + 1;
+	dev->rx_fifo_depth =
+		((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x0000ff00) >> 8) + 1;
+	i2c_dw_init(dev);
+
+	writew(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);
+		goto err_iounmap;
+	}
+
+	adap = &dev->adapter;
+	i2c_set_adapdata(adap, dev);
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	strlcpy(adap->name, "Synopsys DesignWare I2C adapter", 
+			sizeof(adap->name));
+	adap->algo = &i2c_dw_algo;
+	adap->dev.parent = &pdev->dev;
+
+	adap->nr = pdev->id;
+	r = i2c_add_numbered_adapter(adap);
+	if (r) {
+		dev_err(&pdev->dev, "failure adding adapter\n");
+		goto err_free_irq;
+	}
+
+	return 0;
+
+err_free_irq:
+	free_irq(dev->irq, dev);
+err_iounmap:
+	iounmap(dev->base);
+err_unuse_clocks:
+	clk_disable(dev->clk);
+	clk_put(dev->clk);
+	dev->clk = NULL;
+err_free_mem:
+	platform_set_drvdata(pdev, NULL);
+	put_device(&pdev->dev);
+	kfree(dev);
+err_release_region:
+	release_mem_region(mem->start, (mem->end - mem->start) + 1);
+
+	return r;
+}
+
+static int dw_i2c_remove(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+	struct resource *mem;
+
+	platform_set_drvdata(pdev, NULL);
+	i2c_del_adapter(&dev->adapter);
+	put_device(&pdev->dev);
+
+	clk_disable(dev->clk);
+	clk_put(dev->clk);
+	dev->clk = NULL;
+
+	writeb(0, dev->base + DW_IC_ENABLE);
+	free_irq(dev->irq, dev);
+	kfree(dev);
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(mem->start, (mem->end - mem->start) + 1);
+	return 0;
+}
+
+/* work with hotplug and coldplug */
+MODULE_ALIAS("platform:i2c_designware");
+
+static struct platform_driver dw_i2c_driver = {
+	.probe		= dw_i2c_probe,
+	.remove		= dw_i2c_remove,
+	.driver		= {
+		.name	= "i2c_designware",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int __init dw_i2c_init_driver(void)
+{
+	return platform_driver_register(&dw_i2c_driver);
+}
+module_init(dw_i2c_init_driver);
+
+static void __exit dw_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&dw_i2c_driver);
+}
+module_exit(dw_i2c_exit_driver);
+
+MODULE_AUTHOR("Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
+MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
+MODULE_LICENSE("GPL");
-- 
1.6.2.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
       [not found] ` <1242725005-6431-1-git-send-email-baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
  2009-05-26 21:39   ` Ben Dooks
@ 2009-05-27 19:23   ` Linus Walleij
       [not found]     ` <63386a3d0905271223k470bb89elbbb566b68c0d7cd7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2009-05-27 19:23 UTC (permalink / raw)
  To: Baruch Siach; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

Just some comments since I've been reading a lot of drivers lately...

2009/5/19 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
>
> Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig          |    9 +
>  drivers/i2c/busses/Makefile         |    1 +
>  drivers/i2c/busses/i2c-designware.c |  587 +++++++++++++++++++++++++++++++++++
>  3 files changed, 597 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-designware.c
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index f1c6ca7..cef1567 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -326,6 +326,15 @@ config I2C_DAVINCI
>          devices such as DaVinci NIC.
>          For details please see http://www.ti.com/davinci
>
> +config I2C_DESIGNWARE
> +       tristate "Synopsys DesignWare"
> +       help
> +         If you say yes to this option, support will be included for the
> +         Synopsys DesignWare I2C adapter. Only master mode is supported.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called i2c-designware.
> +
>  config I2C_GPIO
>        tristate "GPIO-based bitbanging I2C"
>        depends on GENERIC_GPIO
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 776acb6..c2be11e 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_AU1550)      += i2c-au1550.o
>  obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
>  obj-$(CONFIG_I2C_CPM)          += i2c-cpm.o
>  obj-$(CONFIG_I2C_DAVINCI)      += i2c-davinci.o
> +obj-$(CONFIG_I2C_DESIGNWARE)   += i2c-designware.o
>  obj-$(CONFIG_I2C_GPIO)         += i2c-gpio.o
>  obj-$(CONFIG_I2C_HIGHLANDER)   += i2c-highlander.o
>  obj-$(CONFIG_I2C_IBM_IIC)      += i2c-ibm_iic.o
> diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
> new file mode 100644
> index 0000000..0138fea
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-designware.c
> @@ -0,0 +1,587 @@
> +/*
> + * Synopsys Designware I2C adapter driver (master only).
> + *
> + * Based on the TI DAVINCI I2C adapter driver.
> + *
> + * Copyright (C) 2006 Texas Instruments.
> + * Copyright (C) 2007 MontaVista Software Inc.
> + * Copyright (C) 2009 Provigent Ltd.
> + *
> + * ----------------------------------------------------------------------------
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + * ----------------------------------------------------------------------------
> + *
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +#include <linux/errno.h>
> +#include <linux/sched.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +
> +/*
> + * Registers offset
> + */
> +#define DW_IC_CON              0x0
> +#define DW_IC_TAR              0x4
> +#define DW_IC_DATA_CMD         0x10
> +#define DW_IC_SS_SCL_HCNT      0x14
> +#define DW_IC_SS_SCL_LCNT      0x18
> +#define DW_IC_FS_SCL_HCNT      0x1c
> +#define DW_IC_FS_SCL_LCNT      0x20
> +#define DW_IC_INTR_STAT                0x2c
> +#define DW_IC_INTR_MASK                0x30
> +#define DW_IC_CLR_INTR         0x40
> +#define DW_IC_ENABLE           0x6c
> +#define DW_IC_STATUS           0x70
> +#define DW_IC_TXFLR            0x74
> +#define DW_IC_RXFLR            0x78
> +#define DW_IC_COMP_PARAM_1     0xf4
> +#define DW_IC_TX_ABRT_SOURCE   0x80
> +
> +#define DW_IC_CON_MASTER               0x1
> +#define DW_IC_CON_SPEED_STD            0x2
> +#define DW_IC_CON_SPEED_FAST           0x4
> +#define DW_IC_CON_10BITADDR_MASTER     0x10
> +#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_STOP_DET    0x200
> +
> +#define DW_IC_STATUS_ACTIVITY  0x1
> +
> +#define DW_IC_ERR_TX_ABRT      0x1
> +
> +/*
> + * status codes
> + */
> +#define STATUS_IDLE                    0x0
> +#define STATUS_WRITE_IN_PROGRESS       0x1
> +#define STATUS_READ_IN_PROGRESS                0x2
> +
> +/*
> + * hardware abort codes
> + *
> + * only expected abort codes are listed here
> + * refer to the datasheet for the full list

Write here that this is the bit number in register DW_IC_TX_ABRT_SOURCE
that's good to know!

Makes me curious why the rest of the codes are unexpected, usually
error paths handle the unexpected but hey, I can live with degrees
of unexpectedness..

> + */
> +#define ABRT_7B_ADDR_NOACK     0
> +#define ABRT_10ADDR1_NOACK     1
> +#define ABRT_10ADDR2_NOACK     2
> +#define ABRT_TXDATA_NOACK      3
> +#define ABRT_GCALL_NOACK       4
> +#define ABRT_GCALL_READ                5
> +#define ABRT_SBYTE_ACKDET      7
> +#define ABRT_SBYTE_NORSTRT     9
> +#define ABRT_10B_RD_NORSTRT    10
> +#define ARB_MASTER_DIS         11
> +#define ARB_LOST               12
> +
> +static char *abort_sources[] = {
> +       [ABRT_7B_ADDR_NOACK]    =
> +               "slave address not acknowledged (7bit mode)",
> +       [ABRT_10ADDR1_NOACK]    =
> +               "first address byte not acknowledged (10bit mode)",
> +       [ABRT_10ADDR2_NOACK]    =
> +               "second address byte not acknowledged (10bit mode)",
> +       [ABRT_TXDATA_NOACK]             =
> +               "data not acknowledged",
> +       [ABRT_GCALL_NOACK]              =
> +               "no acknowledgement for a general call",
> +       [ABRT_GCALL_READ]               =
> +               "read after general call",
> +       [ABRT_SBYTE_ACKDET]             =
> +               "start byte acknowledged",
> +       [ABRT_SBYTE_NORSTRT]    =
> +               "trying to send start byte when restart is disabled",
> +       [ABRT_10B_RD_NORSTRT]   =
> +               "trying to read when restart is disabled (10bit mode)",
> +       [ARB_MASTER_DIS]                =
> +               "trying to use disabled adapter",
> +       [ARB_LOST]                      =
> +               "lost arbitration",
> +};
> +
> +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;
> +       struct i2c_msg          *msgs;
> +       int                     msgs_num;
> +       int                     msg_write_idx;
> +       int                     msg_read_idx;
> +       int                     msg_err;
> +       unsigned int            status;
> +       u16                     abort_source;
> +       int                     irq;
> +       struct i2c_adapter      adapter;
> +       unsigned int            tx_fifo_depth;
> +       unsigned int            rx_fifo_depth;
> +};
> +
> +/*
> + * This functions configures I2C and enables I2C.
> + * This function is called during I2C init function.
> + */
> +static int i2c_dw_init(struct dw_i2c_dev *dev)

Can this be tagged with __init?

> +{
> +       u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
> +       u16 ic_con;
> +
> +       /* Disable the adapter */
> +       writeb(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 */
> +                       dev->base + DW_IC_SS_SCL_HCNT);
> +       writew((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 */
> +                       dev->base + DW_IC_FS_SCL_HCNT);
> +       writew((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);
> +
> +       return 0;
> +}
> +
> +/*
> + * Waiting for bus not busy
> + */
> +static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
> +{
> +       unsigned long timeout;
> +
> +       timeout = jiffies + HZ;

Is this timeout really valid if this device is clocked differently than
the CPU, as indicated when supplying a clk? Well, should work...

> +       while (readb(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
> +               if (time_after(jiffies, timeout)) {
> +                       dev_warn(dev->dev,
> +                                "timeout waiting for bus ready\n");
> +                       return -ETIMEDOUT;
> +               }
> +               schedule_timeout(1);
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * 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
> + * that is longer than the size of the TX FIFO.
> + */
> +static void
> +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_valid_bytes = readb(dev->base + DW_IC_TXFLR);
> +       int tx_limit = dev->tx_fifo_depth - tx_valid_bytes;
> +       int rx_limit = dev->rx_fifo_depth - readb(dev->base + DW_IC_RXFLR)
> +               - tx_valid_bytes;
> +       static u16 buf_len = 0;
> +       static u16 addr;
> +
> +       if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> +               /* Disable the adapter */
> +               writeb(0, dev->base + DW_IC_ENABLE);
> +
> +               /* set the slave (target) address */
> +               writew(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);
> +               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);
> +
> +               /* Enable the adapter */
> +               writeb(1, dev->base + DW_IC_ENABLE);
> +
> +               addr = msgs[dev->msg_write_idx].addr;
> +       }
> +
> +       for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
> +               static u8 *buf;
> +
> +               /* if target address has changed, we need to
> +                * reprogram the target address in the i2c
> +                * adapter when we are done with this transfer
> +                */
> +               if (msgs[dev->msg_write_idx].addr != addr)
> +                       return;
> +
> +               if (msgs[dev->msg_write_idx].len == 0) {
> +                       dev_err(dev->dev,
> +                               "%s: invalid message length\n", __func__);
> +                       dev->msg_err = -EINVAL;
> +                       return;
> +               }
> +
> +               if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> +                       /* new i2c_msg */
> +                       buf = msgs[dev->msg_write_idx].buf;
> +                       buf_len = msgs[dev->msg_write_idx].len;
> +               }
> +
> +               for (; buf_len > 0 && tx_limit > 0 && rx_limit > 0; buf_len--)
> +                       if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
> +                               writew(0x100, dev->base + DW_IC_DATA_CMD);
> +                               rx_limit--; tx_limit--;
> +                       } else {
> +                               writew(*buf++, dev->base + DW_IC_DATA_CMD);
> +                               tx_limit--;
> +                       }
> +       }
> +
> +       intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
> +       if (buf_len > 0) { /* more bytes to be written */
> +               intr_mask |= DW_IC_INTR_TX_EMPTY;
> +               dev->status |= STATUS_WRITE_IN_PROGRESS;
> +       } else
> +               dev->status &= ~STATUS_WRITE_IN_PROGRESS;
> +       writew(intr_mask, dev->base + DW_IC_INTR_MASK);
> +}
> +
> +static void
> +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);
> +
> +       for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
> +               static int len;
> +               static u8 *buf;
> +
> +               if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
> +                       continue;
> +
> +               /* different i2c client, reprogram the i2c adapter */
> +               if (msgs[dev->msg_read_idx].addr != addr)
> +                       return;
> +
> +               if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
> +                       len = msgs[dev->msg_read_idx].len;
> +                       buf = msgs[dev->msg_read_idx].buf;
> +               }
> +
> +               for (; len > 0 && rx_valid > 0; len--, rx_valid--)
> +                       *buf++ = readb(dev->base + DW_IC_DATA_CMD);
> +
> +               if (len > 0) {
> +                       dev->status |= STATUS_READ_IN_PROGRESS;
> +                       return;
> +               } else
> +                       dev->status &= ~STATUS_READ_IN_PROGRESS;
> +       }
> +}
> +
> +/*
> + * Prepare controller for a transaction and call i2c_dw_xfer_msg
> + */
> +static int
> +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> +{
> +       struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> +       int ret;
> +
> +       dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> +
> +       mutex_lock(&dev->lock);
> +
> +       INIT_COMPLETION(dev->cmd_complete);
> +       dev->msgs = msgs;
> +       dev->msgs_num = num;
> +       dev->cmd_err = 0;
> +       dev->msg_write_idx = 0;
> +       dev->msg_read_idx = 0;
> +       dev->msg_err = 0;
> +       dev->status = STATUS_IDLE;
> +
> +       ret = i2c_dw_wait_bus_not_busy(dev);
> +       if (ret < 0)
> +               goto done;
> +
> +       /* start the transfers */
> +       i2c_dw_xfer_msg(adap);
> +
> +       /* wait for tx to complete */
> +       ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
> +       if (ret == 0) {
> +               dev_err(dev->dev, "controller timed out\n");
> +               i2c_dw_init(dev);
> +               ret = -ETIMEDOUT;
> +               goto done;
> +       } else if (ret < 0)
> +               goto done;
> +
> +       if (dev->msg_err) {
> +               ret = dev->msg_err;
> +               goto done;
> +       }
> +
> +       /* no error */
> +       if (likely(!dev->cmd_err)) {
> +               /* read rx fifo, and disable the adapter */
> +               do {
> +                       i2c_dw_read(adap);
> +               } while (dev->status & STATUS_READ_IN_PROGRESS);
> +               writeb(0, dev->base + DW_IC_ENABLE);
> +               ret = num;
> +               goto done;
> +       }
> +
> +       /* We have an error */
> +       if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> +               unsigned long abort_source = dev->abort_source;
> +               int i;
> +
> +               for_each_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) {
> +                   dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
> +               }
> +       }
> +       ret = -EIO;
> +
> +done:
> +       mutex_unlock(&dev->lock);
> +
> +       return ret;
> +}
> +
> +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;
> +       u16 intr_mask;
> +
> +       i2c_dw_read(&dev->adapter);
> +       i2c_dw_xfer_msg(&dev->adapter);
> +
> +       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);
> +}
> +
> +/*
> + * Interrupt service routine. This gets called whenever an I2C interrupt
> + * occurs.
> + */
> +static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> +{
> +       struct dw_i2c_dev *dev = dev_id;
> +       u16 stat;
> +
> +       stat = readw(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->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 */
> +       if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
> +               complete(&dev->cmd_complete);
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static struct i2c_algorithm i2c_dw_algo = {
> +       .master_xfer    = i2c_dw_xfer,
> +       .functionality  = i2c_dw_func,
> +};
> +
> +static int dw_i2c_probe(struct platform_device *pdev)

Can you tag this with __init?

> +{
> +       struct dw_i2c_dev *dev;
> +       struct i2c_adapter *adap;
> +       struct resource *mem, *irq, *ioarea;
> +       int r;
> +
> +       /* NOTE: driver uses the static register mapping */
> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!mem) {
> +               dev_err(&pdev->dev, "no mem resource?\n");
> +               return -ENODEV;

I've seen other drivers use -ENOENT which makes sense.

> +       }
> +
> +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       if (!irq) {
> +               dev_err(&pdev->dev, "no irq resource?\n");
> +               return -ENODEV;

-ENOENT

> +       }
> +
> +       ioarea = request_mem_region(mem->start, (mem->end - mem->start) + 1,

Use resource_size(mem) instead of the last argument.

> +                                   pdev->name);
> +       if (!ioarea) {
> +               dev_err(&pdev->dev, "I2C region already claimed\n");
> +               return -EBUSY;
> +       }
> +
> +       dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
> +       if (!dev) {
> +               r = -ENOMEM;
> +               goto err_release_region;
> +       }
> +
> +       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->start;
> +       platform_set_drvdata(pdev, dev);
> +
> +       dev->clk = clk_get(&pdev->dev, "I2CCLK");
> +       if (IS_ERR(dev->clk)) {
> +               r = -ENODEV;
> +               goto err_free_mem;
> +       }
> +       clk_enable(dev->clk);
> +
> +       dev->base = ioremap(mem->start, mem->end - mem->start + 1);

resource_size()

> +       if (dev->base == NULL) {
> +               dev_err(&pdev->dev, "failure mapping io resources\n");
> +               r = -EBUSY;
> +               goto err_unuse_clocks;
> +       }
> +       dev->tx_fifo_depth =
> +               ((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x00ff0000) >> 16) + 1;
> +       dev->rx_fifo_depth =
> +               ((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x0000ff00) >> 8) + 1;
> +       i2c_dw_init(dev);
> +
> +       writew(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);
> +               goto err_iounmap;
> +       }
> +
> +       adap = &dev->adapter;
> +       i2c_set_adapdata(adap, dev);
> +       adap->owner = THIS_MODULE;
> +       adap->class = I2C_CLASS_HWMON;
> +       strlcpy(adap->name, "Synopsys DesignWare I2C adapter",
> +                       sizeof(adap->name));
> +       adap->algo = &i2c_dw_algo;
> +       adap->dev.parent = &pdev->dev;
> +
> +       adap->nr = pdev->id;
> +       r = i2c_add_numbered_adapter(adap);
> +       if (r) {
> +               dev_err(&pdev->dev, "failure adding adapter\n");
> +               goto err_free_irq;
> +       }
> +
> +       return 0;
> +
> +err_free_irq:
> +       free_irq(dev->irq, dev);
> +err_iounmap:
> +       iounmap(dev->base);
> +err_unuse_clocks:
> +       clk_disable(dev->clk);
> +       clk_put(dev->clk);
> +       dev->clk = NULL;
> +err_free_mem:
> +       platform_set_drvdata(pdev, NULL);
> +       put_device(&pdev->dev);
> +       kfree(dev);
> +err_release_region:
> +       release_mem_region(mem->start, (mem->end - mem->start) + 1);

resource_size()

> +
> +       return r;
> +}
> +
> +static int dw_i2c_remove(struct platform_device *pdev)

Can you tag this with __exit?

> +{
> +       struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> +       struct resource *mem;
> +
> +       platform_set_drvdata(pdev, NULL);
> +       i2c_del_adapter(&dev->adapter);
> +       put_device(&pdev->dev);
> +
> +       clk_disable(dev->clk);
> +       clk_put(dev->clk);
> +       dev->clk = NULL;
> +
> +       writeb(0, dev->base + DW_IC_ENABLE);
> +       free_irq(dev->irq, dev);
> +       kfree(dev);
> +
> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       release_mem_region(mem->start, (mem->end - mem->start) + 1);

resource_size()
But I usually cache important resources in the driver private struct
(struct dw_i2c_dev in this case)

> +       return 0;
> +}
> +
> +/* work with hotplug and coldplug */
> +MODULE_ALIAS("platform:i2c_designware");
> +
> +static struct platform_driver dw_i2c_driver = {
> +       .probe          = dw_i2c_probe,

Can you remove this and use platform_driver_probe() as detaile below instead?

> +       .remove         = dw_i2c_remove,

If the function can be tagges __exit, this can be tagged
__exit_p(dw_i2c_remove);

> +       .driver         = {
> +               .name   = "i2c_designware",
> +               .owner  = THIS_MODULE,
> +       },
> +};
> +
> +static int __init dw_i2c_init_driver(void)
> +{
> +       return platform_driver_register(&dw_i2c_driver);

return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe);
and remove the .probe field in the driver.

> +}
> +module_init(dw_i2c_init_driver);
> +
> +static void __exit dw_i2c_exit_driver(void)
> +{
> +       platform_driver_unregister(&dw_i2c_driver);
> +}
> +module_exit(dw_i2c_exit_driver);
> +
> +MODULE_AUTHOR("Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
> +MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
> +MODULE_LICENSE("GPL");
> --
> 1.6.2.4
>


Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
       [not found]     ` <63386a3d0905271223k470bb89elbbb566b68c0d7cd7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2009-05-28  4:10       ` Baruch Siach
  2009-05-28  7:32         ` Linus Walleij
  0 siblings, 1 reply; 12+ messages in thread
From: Baruch Siach @ 2009-05-28  4:10 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

Hi Linus,

On Wed, May 27, 2009 at 09:23:22PM +0200, Linus Walleij wrote:
> Just some comments since I've been reading a lot of drivers lately...

Thank you very much.

> 2009/5/19 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
> >
> > Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > ---
> >  drivers/i2c/busses/Kconfig          |    9 +
> >  drivers/i2c/busses/Makefile         |    1 +
> >  drivers/i2c/busses/i2c-designware.c |  587 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 597 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/i2c/busses/i2c-designware.c
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index f1c6ca7..cef1567 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -326,6 +326,15 @@ config I2C_DAVINCI
> >          devices such as DaVinci NIC.
> >          For details please see http://www.ti.com/davinci
> >
> > +config I2C_DESIGNWARE
> > +       tristate "Synopsys DesignWare"
> > +       help
> > +         If you say yes to this option, support will be included for the
> > +         Synopsys DesignWare I2C adapter. Only master mode is supported.
> > +
> > +         This driver can also be built as a module.  If so, the module
> > +         will be called i2c-designware.
> > +
> >  config I2C_GPIO
> >        tristate "GPIO-based bitbanging I2C"
> >        depends on GENERIC_GPIO
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 776acb6..c2be11e 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_I2C_AU1550)      += i2c-au1550.o
> >  obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
> >  obj-$(CONFIG_I2C_CPM)          += i2c-cpm.o
> >  obj-$(CONFIG_I2C_DAVINCI)      += i2c-davinci.o
> > +obj-$(CONFIG_I2C_DESIGNWARE)   += i2c-designware.o
> >  obj-$(CONFIG_I2C_GPIO)         += i2c-gpio.o
> >  obj-$(CONFIG_I2C_HIGHLANDER)   += i2c-highlander.o
> >  obj-$(CONFIG_I2C_IBM_IIC)      += i2c-ibm_iic.o
> > diff --git a/drivers/i2c/busses/i2c-designware.c b/drivers/i2c/busses/i2c-designware.c
> > new file mode 100644
> > index 0000000..0138fea
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-designware.c
> > @@ -0,0 +1,587 @@
> > +/*
> > + * Synopsys Designware I2C adapter driver (master only).
> > + *
> > + * Based on the TI DAVINCI I2C adapter driver.
> > + *
> > + * Copyright (C) 2006 Texas Instruments.
> > + * Copyright (C) 2007 MontaVista Software Inc.
> > + * Copyright (C) 2009 Provigent Ltd.
> > + *
> > + * ----------------------------------------------------------------------------
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + * ----------------------------------------------------------------------------
> > + *
> > + */
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/clk.h>
> > +#include <linux/errno.h>
> > +#include <linux/sched.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +
> > +/*
> > + * Registers offset
> > + */
> > +#define DW_IC_CON              0x0
> > +#define DW_IC_TAR              0x4
> > +#define DW_IC_DATA_CMD         0x10
> > +#define DW_IC_SS_SCL_HCNT      0x14
> > +#define DW_IC_SS_SCL_LCNT      0x18
> > +#define DW_IC_FS_SCL_HCNT      0x1c
> > +#define DW_IC_FS_SCL_LCNT      0x20
> > +#define DW_IC_INTR_STAT                0x2c
> > +#define DW_IC_INTR_MASK                0x30
> > +#define DW_IC_CLR_INTR         0x40
> > +#define DW_IC_ENABLE           0x6c
> > +#define DW_IC_STATUS           0x70
> > +#define DW_IC_TXFLR            0x74
> > +#define DW_IC_RXFLR            0x78
> > +#define DW_IC_COMP_PARAM_1     0xf4
> > +#define DW_IC_TX_ABRT_SOURCE   0x80
> > +
> > +#define DW_IC_CON_MASTER               0x1
> > +#define DW_IC_CON_SPEED_STD            0x2
> > +#define DW_IC_CON_SPEED_FAST           0x4
> > +#define DW_IC_CON_10BITADDR_MASTER     0x10
> > +#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_STOP_DET    0x200
> > +
> > +#define DW_IC_STATUS_ACTIVITY  0x1
> > +
> > +#define DW_IC_ERR_TX_ABRT      0x1
> > +
> > +/*
> > + * status codes
> > + */
> > +#define STATUS_IDLE                    0x0
> > +#define STATUS_WRITE_IN_PROGRESS       0x1
> > +#define STATUS_READ_IN_PROGRESS                0x2
> > +
> > +/*
> > + * hardware abort codes
> > + *
> > + * only expected abort codes are listed here
> > + * refer to the datasheet for the full list
> 
> Write here that this is the bit number in register DW_IC_TX_ABRT_SOURCE
> that's good to know!

OK.

> Makes me curious why the rest of the codes are unexpected, usually
> error paths handle the unexpected but hey, I can live with degrees
> of unexpectedness..

Aborts no. 6 and 8 are related to high seed which is not supported by this 
driver. Aborts no. 13-15 are only relevant for hardware operating as slave, 
which is not supported as well.

> > + */
> > +#define ABRT_7B_ADDR_NOACK     0
> > +#define ABRT_10ADDR1_NOACK     1
> > +#define ABRT_10ADDR2_NOACK     2
> > +#define ABRT_TXDATA_NOACK      3
> > +#define ABRT_GCALL_NOACK       4
> > +#define ABRT_GCALL_READ                5
> > +#define ABRT_SBYTE_ACKDET      7
> > +#define ABRT_SBYTE_NORSTRT     9
> > +#define ABRT_10B_RD_NORSTRT    10
> > +#define ARB_MASTER_DIS         11
> > +#define ARB_LOST               12
> > +
> > +static char *abort_sources[] = {
> > +       [ABRT_7B_ADDR_NOACK]    =
> > +               "slave address not acknowledged (7bit mode)",
> > +       [ABRT_10ADDR1_NOACK]    =
> > +               "first address byte not acknowledged (10bit mode)",
> > +       [ABRT_10ADDR2_NOACK]    =
> > +               "second address byte not acknowledged (10bit mode)",
> > +       [ABRT_TXDATA_NOACK]             =
> > +               "data not acknowledged",
> > +       [ABRT_GCALL_NOACK]              =
> > +               "no acknowledgement for a general call",
> > +       [ABRT_GCALL_READ]               =
> > +               "read after general call",
> > +       [ABRT_SBYTE_ACKDET]             =
> > +               "start byte acknowledged",
> > +       [ABRT_SBYTE_NORSTRT]    =
> > +               "trying to send start byte when restart is disabled",
> > +       [ABRT_10B_RD_NORSTRT]   =
> > +               "trying to read when restart is disabled (10bit mode)",
> > +       [ARB_MASTER_DIS]                =
> > +               "trying to use disabled adapter",
> > +       [ARB_LOST]                      =
> > +               "lost arbitration",
> > +};
> > +
> > +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;
> > +       struct i2c_msg          *msgs;
> > +       int                     msgs_num;
> > +       int                     msg_write_idx;
> > +       int                     msg_read_idx;
> > +       int                     msg_err;
> > +       unsigned int            status;
> > +       u16                     abort_source;
> > +       int                     irq;
> > +       struct i2c_adapter      adapter;
> > +       unsigned int            tx_fifo_depth;
> > +       unsigned int            rx_fifo_depth;
> > +};
> > +
> > +/*
> > + * This functions configures I2C and enables I2C.
> > + * This function is called during I2C init function.
> > + */
> > +static int i2c_dw_init(struct dw_i2c_dev *dev)
> 
> Can this be tagged with __init?

No. It's called from i2c_dw_xfer() at run time in case of timeout.

> 
> > +{
> > +       u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
> > +       u16 ic_con;
> > +
> > +       /* Disable the adapter */
> > +       writeb(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 */
> > +                       dev->base + DW_IC_SS_SCL_HCNT);
> > +       writew((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 */
> > +                       dev->base + DW_IC_FS_SCL_HCNT);
> > +       writew((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);
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * Waiting for bus not busy
> > + */
> > +static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
> > +{
> > +       unsigned long timeout;
> > +
> > +       timeout = jiffies + HZ;
> 
> Is this timeout really valid if this device is clocked differently than
> the CPU, as indicated when supplying a clk? Well, should work...

This really looks wrong. I'll look for some better way to do this.

> > +       while (readb(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
> > +               if (time_after(jiffies, timeout)) {
> > +                       dev_warn(dev->dev,
> > +                                "timeout waiting for bus ready\n");
> > +                       return -ETIMEDOUT;
> > +               }
> > +               schedule_timeout(1);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * 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
> > + * that is longer than the size of the TX FIFO.
> > + */
> > +static void
> > +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_valid_bytes = readb(dev->base + DW_IC_TXFLR);
> > +       int tx_limit = dev->tx_fifo_depth - tx_valid_bytes;
> > +       int rx_limit = dev->rx_fifo_depth - readb(dev->base + DW_IC_RXFLR)
> > +               - tx_valid_bytes;
> > +       static u16 buf_len = 0;
> > +       static u16 addr;
> > +
> > +       if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> > +               /* Disable the adapter */
> > +               writeb(0, dev->base + DW_IC_ENABLE);
> > +
> > +               /* set the slave (target) address */
> > +               writew(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);
> > +               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);
> > +
> > +               /* Enable the adapter */
> > +               writeb(1, dev->base + DW_IC_ENABLE);
> > +
> > +               addr = msgs[dev->msg_write_idx].addr;
> > +       }
> > +
> > +       for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
> > +               static u8 *buf;
> > +
> > +               /* if target address has changed, we need to
> > +                * reprogram the target address in the i2c
> > +                * adapter when we are done with this transfer
> > +                */
> > +               if (msgs[dev->msg_write_idx].addr != addr)
> > +                       return;
> > +
> > +               if (msgs[dev->msg_write_idx].len == 0) {
> > +                       dev_err(dev->dev,
> > +                               "%s: invalid message length\n", __func__);
> > +                       dev->msg_err = -EINVAL;
> > +                       return;
> > +               }
> > +
> > +               if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
> > +                       /* new i2c_msg */
> > +                       buf = msgs[dev->msg_write_idx].buf;
> > +                       buf_len = msgs[dev->msg_write_idx].len;
> > +               }
> > +
> > +               for (; buf_len > 0 && tx_limit > 0 && rx_limit > 0; buf_len--)
> > +                       if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
> > +                               writew(0x100, dev->base + DW_IC_DATA_CMD);
> > +                               rx_limit--; tx_limit--;
> > +                       } else {
> > +                               writew(*buf++, dev->base + DW_IC_DATA_CMD);
> > +                               tx_limit--;
> > +                       }
> > +       }
> > +
> > +       intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
> > +       if (buf_len > 0) { /* more bytes to be written */
> > +               intr_mask |= DW_IC_INTR_TX_EMPTY;
> > +               dev->status |= STATUS_WRITE_IN_PROGRESS;
> > +       } else
> > +               dev->status &= ~STATUS_WRITE_IN_PROGRESS;
> > +       writew(intr_mask, dev->base + DW_IC_INTR_MASK);
> > +}
> > +
> > +static void
> > +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);
> > +
> > +       for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
> > +               static int len;
> > +               static u8 *buf;
> > +
> > +               if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))
> > +                       continue;
> > +
> > +               /* different i2c client, reprogram the i2c adapter */
> > +               if (msgs[dev->msg_read_idx].addr != addr)
> > +                       return;
> > +
> > +               if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
> > +                       len = msgs[dev->msg_read_idx].len;
> > +                       buf = msgs[dev->msg_read_idx].buf;
> > +               }
> > +
> > +               for (; len > 0 && rx_valid > 0; len--, rx_valid--)
> > +                       *buf++ = readb(dev->base + DW_IC_DATA_CMD);
> > +
> > +               if (len > 0) {
> > +                       dev->status |= STATUS_READ_IN_PROGRESS;
> > +                       return;
> > +               } else
> > +                       dev->status &= ~STATUS_READ_IN_PROGRESS;
> > +       }
> > +}
> > +
> > +/*
> > + * Prepare controller for a transaction and call i2c_dw_xfer_msg
> > + */
> > +static int
> > +i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> > +{
> > +       struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
> > +       int ret;
> > +
> > +       dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
> > +
> > +       mutex_lock(&dev->lock);
> > +
> > +       INIT_COMPLETION(dev->cmd_complete);
> > +       dev->msgs = msgs;
> > +       dev->msgs_num = num;
> > +       dev->cmd_err = 0;
> > +       dev->msg_write_idx = 0;
> > +       dev->msg_read_idx = 0;
> > +       dev->msg_err = 0;
> > +       dev->status = STATUS_IDLE;
> > +
> > +       ret = i2c_dw_wait_bus_not_busy(dev);
> > +       if (ret < 0)
> > +               goto done;
> > +
> > +       /* start the transfers */
> > +       i2c_dw_xfer_msg(adap);
> > +
> > +       /* wait for tx to complete */
> > +       ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);
> > +       if (ret == 0) {
> > +               dev_err(dev->dev, "controller timed out\n");
> > +               i2c_dw_init(dev);
> > +               ret = -ETIMEDOUT;
> > +               goto done;
> > +       } else if (ret < 0)
> > +               goto done;
> > +
> > +       if (dev->msg_err) {
> > +               ret = dev->msg_err;
> > +               goto done;
> > +       }
> > +
> > +       /* no error */
> > +       if (likely(!dev->cmd_err)) {
> > +               /* read rx fifo, and disable the adapter */
> > +               do {
> > +                       i2c_dw_read(adap);
> > +               } while (dev->status & STATUS_READ_IN_PROGRESS);
> > +               writeb(0, dev->base + DW_IC_ENABLE);
> > +               ret = num;
> > +               goto done;
> > +       }
> > +
> > +       /* We have an error */
> > +       if (dev->cmd_err == DW_IC_ERR_TX_ABRT) {
> > +               unsigned long abort_source = dev->abort_source;
> > +               int i;
> > +
> > +               for_each_bit(i, &abort_source, ARRAY_SIZE(abort_sources)) {
> > +                   dev_err(dev->dev, "%s: %s\n", __func__, abort_sources[i]);
> > +               }
> > +       }
> > +       ret = -EIO;
> > +
> > +done:
> > +       mutex_unlock(&dev->lock);
> > +
> > +       return ret;
> > +}
> > +
> > +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;
> > +       u16 intr_mask;
> > +
> > +       i2c_dw_read(&dev->adapter);
> > +       i2c_dw_xfer_msg(&dev->adapter);
> > +
> > +       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);
> > +}
> > +
> > +/*
> > + * Interrupt service routine. This gets called whenever an I2C interrupt
> > + * occurs.
> > + */
> > +static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
> > +{
> > +       struct dw_i2c_dev *dev = dev_id;
> > +       u16 stat;
> > +
> > +       stat = readw(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->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 */
> > +       if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
> > +               complete(&dev->cmd_complete);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static struct i2c_algorithm i2c_dw_algo = {
> > +       .master_xfer    = i2c_dw_xfer,
> > +       .functionality  = i2c_dw_func,
> > +};
> > +
> > +static int dw_i2c_probe(struct platform_device *pdev)
> 
> Can you tag this with __init?

OK. __devinit is probably better.

> > +{
> > +       struct dw_i2c_dev *dev;
> > +       struct i2c_adapter *adap;
> > +       struct resource *mem, *irq, *ioarea;
> > +       int r;
> > +
> > +       /* NOTE: driver uses the static register mapping */
> > +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!mem) {
> > +               dev_err(&pdev->dev, "no mem resource?\n");
> > +               return -ENODEV;
> 
> I've seen other drivers use -ENOENT which makes sense.

OK.

> > +       }
> > +
> > +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +       if (!irq) {
> > +               dev_err(&pdev->dev, "no irq resource?\n");
> > +               return -ENODEV;
> 
> -ENOENT

OK.

> > +       }
> > +
> > +       ioarea = request_mem_region(mem->start, (mem->end - mem->start) + 1,
> 
> Use resource_size(mem) instead of the last argument.

OK.

> > +                                   pdev->name);
> > +       if (!ioarea) {
> > +               dev_err(&pdev->dev, "I2C region already claimed\n");
> > +               return -EBUSY;
> > +       }
> > +
> > +       dev = kzalloc(sizeof(struct dw_i2c_dev), GFP_KERNEL);
> > +       if (!dev) {
> > +               r = -ENOMEM;
> > +               goto err_release_region;
> > +       }
> > +
> > +       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->start;
> > +       platform_set_drvdata(pdev, dev);
> > +
> > +       dev->clk = clk_get(&pdev->dev, "I2CCLK");
> > +       if (IS_ERR(dev->clk)) {
> > +               r = -ENODEV;
> > +               goto err_free_mem;
> > +       }
> > +       clk_enable(dev->clk);
> > +
> > +       dev->base = ioremap(mem->start, mem->end - mem->start + 1);
> 
> resource_size()

OK.

> > +       if (dev->base == NULL) {
> > +               dev_err(&pdev->dev, "failure mapping io resources\n");
> > +               r = -EBUSY;
> > +               goto err_unuse_clocks;
> > +       }
> > +       dev->tx_fifo_depth =
> > +               ((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x00ff0000) >> 16) + 1;
> > +       dev->rx_fifo_depth =
> > +               ((readl(dev->base + DW_IC_COMP_PARAM_1) & 0x0000ff00) >> 8) + 1;
> > +       i2c_dw_init(dev);
> > +
> > +       writew(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);
> > +               goto err_iounmap;
> > +       }
> > +
> > +       adap = &dev->adapter;
> > +       i2c_set_adapdata(adap, dev);
> > +       adap->owner = THIS_MODULE;
> > +       adap->class = I2C_CLASS_HWMON;
> > +       strlcpy(adap->name, "Synopsys DesignWare I2C adapter",
> > +                       sizeof(adap->name));
> > +       adap->algo = &i2c_dw_algo;
> > +       adap->dev.parent = &pdev->dev;
> > +
> > +       adap->nr = pdev->id;
> > +       r = i2c_add_numbered_adapter(adap);
> > +       if (r) {
> > +               dev_err(&pdev->dev, "failure adding adapter\n");
> > +               goto err_free_irq;
> > +       }
> > +
> > +       return 0;
> > +
> > +err_free_irq:
> > +       free_irq(dev->irq, dev);
> > +err_iounmap:
> > +       iounmap(dev->base);
> > +err_unuse_clocks:
> > +       clk_disable(dev->clk);
> > +       clk_put(dev->clk);
> > +       dev->clk = NULL;
> > +err_free_mem:
> > +       platform_set_drvdata(pdev, NULL);
> > +       put_device(&pdev->dev);
> > +       kfree(dev);
> > +err_release_region:
> > +       release_mem_region(mem->start, (mem->end - mem->start) + 1);
> 
> resource_size()

OK.

> > +
> > +       return r;
> > +}
> > +
> > +static int dw_i2c_remove(struct platform_device *pdev)
> 
> Can you tag this with __exit?

Or __devexit. OK.

> > +{
> > +       struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> > +       struct resource *mem;
> > +
> > +       platform_set_drvdata(pdev, NULL);
> > +       i2c_del_adapter(&dev->adapter);
> > +       put_device(&pdev->dev);
> > +
> > +       clk_disable(dev->clk);
> > +       clk_put(dev->clk);
> > +       dev->clk = NULL;
> > +
> > +       writeb(0, dev->base + DW_IC_ENABLE);
> > +       free_irq(dev->irq, dev);
> > +       kfree(dev);
> > +
> > +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       release_mem_region(mem->start, (mem->end - mem->start) + 1);
> 
> resource_size()
> But I usually cache important resources in the driver private struct
> (struct dw_i2c_dev in this case)

OK.

> > +       return 0;
> > +}
> > +
> > +/* work with hotplug and coldplug */
> > +MODULE_ALIAS("platform:i2c_designware");
> > +
> > +static struct platform_driver dw_i2c_driver = {
> > +       .probe          = dw_i2c_probe,
> 
> Can you remove this and use platform_driver_probe() as detaile below instead?

OK.

> > +       .remove         = dw_i2c_remove,
> 
> If the function can be tagges __exit, this can be tagged
> __exit_p(dw_i2c_remove);

OK.

> > +       .driver         = {
> > +               .name   = "i2c_designware",
> > +               .owner  = THIS_MODULE,
> > +       },
> > +};
> > +
> > +static int __init dw_i2c_init_driver(void)
> > +{
> > +       return platform_driver_register(&dw_i2c_driver);
> 
> return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe);
> and remove the .probe field in the driver.

OK.

> > +}
> > +module_init(dw_i2c_init_driver);
> > +
> > +static void __exit dw_i2c_exit_driver(void)
> > +{
> > +       platform_driver_unregister(&dw_i2c_driver);
> > +}
> > +module_exit(dw_i2c_exit_driver);
> > +
> > +MODULE_AUTHOR("Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
> > +MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.6.2.4
> >
> 
> 
> Yours,
> Linus Walleij

baruch

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
  2009-05-28  4:10       ` Baruch Siach
@ 2009-05-28  7:32         ` Linus Walleij
       [not found]           ` <63386a3d0905280032x7f427213t2b9b027d5be2a472-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2009-05-28  7:32 UTC (permalink / raw)
  To: Baruch Siach; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

2009/5/28 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:

>> > +static int dw_i2c_probe(struct platform_device *pdev)
>>
>> Can you tag this with __init?
>
> OK. __devinit is probably better.

(and same for exit)

Look in include/linux/init.h for the use of these macros. There
are many, many erroneous drivers in the kernel, and I think it
is mainly due to the fact that __devinit looks like it has something
to do with "devices", while it does not.

It should be named __hotpluginit or something so one can see
what it actually is.

I don't think any platform_device tagged with __devinit is
correct really, platform_device:s cannot  be hotplugged that's
against the definition of a platform_device.

I should make more cleanup patches regarding this...

Linus

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
       [not found]           ` <63386a3d0905280032x7f427213t2b9b027d5be2a472-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2009-05-31  5:23             ` Baruch Siach
  2009-05-31 18:47               ` Linus Walleij
  2009-06-01 22:53               ` Ben Dooks
  0 siblings, 2 replies; 12+ messages in thread
From: Baruch Siach @ 2009-05-31  5:23 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

Hi Linus,

On Thu, May 28, 2009 at 09:32:37AM +0200, Linus Walleij wrote:
> 2009/5/28 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
> 
> >> > +static int dw_i2c_probe(struct platform_device *pdev)
> >>
> >> Can you tag this with __init?
> >
> > OK. __devinit is probably better.
> 
> I don't think any platform_device tagged with __devinit is
> correct really, platform_device:s cannot  be hotplugged that's
> against the definition of a platform_device.

Doesn't this __devinit have something to do with the ability to compile the 
driver as a module (as is the case here)? See for example 
drivers/watchdog/omap_wdt.c and commit 
0e3912c75f42986c17d955542247bf04c6eef738

> I should make more cleanup patches regarding this...

baruch

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
  2009-05-31  5:23             ` Baruch Siach
@ 2009-05-31 18:47               ` Linus Walleij
       [not found]                 ` <63386a3d0905311147h5a92bb3cy6659db914acd476e-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2009-06-01 22:53               ` Ben Dooks
  1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2009-05-31 18:47 UTC (permalink / raw)
  To: Baruch Siach; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

2009/5/31 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
> On Thu, May 28, 2009 at 09:32:37AM +0200, Linus Walleij wrote:
>> 2009/5/28 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
>>
>> >> > +static int dw_i2c_probe(struct platform_device *pdev)
>> >>
>> >> Can you tag this with __init?
>> >
>> > OK. __devinit is probably better.
>>
>> I don't think any platform_device tagged with __devinit is
>> correct really, platform_device:s cannot  be hotplugged that's
>> against the definition of a platform_device.
>
> Doesn't this __devinit have something to do with the ability to compile the
> driver as a module (as is the case here)? See for example
> drivers/watchdog/omap_wdt.c and commit
> 0e3912c75f42986c17d955542247bf04c6eef738

In that case I doubt that even module_init() could be __init, as it is.

The linkage handling of these symbols is defined in:
include/asm-generic/vmlinux.lds.h

As you can see:

#ifdef CONFIG_HOTPLUG
#define DEV_KEEP(sec)    *(.dev##sec)
#define DEV_DISCARD(sec)
#else
#define DEV_KEEP(sec)
#define DEV_DISCARD(sec) *(.dev##sec)
#endif

Further down the file you can see how this is handled in different
kinds of symbols. It is kept if CONFIG_HOTPLUG is set, it is moved
into the DATA_DATA section and kept in memory (so that it can be
reused when devices are plugged/unplugged) else it is discarded
after init.

In practice this means that if you device is _NOT_ hot-pluggable, the
kernel will still nevertheless keep all the code in __devinit and
__devexit around which means that __devinit code cannot be
free:ed after init and it increases the footprint of your kernel for no
good reason.

Linus

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
       [not found]                 ` <63386a3d0905311147h5a92bb3cy6659db914acd476e-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2009-06-01  3:43                   ` Baruch Siach
  0 siblings, 0 replies; 12+ messages in thread
From: Baruch Siach @ 2009-06-01  3:43 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

Hi Linus,

On Sun, May 31, 2009 at 08:47:31PM +0200, Linus Walleij wrote:
> 2009/5/31 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
> > On Thu, May 28, 2009 at 09:32:37AM +0200, Linus Walleij wrote:
> >> 2009/5/28 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
> >>
> >> >> > +static int dw_i2c_probe(struct platform_device *pdev)
> >> >>
> >> >> Can you tag this with __init?
> >> >
> >> > OK. __devinit is probably better.
> >>
> >> I don't think any platform_device tagged with __devinit is
> >> correct really, platform_device:s cannot  be hotplugged that's
> >> against the definition of a platform_device.
> >
> > Doesn't this __devinit have something to do with the ability to compile the
> > driver as a module (as is the case here)? See for example
> > drivers/watchdog/omap_wdt.c and commit
> > 0e3912c75f42986c17d955542247bf04c6eef738
> 
> ...
>
> In practice this means that if you device is _NOT_ hot-pluggable, the
> kernel will still nevertheless keep all the code in __devinit and
> __devexit around which means that __devinit code cannot be
> free:ed after init and it increases the footprint of your kernel for no
> good reason.

Thanks for your detailed explanation. I'll go for __init then.

baruch

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
  2009-05-31  5:23             ` Baruch Siach
  2009-05-31 18:47               ` Linus Walleij
@ 2009-06-01 22:53               ` Ben Dooks
       [not found]                 ` <20090601225354.GB18453-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
  1 sibling, 1 reply; 12+ messages in thread
From: Ben Dooks @ 2009-06-01 22:53 UTC (permalink / raw)
  To: Baruch Siach; +Cc: Linus Walleij, linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

On Sun, May 31, 2009 at 08:23:45AM +0300, Baruch Siach wrote:
> Hi Linus,
> 
> On Thu, May 28, 2009 at 09:32:37AM +0200, Linus Walleij wrote:
> > 2009/5/28 Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>:
> > 
> > >> > +static int dw_i2c_probe(struct platform_device *pdev)
> > >>
> > >> Can you tag this with __init?
> > >
> > > OK. __devinit is probably better.
> > 
> > I don't think any platform_device tagged with __devinit is
> > correct really, platform_device:s cannot  be hotplugged that's
> > against the definition of a platform_device.

I disagree, just beucase they aren't in general doesn't mean they
cannot be. I have been looking at an implementation which uses
hotplug to generate devices from a board description file.

It also makes life difficult if you want to unbind/bind device via
sysfs. basically, __devinit and __devexit, are in my view the correct
things to use here.
 
> Doesn't this __devinit have something to do with the ability to compile the 
> driver as a module (as is the case here)? See for example 
> drivers/watchdog/omap_wdt.c and commit 
> 0e3912c75f42986c17d955542247bf04c6eef738

This is an example where __devinit/_devexit are being used proplerly.
 
> > I should make more cleanup patches regarding this...
> 
> baruch
> 
> -- 
>                                                      ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
       [not found]                 ` <20090601225354.GB18453-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
@ 2009-06-02  6:57                   ` Linus Walleij
  0 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2009-06-02  6:57 UTC (permalink / raw)
  To: Ben Dooks; +Cc: Baruch Siach, linux-i2c-u79uwXL29TY76Z2rM5mHXA

2009/6/2 Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>:

>> > I don't think any platform_device tagged with __devinit is
>> > correct really, platform_device:s cannot  be hotplugged that's
>> > against the definition of a platform_device.
>
> I disagree, just beucase they aren't in general doesn't mean they
> cannot be. I have been looking at an implementation which uses
> hotplug to generate devices from a board description file.

OK I stand corrected!

Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] i2c: driver for the Synopsys DesignWare I2C controller
       [not found]         ` <1243398392-1516-1-git-send-email-baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
@ 2009-06-13  9:28           ` Ben Dooks
  0 siblings, 0 replies; 12+ messages in thread
From: Ben Dooks @ 2009-06-13  9:28 UTC (permalink / raw)
  To: Baruch Siach; +Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA, Ben Dooks

Notes:

1) failed to fix resource_size()
2) No explanation over use of static in loop

Please sort these out and post a new patch.

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2009-06-13  9:28 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-05-19  9:23 [PATCH] i2c: driver for the Synopsys DesignWare I2C controller Baruch Siach
     [not found] ` <1242725005-6431-1-git-send-email-baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
2009-05-26 21:39   ` Ben Dooks
     [not found]     ` <20090526213917.GH23114-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
2009-05-27  4:26       ` Baruch Siach
     [not found]         ` <1243398392-1516-1-git-send-email-baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
2009-06-13  9:28           ` Ben Dooks
2009-05-27 19:23   ` Linus Walleij
     [not found]     ` <63386a3d0905271223k470bb89elbbb566b68c0d7cd7-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-05-28  4:10       ` Baruch Siach
2009-05-28  7:32         ` Linus Walleij
     [not found]           ` <63386a3d0905280032x7f427213t2b9b027d5be2a472-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-05-31  5:23             ` Baruch Siach
2009-05-31 18:47               ` Linus Walleij
     [not found]                 ` <63386a3d0905311147h5a92bb3cy6659db914acd476e-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-06-01  3:43                   ` Baruch Siach
2009-06-01 22:53               ` Ben Dooks
     [not found]                 ` <20090601225354.GB18453-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
2009-06-02  6:57                   ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.