[PATCH] i2c: tegra: Add i2c support

[PATCH] i2c: tegra: Add i2c support

[Mark Brown]

From: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>

Signed-off-by: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---
 drivers/i2c/busses/Kconfig     |    7 +
 drivers/i2c/busses/Makefile    |    1 +
 drivers/i2c/busses/i2c-tegra.c |  665 ++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-tegra.h      |   25 ++
 4 files changed, 698 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-tegra.c
 create mode 100644 include/linux/i2c-tegra.h

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 30f8dbd..b76a2a4 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -607,6 +607,13 @@ config I2C_STU300
 	  This driver can also be built as a module. If so, the module
 	  will be called i2c-stu300.
 
+config I2C_TEGRA
+	tristate "NVIDIA Tegra internal I2C controller"
+	depends on ARCH_TEGRA
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C controller embedded in NVIDIA Tegra SOCs
+
 config I2C_VERSATILE
 	tristate "ARM Versatile/Realview I2C bus support"
 	depends on ARCH_VERSATILE || ARCH_REALVIEW || ARCH_VEXPRESS
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 3c630b7..f505253 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
 obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
 obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
+obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
 obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
 obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
new file mode 100644
index 0000000..cfa0084
--- /dev/null
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -0,0 +1,665 @@
+/*
+ * drivers/i2c/busses/i2c-tegra.c
+ *
+ * Copyright (C) 2010 Google, Inc.
+ * Author: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/i2c-tegra.h>
+
+#include <asm/unaligned.h>
+
+#include <mach/clk.h>
+
+#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
+#define BYTES_PER_FIFO_WORD 4
+
+#define I2C_CNFG				0x000
+#define I2C_CNFG_PACKET_MODE_EN			(1<<10)
+#define I2C_CNFG_NEW_MASTER_FSM			(1<<11)
+#define I2C_SL_CNFG				0x020
+#define I2C_SL_CNFG_NEWSL			(1<<2)
+#define I2C_SL_ADDR1				0x02c
+#define I2C_TX_FIFO				0x050
+#define I2C_RX_FIFO				0x054
+#define I2C_PACKET_TRANSFER_STATUS		0x058
+#define I2C_FIFO_CONTROL			0x05c
+#define I2C_FIFO_CONTROL_TX_FLUSH		(1<<1)
+#define I2C_FIFO_CONTROL_RX_FLUSH		(1<<0)
+#define I2C_FIFO_CONTROL_TX_TRIG_SHIFT		5
+#define I2C_FIFO_CONTROL_RX_TRIG_SHIFT		2
+#define I2C_FIFO_STATUS				0x060
+#define I2C_FIFO_STATUS_TX_MASK			0xF0
+#define I2C_FIFO_STATUS_TX_SHIFT		4
+#define I2C_FIFO_STATUS_RX_MASK			0x0F
+#define I2C_FIFO_STATUS_RX_SHIFT		0
+#define I2C_INT_MASK				0x064
+#define I2C_INT_STATUS				0x068
+#define I2C_INT_PACKET_XFER_COMPLETE		(1<<7)
+#define I2C_INT_ALL_PACKETS_XFER_COMPLETE	(1<<6)
+#define I2C_INT_TX_FIFO_OVERFLOW		(1<<5)
+#define I2C_INT_RX_FIFO_UNDERFLOW		(1<<4)
+#define I2C_INT_NO_ACK				(1<<3)
+#define I2C_INT_ARBITRATION_LOST		(1<<2)
+#define I2C_INT_TX_FIFO_DATA_REQ		(1<<1)
+#define I2C_INT_RX_FIFO_DATA_REQ		(1<<0)
+#define I2C_CLK_DIVISOR				0x06c
+
+#define DVC_CTRL_REG1				0x000
+#define DVC_CTRL_REG1_INTR_EN			(1<<10)
+#define DVC_CTRL_REG2				0x004
+#define DVC_CTRL_REG3				0x008
+#define DVC_CTRL_REG3_SW_PROG			(1<<26)
+#define DVC_CTRL_REG3_I2C_DONE_INTR_EN		(1<<30)
+#define DVC_STATUS				0x00c
+#define DVC_STATUS_I2C_DONE_INTR		(1<<30)
+
+#define I2C_ERR_NONE				0x00
+#define I2C_ERR_NO_ACK				0x01
+#define I2C_ERR_ARBITRATION_LOST		0x02
+
+#define PACKET_HEADER0_HEADER_SIZE_SHIFT	28
+#define PACKET_HEADER0_PACKET_ID_SHIFT		16
+#define PACKET_HEADER0_CONT_ID_SHIFT		12
+#define PACKET_HEADER0_PROTOCOL_I2C		(1<<4)
+
+#define I2C_HEADER_HIGHSPEED_MODE		(1<<22)
+#define I2C_HEADER_CONT_ON_NAK			(1<<21)
+#define I2C_HEADER_SEND_START_BYTE		(1<<20)
+#define I2C_HEADER_READ				(1<<19)
+#define I2C_HEADER_10BIT_ADDR			(1<<18)
+#define I2C_HEADER_IE_ENABLE			(1<<17)
+#define I2C_HEADER_REPEAT_START			(1<<16)
+#define I2C_HEADER_MASTER_ADDR_SHIFT		12
+#define I2C_HEADER_SLAVE_ADDR_SHIFT		1
+
+struct tegra_i2c_dev {
+	struct device *dev;
+	struct i2c_adapter adapter;
+	struct clk *clk;
+	struct clk *i2c_clk;
+	struct resource *iomem;
+	void __iomem *base;
+	int cont_id;
+	int irq;
+	int is_dvc;
+	struct completion msg_complete;
+	int msg_err;
+	u8 *msg_buf;
+	size_t msg_buf_remaining;
+	int msg_read;
+	int msg_transfer_complete;
+	unsigned long bus_clk_rate;
+	bool is_suspended;
+};
+
+static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
+{
+	writel(val, i2c_dev->base + reg);
+}
+
+static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
+{
+	return readl(i2c_dev->base + reg);
+}
+
+/*
+ * i2c_writel and i2c_readl will offset the register if necessary to talk
+ * to the I2C block inside the DVC block
+ */
+static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
+{
+	if (i2c_dev->is_dvc)
+		reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
+	writel(val, i2c_dev->base + reg);
+}
+
+static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
+{
+	if (i2c_dev->is_dvc)
+		reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
+	return readl(i2c_dev->base + reg);
+}
+
+static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
+{
+	u32 int_mask = i2c_readl(i2c_dev, I2C_INT_MASK);
+	int_mask &= ~mask;
+	i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
+}
+
+static void tegra_i2c_unmask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
+{
+	u32 int_mask = i2c_readl(i2c_dev, I2C_INT_MASK);
+	int_mask |= mask;
+	i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
+}
+
+static void tegra_i2c_set_clk(struct tegra_i2c_dev *i2c_dev, unsigned int freq)
+{
+	clk_set_rate(i2c_dev->clk, freq * 8);
+}
+
+static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
+{
+	unsigned long timeout = jiffies + HZ;
+	u32 val = i2c_readl(i2c_dev, I2C_FIFO_CONTROL);
+	val |= I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH;
+	i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
+
+	while (i2c_readl(i2c_dev, I2C_FIFO_CONTROL) &
+		(I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH)) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
+			return -ETIMEDOUT;
+		}
+		msleep(1);
+	}
+	return 0;
+}
+
+static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val;
+	int rx_fifo_avail;
+	int word;
+	u8 *buf = i2c_dev->msg_buf;
+	size_t buf_remaining = i2c_dev->msg_buf_remaining;
+	int words_to_transfer;
+
+	val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
+	rx_fifo_avail = (val & I2C_FIFO_STATUS_RX_MASK) >>
+		I2C_FIFO_STATUS_RX_SHIFT;
+
+	words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
+	if (words_to_transfer > rx_fifo_avail)
+		words_to_transfer = rx_fifo_avail;
+
+	for (word = 0; word < words_to_transfer; word++) {
+		val = i2c_readl(i2c_dev, I2C_RX_FIFO);
+		put_unaligned_le32(val, buf);
+		buf += BYTES_PER_FIFO_WORD;
+		buf_remaining -= BYTES_PER_FIFO_WORD;
+		rx_fifo_avail--;
+	}
+
+	if (rx_fifo_avail > 0 && buf_remaining > 0) {
+		int bytes_to_transfer = buf_remaining;
+		int byte;
+		BUG_ON(bytes_to_transfer > 3);
+		val = i2c_readl(i2c_dev, I2C_RX_FIFO);
+		for (byte = 0; byte < bytes_to_transfer; byte++) {
+			*buf++ = val & 0xFF;
+			val >>= 8;
+		}
+		buf_remaining -= bytes_to_transfer;
+		rx_fifo_avail--;
+	}
+	BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
+	i2c_dev->msg_buf_remaining = buf_remaining;
+	i2c_dev->msg_buf = buf;
+	return 0;
+}
+
+static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val;
+	int tx_fifo_avail;
+	int word;
+	u8 *buf = i2c_dev->msg_buf;
+	size_t buf_remaining = i2c_dev->msg_buf_remaining;
+	int words_to_transfer;
+
+	val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
+	tx_fifo_avail = (val & I2C_FIFO_STATUS_TX_MASK) >>
+		I2C_FIFO_STATUS_TX_SHIFT;
+
+	words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
+	if (words_to_transfer > tx_fifo_avail)
+		words_to_transfer = tx_fifo_avail;
+
+	for (word = 0; word < words_to_transfer; word++) {
+		val = get_unaligned_le32(buf);
+		i2c_writel(i2c_dev, val, I2C_TX_FIFO);
+		buf += BYTES_PER_FIFO_WORD;
+		buf_remaining -= BYTES_PER_FIFO_WORD;
+		tx_fifo_avail--;
+	}
+
+	if (tx_fifo_avail > 0 && buf_remaining > 0) {
+		int bytes_to_transfer = buf_remaining;
+		int byte;
+		BUG_ON(bytes_to_transfer > 3);
+		val = 0;
+		for (byte = 0; byte < bytes_to_transfer; byte++)
+			val |= (*buf++) << (byte * 8);
+		i2c_writel(i2c_dev, val, I2C_TX_FIFO);
+		buf_remaining -= bytes_to_transfer;
+		tx_fifo_avail--;
+	}
+	BUG_ON(tx_fifo_avail > 0 && buf_remaining > 0);
+	i2c_dev->msg_buf_remaining = buf_remaining;
+	i2c_dev->msg_buf = buf;
+	return 0;
+}
+
+/*
+ * One of the Tegra I2C blocks is inside the DVC (Digital Voltage Controller)
+ * block.  This block is identical to the rest of the I2C blocks, except that
+ * it only supports master mode, it has registers moved around, and it needs
+ * some extra init to get it into I2C mode.  The register moves are handled
+ * by i2c_readl and i2c_writel
+ */
+static void tegra_dvc_init(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val = 0;
+	val = dvc_readl(i2c_dev, DVC_CTRL_REG3);
+	val |= DVC_CTRL_REG3_SW_PROG;
+	val |= DVC_CTRL_REG3_I2C_DONE_INTR_EN;
+	dvc_writel(i2c_dev, val, DVC_CTRL_REG3);
+
+	val = dvc_readl(i2c_dev, DVC_CTRL_REG1);
+	val |= DVC_CTRL_REG1_INTR_EN;
+	dvc_writel(i2c_dev, val, DVC_CTRL_REG1);
+}
+
+static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val;
+	int err = 0;
+
+	clk_enable(i2c_dev->clk);
+
+	tegra_periph_reset_assert(i2c_dev->clk);
+	udelay(2);
+	tegra_periph_reset_deassert(i2c_dev->clk);
+
+	if (i2c_dev->is_dvc)
+		tegra_dvc_init(i2c_dev);
+
+	val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN;
+	i2c_writel(i2c_dev, val, I2C_CNFG);
+	i2c_writel(i2c_dev, 0, I2C_INT_MASK);
+	tegra_i2c_set_clk(i2c_dev, i2c_dev->bus_clk_rate);
+
+	val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
+		0 << I2C_FIFO_CONTROL_RX_TRIG_SHIFT;
+	i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
+
+	if (tegra_i2c_flush_fifos(i2c_dev))
+		err = -ETIMEDOUT;
+
+	clk_disable(i2c_dev->clk);
+	return 0;
+}
+
+static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
+{
+	u32 status;
+	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+	struct tegra_i2c_dev *i2c_dev = dev_id;
+
+	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
+
+	if (status == 0) {
+		dev_warn(i2c_dev->dev, "irq status 0 %08x\n",
+			i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS));
+		return IRQ_HANDLED;
+	}
+
+	if (unlikely(status & status_err)) {
+		if (status & I2C_INT_NO_ACK)
+			i2c_dev->msg_err |= I2C_ERR_NO_ACK;
+		if (status & I2C_INT_ARBITRATION_LOST)
+			i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
+		complete(&i2c_dev->msg_complete);
+		goto err;
+	}
+
+	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
+		if (i2c_dev->msg_buf_remaining)
+			tegra_i2c_empty_rx_fifo(i2c_dev);
+		else
+			BUG();
+	}
+
+	if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
+		if (i2c_dev->msg_buf_remaining)
+			tegra_i2c_fill_tx_fifo(i2c_dev);
+		else
+			tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
+	}
+
+	if (status & I2C_INT_PACKET_XFER_COMPLETE)
+		i2c_dev->msg_transfer_complete = 1;
+
+	if (i2c_dev->msg_transfer_complete && !i2c_dev->msg_buf_remaining)
+		complete(&i2c_dev->msg_complete);
+	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
+	if (i2c_dev->is_dvc)
+		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
+	return IRQ_HANDLED;
+err:
+	/* An error occured, mask all interrupts */
+	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
+		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
+		I2C_INT_RX_FIFO_DATA_REQ);
+	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
+	return IRQ_HANDLED;
+}
+
+static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
+	struct i2c_msg *msg, int stop)
+{
+	u32 packet_header;
+	u32 int_mask;
+	int ret;
+
+	tegra_i2c_flush_fifos(i2c_dev);
+	i2c_writel(i2c_dev, 0xFF, I2C_INT_STATUS);
+
+	if (msg->len == 0)
+		return -EINVAL;
+
+	i2c_dev->msg_buf = msg->buf;
+	i2c_dev->msg_buf_remaining = msg->len;
+	i2c_dev->msg_err = I2C_ERR_NONE;
+	i2c_dev->msg_transfer_complete = 0;
+	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
+	INIT_COMPLETION(i2c_dev->msg_complete);
+
+	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
+			PACKET_HEADER0_PROTOCOL_I2C |
+			(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
+			(1 << PACKET_HEADER0_PACKET_ID_SHIFT);
+	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+
+	packet_header = msg->len - 1;
+	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+
+	packet_header = msg->addr << I2C_HEADER_SLAVE_ADDR_SHIFT;
+	packet_header |= I2C_HEADER_IE_ENABLE;
+	if (msg->flags & I2C_M_TEN)
+		packet_header |= I2C_HEADER_10BIT_ADDR;
+	if (msg->flags & I2C_M_IGNORE_NAK)
+		packet_header |= I2C_HEADER_CONT_ON_NAK;
+	if (msg->flags & I2C_M_NOSTART)
+		packet_header |= I2C_HEADER_REPEAT_START;
+	if (msg->flags & I2C_M_RD)
+		packet_header |= I2C_HEADER_READ;
+	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+
+	if (!(msg->flags & I2C_M_RD))
+		tegra_i2c_fill_tx_fifo(i2c_dev);
+
+	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+	if (msg->flags & I2C_M_RD)
+		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
+	else if (i2c_dev->msg_buf_remaining)
+		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+	tegra_i2c_unmask_irq(i2c_dev, int_mask);
+	pr_debug("unmasked irq: %02x\n", i2c_readl(i2c_dev, I2C_INT_MASK));
+
+	ret = wait_for_completion_timeout(&i2c_dev->msg_complete, TEGRA_I2C_TIMEOUT);
+	tegra_i2c_mask_irq(i2c_dev, int_mask);
+
+	if (WARN_ON(ret == 0)) {
+		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
+
+		tegra_i2c_init(i2c_dev);
+		return -ETIMEDOUT;
+	}
+
+	pr_debug("transfer complete: %d %d %d\n", ret, completion_done(&i2c_dev->msg_complete), i2c_dev->msg_err);
+
+	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
+		return 0;
+
+	tegra_i2c_init(i2c_dev);
+	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
+		if (msg->flags & I2C_M_IGNORE_NAK)
+			return 0;
+		return -EREMOTEIO;
+	}
+
+	return -EIO;
+}
+
+static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+	int num)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
+	int i;
+	int ret = 0;
+
+	if (i2c_dev->is_suspended)
+		return -EBUSY;
+
+	clk_enable(i2c_dev->clk);
+	for (i = 0; i < num; i++) {
+		int stop = (i == (num - 1)) ? 1  : 0;
+		ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], stop);
+		if (ret)
+			break;
+	}
+	clk_disable(i2c_dev->clk);
+	return ret ?: i;
+}
+
+static u32 tegra_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C;
+}
+
+static const struct i2c_algorithm tegra_i2c_algo = {
+	.master_xfer	= tegra_i2c_xfer,
+	.functionality	= tegra_i2c_func,
+};
+
+static int tegra_i2c_probe(struct platform_device *pdev)
+{
+	struct tegra_i2c_dev *i2c_dev;
+	struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
+	struct resource *res;
+	struct resource *iomem;
+	struct clk *clk;
+	struct clk *i2c_clk;
+	void *base;
+	int irq;
+	int ret = 0;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no mem resource?\n");
+		return -ENODEV;
+	}
+	iomem = request_mem_region(res->start, resource_size(res), pdev->name);
+	if (!iomem) {
+		dev_err(&pdev->dev, "I2C region already claimed\n");
+		return -EBUSY;
+	}
+
+	base = ioremap(iomem->start, resource_size(iomem));
+	if (!base) {
+		dev_err(&pdev->dev, "Can't ioremap I2C region\n");
+		return -ENOMEM;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no irq resource?\n");
+		ret = -ENODEV;
+		goto err_iounmap;
+	}
+	irq = res->start;
+
+	clk = clk_get(&pdev->dev, NULL);
+	if (!clk) {
+		ret = -ENOMEM;
+		goto err_release_region;
+	}
+
+	i2c_clk = clk_get(&pdev->dev, "i2c");
+	if (!i2c_clk) {
+		ret = -ENOMEM;
+		goto err_clk_put;
+	}
+
+	i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
+	if (!i2c_dev) {
+		ret = -ENOMEM;
+		goto err_i2c_clk_put;
+	}
+
+	i2c_dev->base = base;
+	i2c_dev->clk = clk;
+	i2c_dev->i2c_clk = i2c_clk;
+	i2c_dev->iomem = iomem;
+	i2c_dev->adapter.algo = &tegra_i2c_algo;
+	i2c_dev->irq = irq;
+	i2c_dev->cont_id = pdev->id;
+	i2c_dev->dev = &pdev->dev;
+	i2c_dev->bus_clk_rate = pdata ? pdata->bus_clk_rate : 100000;
+
+	if (pdev->id == 3)
+		i2c_dev->is_dvc = 1;
+	init_completion(&i2c_dev->msg_complete);
+
+	platform_set_drvdata(pdev, i2c_dev);
+
+	ret = tegra_i2c_init(i2c_dev);
+	if (ret)
+		goto err_free;
+
+	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, IRQF_DISABLED,
+		pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+		goto err_free;
+	}
+
+	clk_enable(i2c_dev->i2c_clk);
+
+	i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
+	i2c_dev->adapter.owner = THIS_MODULE;
+	i2c_dev->adapter.class = I2C_CLASS_HWMON;
+	strlcpy(i2c_dev->adapter.name, "Tegra I2C adapter",
+		sizeof(i2c_dev->adapter.name));
+	i2c_dev->adapter.algo = &tegra_i2c_algo;
+	i2c_dev->adapter.dev.parent = &pdev->dev;
+	i2c_dev->adapter.nr = pdev->id;
+
+	ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add I2C adapter\n");
+		goto err_free_irq;
+	}
+
+	return 0;
+err_free_irq:
+	free_irq(i2c_dev->irq, i2c_dev);
+err_free:
+	kfree(i2c_dev);
+err_i2c_clk_put:
+	clk_put(i2c_clk);
+err_clk_put:
+	clk_put(clk);
+err_release_region:
+	release_mem_region(iomem->start, resource_size(iomem));
+err_iounmap:
+	iounmap(base);
+	return ret;
+}
+
+static int tegra_i2c_remove(struct platform_device *pdev)
+{
+	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+	i2c_del_adapter(&i2c_dev->adapter);
+	free_irq(i2c_dev->irq, i2c_dev);
+	clk_put(i2c_dev->i2c_clk);
+	clk_put(i2c_dev->clk);
+	release_mem_region(i2c_dev->iomem->start,
+		resource_size(i2c_dev->iomem));
+	iounmap(i2c_dev->base);
+	kfree(i2c_dev);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int tegra_i2c_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_lock_adapter(&i2c_dev->adapter);
+	i2c_dev->is_suspended = true;
+	i2c_unlock_adapter(&i2c_dev->adapter);
+
+	return 0;
+}
+
+static int tegra_i2c_resume(struct platform_device *pdev)
+{
+	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+	int ret;
+
+	i2c_lock_adapter(&i2c_dev->adapter);
+
+	ret = tegra_i2c_init(i2c_dev);
+
+	if (ret) {
+		i2c_unlock_adapter(&i2c_dev->adapter);
+		return ret;
+	}
+
+	i2c_dev->is_suspended = false;
+
+	i2c_unlock_adapter(&i2c_dev->adapter);
+
+	return 0;
+}
+#endif
+
+static struct platform_driver tegra_i2c_driver = {
+	.probe   = tegra_i2c_probe,
+	.remove  = tegra_i2c_remove,
+#ifdef CONFIG_PM
+	.suspend = tegra_i2c_suspend,
+	.resume  = tegra_i2c_resume,
+#endif
+	.driver  = {
+		.name  = "tegra-i2c",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init tegra_i2c_init_driver(void)
+{
+	return platform_driver_register(&tegra_i2c_driver);
+}
+
+static void __exit tegra_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&tegra_i2c_driver);
+}
+
+subsys_initcall(tegra_i2c_init_driver);
+module_exit(tegra_i2c_exit_driver);
diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
new file mode 100644
index 0000000..9c85da4
--- /dev/null
+++ b/include/linux/i2c-tegra.h
@@ -0,0 +1,25 @@
+/*
+ * drivers/i2c/busses/i2c-tegra.c
+ *
+ * Copyright (C) 2010 Google, Inc.
+ * Author: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ */
+
+#ifndef _LINUX_I2C_TEGRA_H
+#define _LINUX_I2C_TEGRA_H
+
+struct tegra_i2c_platform_data {
+	unsigned long bus_clk_rate;
+};
+
+#endif /* _LINUX_I2C_TEGRA_H */
-- 
1.7.2.3

Re: [PATCH] i2c: tegra: Add i2c support

[Ben Dooks]

On 08/02/11 12:44, Mark Brown wrote:
> From: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>

Some sort of explanation here would have been nice.

> Signed-off-by: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig     |    7 +
>  drivers/i2c/busses/Makefile    |    1 +
>  drivers/i2c/busses/i2c-tegra.c |  665 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c-tegra.h      |   25 ++
>  4 files changed, 698 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-tegra.c
>  create mode 100644 include/linux/i2c-tegra.h
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 30f8dbd..b76a2a4 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig

Fine.

> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 3c630b7..f505253 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile

Fine.

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> new file mode 100644
> index 0000000..cfa0084
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -0,0 +1,665 @@
> +/*
> + * drivers/i2c/busses/i2c-tegra.c
> + *
> + * Copyright (C) 2010 Google, Inc.
> + * Author: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/i2c.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/slab.h>
> +#include <linux/i2c-tegra.h>
> +
> +#include <asm/unaligned.h>
> +
> +#include <mach/clk.h>

I don't think you should be including


> +
> +struct tegra_i2c_dev {
> +	struct device *dev;
> +	struct i2c_adapter adapter;
> +	struct clk *clk;
> +	struct clk *i2c_clk;
> +	struct resource *iomem;
> +	void __iomem *base;
> +	int cont_id;
> +	int irq;
> +	int is_dvc;
> +	struct completion msg_complete;
> +	int msg_err;
> +	u8 *msg_buf;
> +	size_t msg_buf_remaining;
> +	int msg_read;
> +	int msg_transfer_complete;
> +	unsigned long bus_clk_rate;
> +	bool is_suspended;
> +};

would have been nice to have some kerneldoc for this.

> +/*
> + * i2c_writel and i2c_readl will offset the register if necessary to talk
> + * to the I2C block inside the DVC block
> + */
> +static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
> +{
> +	if (i2c_dev->is_dvc)
> +		reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
> +	writel(val, i2c_dev->base + reg);
> +}
> +
> +static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
> +{
> +	if (i2c_dev->is_dvc)
> +		reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
> +	return readl(i2c_dev->base + reg);
> +}

as a note, would have been better to wrap up the address changes into
one place to ensure that they're consistent.

[snip]

> +static void tegra_i2c_set_clk(struct tegra_i2c_dev *i2c_dev, unsigned int freq)
> +{
> +	clk_set_rate(i2c_dev->clk, freq * 8);
> +}

hmm, really need a separate function?

> +static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
> +{
> +	unsigned long timeout = jiffies + HZ;
> +	u32 val = i2c_readl(i2c_dev, I2C_FIFO_CONTROL);
> +	val |= I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH;
> +	i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
> +
> +	while (i2c_readl(i2c_dev, I2C_FIFO_CONTROL) &
> +		(I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH)) {
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
> +			return -ETIMEDOUT;
> +		}
> +		msleep(1);
> +	}
> +	return 0;
> +}
> +
> +static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
> +{
> +	u32 val;
> +	int rx_fifo_avail;
> +	int word;
> +	u8 *buf = i2c_dev->msg_buf;
> +	size_t buf_remaining = i2c_dev->msg_buf_remaining;
> +	int words_to_transfer;
> +
> +	val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
> +	rx_fifo_avail = (val & I2C_FIFO_STATUS_RX_MASK) >>
> +		I2C_FIFO_STATUS_RX_SHIFT;
> +
> +	words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
> +	if (words_to_transfer > rx_fifo_avail)
> +		words_to_transfer = rx_fifo_avail;
> +
> +	for (word = 0; word < words_to_transfer; word++) {
> +		val = i2c_readl(i2c_dev, I2C_RX_FIFO);
> +		put_unaligned_le32(val, buf);
> +		buf += BYTES_PER_FIFO_WORD;
> +		buf_remaining -= BYTES_PER_FIFO_WORD;
> +		rx_fifo_avail--;
> +	}

you know, there's a readsl() function that does this.

> +
> +	if (rx_fifo_avail > 0 && buf_remaining > 0) {
> +		int bytes_to_transfer = buf_remaining;
> +		int byte;
> +		BUG_ON(bytes_to_transfer > 3);
> +		val = i2c_readl(i2c_dev, I2C_RX_FIFO);
> +		for (byte = 0; byte < bytes_to_transfer; byte++) {
> +			*buf++ = val & 0xFF;
> +			val >>= 8;
> +		}
> +		buf_remaining -= bytes_to_transfer;
> +		rx_fifo_avail--;
> +	}
> +	BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
> +	i2c_dev->msg_buf_remaining = buf_remaining;
> +	i2c_dev->msg_buf = buf;
> +	return 0;
> +}

this whole function gives me the creeps, is there any reason why
we can't use the readsl or similar functions for this?

> +static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
> +{
> +	u32 val;
> +	int tx_fifo_avail;
> +	int word;
> +	u8 *buf = i2c_dev->msg_buf;
> +	size_t buf_remaining = i2c_dev->msg_buf_remaining;
> +	int words_to_transfer;
> +
> +	val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
> +	tx_fifo_avail = (val & I2C_FIFO_STATUS_TX_MASK) >>
> +		I2C_FIFO_STATUS_TX_SHIFT;
> +
> +	words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
> +	if (words_to_transfer > tx_fifo_avail)
> +		words_to_transfer = tx_fifo_avail;
> +
> +	for (word = 0; word < words_to_transfer; word++) {
> +		val = get_unaligned_le32(buf);
> +		i2c_writel(i2c_dev, val, I2C_TX_FIFO);
> +		buf += BYTES_PER_FIFO_WORD;
> +		buf_remaining -= BYTES_PER_FIFO_WORD;
> +		tx_fifo_avail--;
> +	}
> +
> +	if (tx_fifo_avail > 0 && buf_remaining > 0) {
> +		int bytes_to_transfer = buf_remaining;
> +		int byte;
> +		BUG_ON(bytes_to_transfer > 3);
> +		val = 0;
> +		for (byte = 0; byte < bytes_to_transfer; byte++)
> +			val |= (*buf++) << (byte * 8);
> +		i2c_writel(i2c_dev, val, I2C_TX_FIFO);
> +		buf_remaining -= bytes_to_transfer;
> +		tx_fifo_avail--;
> +	}
> +	BUG_ON(tx_fifo_avail > 0 && buf_remaining > 0);
> +	i2c_dev->msg_buf_remaining = buf_remaining;
> +	i2c_dev->msg_buf = buf;
> +	return 0;
> +}

And the same goes for this one too.

> +static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
> +{
> +	u32 val;
> +	int err = 0;
> +
> +	clk_enable(i2c_dev->clk);
> +
> +	tegra_periph_reset_assert(i2c_dev->clk);
> +	udelay(2);
> +	tegra_periph_reset_deassert(i2c_dev->clk);
> +
> +	if (i2c_dev->is_dvc)
> +		tegra_dvc_init(i2c_dev);
> +
> +	val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN;
> +	i2c_writel(i2c_dev, val, I2C_CNFG);
> +	i2c_writel(i2c_dev, 0, I2C_INT_MASK);
> +	tegra_i2c_set_clk(i2c_dev, i2c_dev->bus_clk_rate);
> +
> +	val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
> +		0 << I2C_FIFO_CONTROL_RX_TRIG_SHIFT;
> +	i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
> +
> +	if (tegra_i2c_flush_fifos(i2c_dev))
> +		err = -ETIMEDOUT;
> +
> +	clk_disable(i2c_dev->clk);
> +	return 0;
> +}

err never gets returned. please remove or return.

> +static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
> +{
> +	u32 status;
> +	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> +	struct tegra_i2c_dev *i2c_dev = dev_id;
> +
> +	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
> +
> +	if (status == 0) {
> +		dev_warn(i2c_dev->dev, "irq status 0 %08x\n",
> +			i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS));
> +		return IRQ_HANDLED;

maybe return IRQ_UNHANDED?

> +	if (WARN_ON(ret == 0)) {

suppose a WARN_ON() is OK here.


> +		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
> +
> +		tegra_i2c_init(i2c_dev);
> +		return -ETIMEDOUT;
> +	}
> +
> +	pr_debug("transfer complete: %d %d %d\n", ret, completion_done(&i2c_dev->msg_complete), i2c_dev->msg_err);

not dev_dbg()?

> +	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
> +		return 0;
> +
> +	tegra_i2c_init(i2c_dev);
> +	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
> +		if (msg->flags & I2C_M_IGNORE_NAK)
> +			return 0;
> +		return -EREMOTEIO;
> +	}
> +
> +	return -EIO;
> +}

think that's fine.

[snip]

> +static int tegra_i2c_probe(struct platform_device *pdev)
> +{
> +	struct tegra_i2c_dev *i2c_dev;
> +	struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
> +	struct resource *res;
> +	struct resource *iomem;
> +	struct clk *clk;
> +	struct clk *i2c_clk;
> +	void *base;
> +	int irq;
> +	int ret = 0;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "no mem resource?\n");
> +		return -ENODEV;
> +	}

-ENODEV gets ignored by the upper layer, maybe find something else to
return.

> +	iomem = request_mem_region(res->start, resource_size(res), pdev->name);
> +	if (!iomem) {
> +		dev_err(&pdev->dev, "I2C region already claimed\n");
> +		return -EBUSY;
> +	}
> +
> +	base = ioremap(iomem->start, resource_size(iomem));
> +	if (!base) {
> +		dev_err(&pdev->dev, "Can't ioremap I2C region\n");

would prefer Cannot

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

No need for ? after this.

> +		ret = -ENODEV;

and see again.

> +		goto err_iounmap;
> +	}
> +	irq = res->start;
> +
> +	clk = clk_get(&pdev->dev, NULL);
> +	if (!clk) {

you've departed from no dev_err() here.

> +		ret = -ENOMEM;
> +		goto err_release_region;
> +	}
> +


> +	i2c_clk = clk_get(&pdev->dev, "i2c");
> +	if (!i2c_clk) {

see again.

> +		ret = -ENOMEM;
> +		goto err_clk_put;
> +	}
> +
> +	i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
> +	if (!i2c_dev) {
> +		ret = -ENOMEM;
> +		goto err_i2c_clk_put;
> +	}
> +
> +	i2c_dev->base = base;
> +	i2c_dev->clk = clk;
> +	i2c_dev->i2c_clk = i2c_clk;
> +	i2c_dev->iomem = iomem;
> +	i2c_dev->adapter.algo = &tegra_i2c_algo;
> +	i2c_dev->irq = irq;
> +	i2c_dev->cont_id = pdev->id;
> +	i2c_dev->dev = &pdev->dev;
> +	i2c_dev->bus_clk_rate = pdata ? pdata->bus_clk_rate : 100000;
> +
> +	if (pdev->id == 3)
> +		i2c_dev->is_dvc = 1;
> +	init_completion(&i2c_dev->msg_complete);
> +
> +	platform_set_drvdata(pdev, i2c_dev);
> +
> +	ret = tegra_i2c_init(i2c_dev);
> +	if (ret)
> +		goto err_free;

does tegra_i2c_init() print an appropriate error?

> +
> +	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, IRQF_DISABLED,
> +		pdev->name, i2c_dev);

hmm, do you really want to be running IRQF_DISABLED?

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> +		goto err_free;
> +	}
> +
> +	clk_enable(i2c_dev->i2c_clk);
> +
> +	i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
> +	i2c_dev->adapter.owner = THIS_MODULE;
> +	i2c_dev->adapter.class = I2C_CLASS_HWMON;
> +	strlcpy(i2c_dev->adapter.name, "Tegra I2C adapter",
> +		sizeof(i2c_dev->adapter.name));
> +	i2c_dev->adapter.algo = &tegra_i2c_algo;
> +	i2c_dev->adapter.dev.parent = &pdev->dev;
> +	i2c_dev->adapter.nr = pdev->id;
> +
> +	ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to add I2C adapter\n");
> +		goto err_free_irq;
> +	}
> +
> +	return 0;
> +err_free_irq:
> +	free_irq(i2c_dev->irq, i2c_dev);
> +err_free:
> +	kfree(i2c_dev);
> +err_i2c_clk_put:
> +	clk_put(i2c_clk);
> +err_clk_put:
> +	clk_put(clk);
> +err_release_region:
> +	release_mem_region(iomem->start, resource_size(iomem));
> +err_iounmap:
> +	iounmap(base);
> +	return ret;
> +}

[snip]

how about MODULE_* decelerations at the end of this, with license, etc?

> diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
> new file mode 100644
> index 0000000..9c85da4
> --- /dev/null

looks ok.

could do with a few tidies before inclusion.

-- 
Ben

RE: [PATCH] i2c: tegra: Add i2c support

[Stephen Warren]

On 08/02/11 12:44, Mark Brown wrote:
> ...
> +		goto err_iounmap;
> +	}
> +	irq = res->start;
> +
> +	clk = clk_get(&pdev->dev, NULL);
> +	if (!clk) {

This should be:

if (!IS_ERR(clk)) {

> +		ret = -ENOMEM;

This should probably be:

ret = PTR_ERR(clk);

although Ben made some comments in his review re: certain error codes being
ignored by the higher layers; I'm not sure if that applies here or not.

> +		goto err_release_region;
> +	}
> +

-- 
nvpublic

Re: [PATCH] i2c: tegra: Add i2c support

[Colin Cross]

On Tue, Feb 15, 2011 at 3:48 PM, Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
> On 08/02/11 12:44, Mark Brown wrote:
>> From: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>
> Some sort of explanation here would have been nice.
Will fix

>> Signed-off-by: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
>> ---
>>  drivers/i2c/busses/Kconfig     |    7 +
>>  drivers/i2c/busses/Makefile    |    1 +
>>  drivers/i2c/busses/i2c-tegra.c |  665 ++++++++++++++++++++++++++++++++++++++++
>>  include/linux/i2c-tegra.h      |   25 ++
>>  4 files changed, 698 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/i2c/busses/i2c-tegra.c
>>  create mode 100644 include/linux/i2c-tegra.h
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 30f8dbd..b76a2a4 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>
> Fine.
>
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 3c630b7..f505253 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>
> Fine.
>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> new file mode 100644
>> index 0000000..cfa0084
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -0,0 +1,665 @@
>> +/*
>> + * drivers/i2c/busses/i2c-tegra.c
>> + *
>> + * Copyright (C) 2010 Google, Inc.
>> + * Author: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/init.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/i2c.h>
>> +#include <linux/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/delay.h>
>> +#include <linux/slab.h>
>> +#include <linux/i2c-tegra.h>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <mach/clk.h>
>
> I don't think you should be including
mach/clk.h provides the definitions of tegra_assert_periph_reset.  The
Tegra resets are part of the clock controller, and the I2C bus
requires resets at various times, including after a NACK.  It does not
export any of the private clock implementation.

>> +
>> +struct tegra_i2c_dev {
>> +     struct device *dev;
>> +     struct i2c_adapter adapter;
>> +     struct clk *clk;
>> +     struct clk *i2c_clk;
>> +     struct resource *iomem;
>> +     void __iomem *base;
>> +     int cont_id;
>> +     int irq;
>> +     int is_dvc;
>> +     struct completion msg_complete;
>> +     int msg_err;
>> +     u8 *msg_buf;
>> +     size_t msg_buf_remaining;
>> +     int msg_read;
>> +     int msg_transfer_complete;
>> +     unsigned long bus_clk_rate;
>> +     bool is_suspended;
>> +};
>
> would have been nice to have some kerneldoc for this.
Will do

>> +/*
>> + * i2c_writel and i2c_readl will offset the register if necessary to talk
>> + * to the I2C block inside the DVC block
>> + */
>> +static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
>> +{
>> +     if (i2c_dev->is_dvc)
>> +             reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
>> +     writel(val, i2c_dev->base + reg);
>> +}
>> +
>> +static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
>> +{
>> +     if (i2c_dev->is_dvc)
>> +             reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
>> +     return readl(i2c_dev->base + reg);
>> +}
>
> as a note, would have been better to wrap up the address changes into
> one place to ensure that they're consistent.
Will fix

> [snip]
>
>> +static void tegra_i2c_set_clk(struct tegra_i2c_dev *i2c_dev, unsigned int freq)
>> +{
>> +     clk_set_rate(i2c_dev->clk, freq * 8);
>> +}
>
> hmm, really need a separate function?
Will fix

>> +static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>> +{
>> +     unsigned long timeout = jiffies + HZ;
>> +     u32 val = i2c_readl(i2c_dev, I2C_FIFO_CONTROL);
>> +     val |= I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH;
>> +     i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
>> +
>> +     while (i2c_readl(i2c_dev, I2C_FIFO_CONTROL) &
>> +             (I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH)) {
>> +             if (time_after(jiffies, timeout)) {
>> +                     dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
>> +                     return -ETIMEDOUT;
>> +             }
>> +             msleep(1);
>> +     }
>> +     return 0;
>> +}
>> +
>> +static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
>> +{
>> +     u32 val;
>> +     int rx_fifo_avail;
>> +     int word;
>> +     u8 *buf = i2c_dev->msg_buf;
>> +     size_t buf_remaining = i2c_dev->msg_buf_remaining;
>> +     int words_to_transfer;
>> +
>> +     val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
>> +     rx_fifo_avail = (val & I2C_FIFO_STATUS_RX_MASK) >>
>> +             I2C_FIFO_STATUS_RX_SHIFT;
>> +
>> +     words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
>> +     if (words_to_transfer > rx_fifo_avail)
>> +             words_to_transfer = rx_fifo_avail;
>> +
>> +     for (word = 0; word < words_to_transfer; word++) {
>> +             val = i2c_readl(i2c_dev, I2C_RX_FIFO);
>> +             put_unaligned_le32(val, buf);
>> +             buf += BYTES_PER_FIFO_WORD;
>> +             buf_remaining -= BYTES_PER_FIFO_WORD;
>> +             rx_fifo_avail--;
>> +     }
>
> you know, there's a readsl() function that does this.
I think readsl can't handle the possibly unaligned buf pointer.

>> +
>> +     if (rx_fifo_avail > 0 && buf_remaining > 0) {
>> +             int bytes_to_transfer = buf_remaining;
>> +             int byte;
>> +             BUG_ON(bytes_to_transfer > 3);
>> +             val = i2c_readl(i2c_dev, I2C_RX_FIFO);
>> +             for (byte = 0; byte < bytes_to_transfer; byte++) {
>> +                     *buf++ = val & 0xFF;
>> +                     val >>= 8;
>> +             }
>> +             buf_remaining -= bytes_to_transfer;
>> +             rx_fifo_avail--;
>> +     }
>> +     BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
>> +     i2c_dev->msg_buf_remaining = buf_remaining;
>> +     i2c_dev->msg_buf = buf;
>> +     return 0;
>> +}
>
> this whole function gives me the creeps, is there any reason why
> we can't use the readsl or similar functions for this?
Same here - readsl can't handle the alignment requirements, readsb
can't  handle the required 32 bit register read, and the bytes in the
same word but after the end buf may not be part of buf, so byte writes
to buf are required.

>> +static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
>> +{
>> +     u32 val;
>> +     int tx_fifo_avail;
>> +     int word;
>> +     u8 *buf = i2c_dev->msg_buf;
>> +     size_t buf_remaining = i2c_dev->msg_buf_remaining;
>> +     int words_to_transfer;
>> +
>> +     val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
>> +     tx_fifo_avail = (val & I2C_FIFO_STATUS_TX_MASK) >>
>> +             I2C_FIFO_STATUS_TX_SHIFT;
>> +
>> +     words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
>> +     if (words_to_transfer > tx_fifo_avail)
>> +             words_to_transfer = tx_fifo_avail;
>> +
>> +     for (word = 0; word < words_to_transfer; word++) {
>> +             val = get_unaligned_le32(buf);
>> +             i2c_writel(i2c_dev, val, I2C_TX_FIFO);
>> +             buf += BYTES_PER_FIFO_WORD;
>> +             buf_remaining -= BYTES_PER_FIFO_WORD;
>> +             tx_fifo_avail--;
>> +     }
>> +
>> +     if (tx_fifo_avail > 0 && buf_remaining > 0) {
>> +             int bytes_to_transfer = buf_remaining;
>> +             int byte;
>> +             BUG_ON(bytes_to_transfer > 3);
>> +             val = 0;
>> +             for (byte = 0; byte < bytes_to_transfer; byte++)
>> +                     val |= (*buf++) << (byte * 8);
>> +             i2c_writel(i2c_dev, val, I2C_TX_FIFO);
>> +             buf_remaining -= bytes_to_transfer;
>> +             tx_fifo_avail--;
>> +     }
>> +     BUG_ON(tx_fifo_avail > 0 && buf_remaining > 0);
>> +     i2c_dev->msg_buf_remaining = buf_remaining;
>> +     i2c_dev->msg_buf = buf;
>> +     return 0;
>> +}
>
> And the same goes for this one too.
This one can probably be simplified, it should be safe to read the
bytes after the end of buf and send them to the I2C controller even if
they aren't part of buf.  The second loop can be combined into the
first.

>> +static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>> +{
>> +     u32 val;
>> +     int err = 0;
>> +
>> +     clk_enable(i2c_dev->clk);
>> +
>> +     tegra_periph_reset_assert(i2c_dev->clk);
>> +     udelay(2);
>> +     tegra_periph_reset_deassert(i2c_dev->clk);
>> +
>> +     if (i2c_dev->is_dvc)
>> +             tegra_dvc_init(i2c_dev);
>> +
>> +     val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN;
>> +     i2c_writel(i2c_dev, val, I2C_CNFG);
>> +     i2c_writel(i2c_dev, 0, I2C_INT_MASK);
>> +     tegra_i2c_set_clk(i2c_dev, i2c_dev->bus_clk_rate);
>> +
>> +     val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
>> +             0 << I2C_FIFO_CONTROL_RX_TRIG_SHIFT;
>> +     i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
>> +
>> +     if (tegra_i2c_flush_fifos(i2c_dev))
>> +             err = -ETIMEDOUT;
>> +
>> +     clk_disable(i2c_dev->clk);
>> +     return 0;
>> +}
>
> err never gets returned. please remove or return.
Should be returned, will fix.

>> +static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>> +{
>> +     u32 status;
>> +     const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
>> +     struct tegra_i2c_dev *i2c_dev = dev_id;
>> +
>> +     status = i2c_readl(i2c_dev, I2C_INT_STATUS);
>> +
>> +     if (status == 0) {
>> +             dev_warn(i2c_dev->dev, "irq status 0 %08x\n",
>> +                     i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS));
>> +             return IRQ_HANDLED;
>
> maybe return IRQ_UNHANDED?
Sure - there are some poorly understood cases where the I2C controller
can assert its interrupt with no status bits, but the handling to
detect it and reset the controller is not in this patch, so dev_warn
and IRQ_UNHANDLED is better.

>> +     if (WARN_ON(ret == 0)) {
>
> suppose a WARN_ON() is OK here.
>
>
>> +             dev_err(i2c_dev->dev, "i2c transfer timed out\n");
>> +
>> +             tegra_i2c_init(i2c_dev);
>> +             return -ETIMEDOUT;
>> +     }
>> +
>> +     pr_debug("transfer complete: %d %d %d\n", ret, completion_done(&i2c_dev->msg_complete), i2c_dev->msg_err);
>
> not dev_dbg()?
Will fix

>> +     if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
>> +             return 0;
>> +
>> +     tegra_i2c_init(i2c_dev);
>> +     if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
>> +             if (msg->flags & I2C_M_IGNORE_NAK)
>> +                     return 0;
>> +             return -EREMOTEIO;
>> +     }
>> +
>> +     return -EIO;
>> +}
>
> think that's fine.
>
> [snip]
>
>> +static int tegra_i2c_probe(struct platform_device *pdev)
>> +{
>> +     struct tegra_i2c_dev *i2c_dev;
>> +     struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
>> +     struct resource *res;
>> +     struct resource *iomem;
>> +     struct clk *clk;
>> +     struct clk *i2c_clk;
>> +     void *base;
>> +     int irq;
>> +     int ret = 0;
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     if (!res) {
>> +             dev_err(&pdev->dev, "no mem resource?\n");
>> +             return -ENODEV;
>> +     }
>
> -ENODEV gets ignored by the upper layer, maybe find something else to
> return.
I'll return -EINVAL

>> +     iomem = request_mem_region(res->start, resource_size(res), pdev->name);
>> +     if (!iomem) {
>> +             dev_err(&pdev->dev, "I2C region already claimed\n");
>> +             return -EBUSY;
>> +     }
>> +
>> +     base = ioremap(iomem->start, resource_size(iomem));
>> +     if (!base) {
>> +             dev_err(&pdev->dev, "Can't ioremap I2C region\n");
>
> would prefer Cannot
Will fix

>> +             return -ENOMEM;
>> +     }
>> +
>> +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> +     if (!res) {
>> +             dev_err(&pdev->dev, "no irq resource?\n");
>
> No need for ? after this.
Will fix

>> +             ret = -ENODEV;
>
> and see again.
Will fix

>> +             goto err_iounmap;
>> +     }
>> +     irq = res->start;
>> +
>> +     clk = clk_get(&pdev->dev, NULL);
>> +     if (!clk) {
>
> you've departed from no dev_err() here.
Will fix

>> +             ret = -ENOMEM;
>> +             goto err_release_region;
>> +     }
>> +
>
>
>> +     i2c_clk = clk_get(&pdev->dev, "i2c");
>> +     if (!i2c_clk) {
>
> see again.
Will fix

>> +             ret = -ENOMEM;
>> +             goto err_clk_put;
>> +     }
>> +
>> +     i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
>> +     if (!i2c_dev) {
>> +             ret = -ENOMEM;
>> +             goto err_i2c_clk_put;
>> +     }
>> +
>> +     i2c_dev->base = base;
>> +     i2c_dev->clk = clk;
>> +     i2c_dev->i2c_clk = i2c_clk;
>> +     i2c_dev->iomem = iomem;
>> +     i2c_dev->adapter.algo = &tegra_i2c_algo;
>> +     i2c_dev->irq = irq;
>> +     i2c_dev->cont_id = pdev->id;
>> +     i2c_dev->dev = &pdev->dev;
>> +     i2c_dev->bus_clk_rate = pdata ? pdata->bus_clk_rate : 100000;
>> +
>> +     if (pdev->id == 3)
>> +             i2c_dev->is_dvc = 1;
>> +     init_completion(&i2c_dev->msg_complete);
>> +
>> +     platform_set_drvdata(pdev, i2c_dev);
>> +
>> +     ret = tegra_i2c_init(i2c_dev);
>> +     if (ret)
>> +             goto err_free;
>
> does tegra_i2c_init() print an appropriate error?
Will fix

>> +
>> +     ret = request_irq(i2c_dev->irq, tegra_i2c_isr, IRQF_DISABLED,
>> +             pdev->name, i2c_dev);
>
> hmm, do you really want to be running IRQF_DISABLED?
No, I'll drop it.

>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
>> +             goto err_free;
>> +     }
>> +
>> +     clk_enable(i2c_dev->i2c_clk);
>> +
>> +     i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
>> +     i2c_dev->adapter.owner = THIS_MODULE;
>> +     i2c_dev->adapter.class = I2C_CLASS_HWMON;
>> +     strlcpy(i2c_dev->adapter.name, "Tegra I2C adapter",
>> +             sizeof(i2c_dev->adapter.name));
>> +     i2c_dev->adapter.algo = &tegra_i2c_algo;
>> +     i2c_dev->adapter.dev.parent = &pdev->dev;
>> +     i2c_dev->adapter.nr = pdev->id;
>> +
>> +     ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "Failed to add I2C adapter\n");
>> +             goto err_free_irq;
>> +     }
>> +
>> +     return 0;
>> +err_free_irq:
>> +     free_irq(i2c_dev->irq, i2c_dev);
>> +err_free:
>> +     kfree(i2c_dev);
>> +err_i2c_clk_put:
>> +     clk_put(i2c_clk);
>> +err_clk_put:
>> +     clk_put(clk);
>> +err_release_region:
>> +     release_mem_region(iomem->start, resource_size(iomem));
>> +err_iounmap:
>> +     iounmap(base);
>> +     return ret;
>> +}
>
> [snip]
>
> how about MODULE_* decelerations at the end of this, with license, etc?
Will fix
>
>> diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
>> new file mode 100644
>> index 0000000..9c85da4
>> --- /dev/null
>
> looks ok.
>
> could do with a few tidies before inclusion.
>
> --
> Ben
>
>

Re: [PATCH] i2c: tegra: Add i2c support

[Colin Cross]

On Wed, Feb 16, 2011 at 9:37 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 08/02/11 12:44, Mark Brown wrote:
>> ...
>> +             goto err_iounmap;
>> +     }
>> +     irq = res->start;
>> +
>> +     clk = clk_get(&pdev->dev, NULL);
>> +     if (!clk) {
>
> This should be:
>
> if (!IS_ERR(clk)) {
Just if (IS_ERR(clk))

>> +             ret = -ENOMEM;
>
> This should probably be:
>
> ret = PTR_ERR(clk);
Yes

> although Ben made some comments in his review re: certain error codes being
> ignored by the higher layers; I'm not sure if that applies here or not.
The clkdev clk_get implementation returns -ENOENT, so it should be OK.

>> +             goto err_release_region;
>> +     }
>> +
>
> --
> nvpublic
>
>

Re: [PATCH] i2c: tegra: Add i2c support

[Ben Dooks]

On Sun, Feb 20, 2011 at 11:49:56AM -0800, Colin Cross wrote:
> On Tue, Feb 15, 2011 at 3:48 PM, Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
> > On 08/02/11 12:44, Mark Brown wrote:
> >> From: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> >
> > Some sort of explanation here would have been nice.
> Will fix
> 
> >> Signed-off-by: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> >> Signed-off-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> >> ---
> >>  drivers/i2c/busses/Kconfig     |    7 +
> >>  drivers/i2c/busses/Makefile    |    1 +
> >>  drivers/i2c/busses/i2c-tegra.c |  665 ++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/i2c-tegra.h      |   25 ++
> >>  4 files changed, 698 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/i2c/busses/i2c-tegra.c
> >>  create mode 100644 include/linux/i2c-tegra.h
> >>
> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> >> index 30f8dbd..b76a2a4 100644
> >> --- a/drivers/i2c/busses/Kconfig
> >> +++ b/drivers/i2c/busses/Kconfig
> >
> > Fine.
> >
> >> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> >> index 3c630b7..f505253 100644
> >> --- a/drivers/i2c/busses/Makefile
> >> +++ b/drivers/i2c/busses/Makefile
> >
> > Fine.
> >
> >> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> >> new file mode 100644
> >> index 0000000..cfa0084
> >> --- /dev/null
> >> +++ b/drivers/i2c/busses/i2c-tegra.c
> >> @@ -0,0 +1,665 @@
> >> +/*
> >> + * drivers/i2c/busses/i2c-tegra.c
> >> + *
> >> + * Copyright (C) 2010 Google, Inc.
> >> + * Author: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
> >> + *
> >> + * This software is licensed under the terms of the GNU General Public
> >> + * License version 2, as published by the Free Software Foundation, and
> >> + * may be copied, distributed, and modified under those terms.
> >> + *
> >> + * 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.
> >> + *
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/init.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/io.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/i2c-tegra.h>
> >> +
> >> +#include <asm/unaligned.h>
> >> +
> >> +#include <mach/clk.h>
> >
> > I don't think you should be including
> mach/clk.h provides the definitions of tegra_assert_periph_reset.  The
> Tegra resets are part of the clock controller, and the I2C bus
> requires resets at various times, including after a NACK.  It does not
> export any of the private clock implementation.
> 
> >> +
> >> +struct tegra_i2c_dev {
> >> +     struct device *dev;
> >> +     struct i2c_adapter adapter;
> >> +     struct clk *clk;
> >> +     struct clk *i2c_clk;
> >> +     struct resource *iomem;
> >> +     void __iomem *base;
> >> +     int cont_id;
> >> +     int irq;
> >> +     int is_dvc;
> >> +     struct completion msg_complete;
> >> +     int msg_err;
> >> +     u8 *msg_buf;
> >> +     size_t msg_buf_remaining;
> >> +     int msg_read;
> >> +     int msg_transfer_complete;
> >> +     unsigned long bus_clk_rate;
> >> +     bool is_suspended;
> >> +};
> >
> > would have been nice to have some kerneldoc for this.
> Will do
> 
> >> +/*
> >> + * i2c_writel and i2c_readl will offset the register if necessary to talk
> >> + * to the I2C block inside the DVC block
> >> + */
> >> +static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
> >> +{
> >> +     if (i2c_dev->is_dvc)
> >> +             reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
> >> +     writel(val, i2c_dev->base + reg);
> >> +}
> >> +
> >> +static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
> >> +{
> >> +     if (i2c_dev->is_dvc)
> >> +             reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
> >> +     return readl(i2c_dev->base + reg);
> >> +}
> >
> > as a note, would have been better to wrap up the address changes into
> > one place to ensure that they're consistent.
> Will fix
> 
> > [snip]
> >
> >> +static void tegra_i2c_set_clk(struct tegra_i2c_dev *i2c_dev, unsigned int freq)
> >> +{
> >> +     clk_set_rate(i2c_dev->clk, freq * 8);
> >> +}
> >
> > hmm, really need a separate function?
> Will fix
> 
> >> +static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
> >> +{
> >> +     unsigned long timeout = jiffies + HZ;
> >> +     u32 val = i2c_readl(i2c_dev, I2C_FIFO_CONTROL);
> >> +     val |= I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH;
> >> +     i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
> >> +
> >> +     while (i2c_readl(i2c_dev, I2C_FIFO_CONTROL) &
> >> +             (I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH)) {
> >> +             if (time_after(jiffies, timeout)) {
> >> +                     dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
> >> +                     return -ETIMEDOUT;
> >> +             }
> >> +             msleep(1);
> >> +     }
> >> +     return 0;
> >> +}
> >> +
> >> +static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
> >> +{
> >> +     u32 val;
> >> +     int rx_fifo_avail;
> >> +     int word;
> >> +     u8 *buf = i2c_dev->msg_buf;
> >> +     size_t buf_remaining = i2c_dev->msg_buf_remaining;
> >> +     int words_to_transfer;
> >> +
> >> +     val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
> >> +     rx_fifo_avail = (val & I2C_FIFO_STATUS_RX_MASK) >>
> >> +             I2C_FIFO_STATUS_RX_SHIFT;
> >> +
> >> +     words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
> >> +     if (words_to_transfer > rx_fifo_avail)
> >> +             words_to_transfer = rx_fifo_avail;
> >> +
> >> +     for (word = 0; word < words_to_transfer; word++) {
> >> +             val = i2c_readl(i2c_dev, I2C_RX_FIFO);
> >> +             put_unaligned_le32(val, buf);
> >> +             buf += BYTES_PER_FIFO_WORD;
> >> +             buf_remaining -= BYTES_PER_FIFO_WORD;
> >> +             rx_fifo_avail--;
> >> +     }
> >
> > you know, there's a readsl() function that does this.
> I think readsl can't handle the possibly unaligned buf pointer.

it does. see arch/arm/lib/io-writesl.S for proof.

> > this whole function gives me the creeps, is there any reason why
> > we can't use the readsl or similar functions for this?
> Same here - readsl can't handle the alignment requirements, readsb
> can't  handle the required 32 bit register read, and the bytes in the
> same word but after the end buf may not be part of buf, so byte writes
> to buf are required.

You'll find it can, too. arch/arm/lib/io-readsl.S.

[snip]
 
> >> +static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
> >> +{
> >> +     u32 val;
> >> +     int err = 0;
> >> +
> >> +     clk_enable(i2c_dev->clk);
> >> +
> >> +     tegra_periph_reset_assert(i2c_dev->clk);
> >> +     udelay(2);
> >> +     tegra_periph_reset_deassert(i2c_dev->clk);
> >> +
> >> +     if (i2c_dev->is_dvc)
> >> +             tegra_dvc_init(i2c_dev);
> >> +
> >> +     val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN;
> >> +     i2c_writel(i2c_dev, val, I2C_CNFG);
> >> +     i2c_writel(i2c_dev, 0, I2C_INT_MASK);
> >> +     tegra_i2c_set_clk(i2c_dev, i2c_dev->bus_clk_rate);
> >> +
> >> +     val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
> >> +             0 << I2C_FIFO_CONTROL_RX_TRIG_SHIFT;
> >> +     i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
> >> +
> >> +     if (tegra_i2c_flush_fifos(i2c_dev))
> >> +             err = -ETIMEDOUT;
> >> +
> >> +     clk_disable(i2c_dev->clk);
> >> +     return 0;
> >> +}
> >
> > err never gets returned. please remove or return.
> Should be returned, will fix.

good. unused return codes make maintainers cry.
 
> >> +static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
> >> +{
> >> +     u32 status;
> >> +     const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> >> +     struct tegra_i2c_dev *i2c_dev = dev_id;
> >> +
> >> +     status = i2c_readl(i2c_dev, I2C_INT_STATUS);
> >> +
> >> +     if (status == 0) {
> >> +             dev_warn(i2c_dev->dev, "irq status 0 %08x\n",
> >> +                     i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS));
> >> +             return IRQ_HANDLED;
> >
> > maybe return IRQ_UNHANDED?
> Sure - there are some poorly understood cases where the I2C controller
> can assert its interrupt with no status bits, but the handling to
> detect it and reset the controller is not in this patch, so dev_warn
> and IRQ_UNHANDLED is better.

I'm not too concerned if you have `handled` it, but please note what
is happening.

> >> +     if (WARN_ON(ret == 0)) {
> >
> > suppose a WARN_ON() is OK here.
> >
> >
> >> +             dev_err(i2c_dev->dev, "i2c transfer timed out\n");
> >> +
> >> +             tegra_i2c_init(i2c_dev);
> >> +             return -ETIMEDOUT;
> >> +     }
> >> +
> >> +     pr_debug("transfer complete: %d %d %d\n", ret, completion_done(&i2c_dev->msg_complete), i2c_dev->msg_err);
> >
> > not dev_dbg()?
> Will fix
 
> >> +     if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
> >> +             return 0;
> >> +
> >> +     tegra_i2c_init(i2c_dev);
> >> +     if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
> >> +             if (msg->flags & I2C_M_IGNORE_NAK)
> >> +                     return 0;
> >> +             return -EREMOTEIO;
> >> +     }
> >> +
> >> +     return -EIO;
> >> +}
> >
> > think that's fine.
> >
> > [snip]
> >
> >> +static int tegra_i2c_probe(struct platform_device *pdev)
> >> +{
> >> +     struct tegra_i2c_dev *i2c_dev;
> >> +     struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
> >> +     struct resource *res;
> >> +     struct resource *iomem;
> >> +     struct clk *clk;
> >> +     struct clk *i2c_clk;
> >> +     void *base;
> >> +     int irq;
> >> +     int ret = 0;
> >> +
> >> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +     if (!res) {
> >> +             dev_err(&pdev->dev, "no mem resource?\n");
> >> +             return -ENODEV;
> >> +     }
> >
> > -ENODEV gets ignored by the upper layer, maybe find something else to
> > return.
> I'll return -EINVAL

I really should get around to finish writing a good guide to this.
 
> >> +     iomem = request_mem_region(res->start, resource_size(res), pdev->name);
> >> +     if (!iomem) {
> >> +             dev_err(&pdev->dev, "I2C region already claimed\n");
> >> +             return -EBUSY;
> >> +     }
> >> +
> >> +     base = ioremap(iomem->start, resource_size(iomem));
> >> +     if (!base) {
> >> +             dev_err(&pdev->dev, "Can't ioremap I2C region\n");
> >
> > would prefer Cannot
> Will fix
> 
> >> +             return -ENOMEM;
> >> +     }
> >> +
> >> +     res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> +     if (!res) {
> >> +             dev_err(&pdev->dev, "no irq resource?\n");
> >
> > No need for ? after this.
> Will fix
> 
> >> +             ret = -ENODEV;
> >
> > and see again.
> Will fix
> 
> >> +             goto err_iounmap;
> >> +     }
> >> +     irq = res->start;
> >> +
> >> +     clk = clk_get(&pdev->dev, NULL);
> >> +     if (!clk) {
> >
> > you've departed from no dev_err() here.
> Will fix
> 
> >> +             ret = -ENOMEM;
> >> +             goto err_release_region;
> >> +     }
> >> +
> >
> >
> >> +     i2c_clk = clk_get(&pdev->dev, "i2c");
> >> +     if (!i2c_clk) {
> >
> > see again.
> Will fix
> 
> >> +             ret = -ENOMEM;
> >> +             goto err_clk_put;
> >> +     }
> >> +
> >> +     i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
> >> +     if (!i2c_dev) {
> >> +             ret = -ENOMEM;
> >> +             goto err_i2c_clk_put;
> >> +     }
> >> +
> >> +     i2c_dev->base = base;
> >> +     i2c_dev->clk = clk;
> >> +     i2c_dev->i2c_clk = i2c_clk;
> >> +     i2c_dev->iomem = iomem;
> >> +     i2c_dev->adapter.algo = &tegra_i2c_algo;
> >> +     i2c_dev->irq = irq;
> >> +     i2c_dev->cont_id = pdev->id;
> >> +     i2c_dev->dev = &pdev->dev;
> >> +     i2c_dev->bus_clk_rate = pdata ? pdata->bus_clk_rate : 100000;
> >> +
> >> +     if (pdev->id == 3)
> >> +             i2c_dev->is_dvc = 1;
> >> +     init_completion(&i2c_dev->msg_complete);
> >> +
> >> +     platform_set_drvdata(pdev, i2c_dev);
> >> +
> >> +     ret = tegra_i2c_init(i2c_dev);
> >> +     if (ret)
> >> +             goto err_free;
> >
> > does tegra_i2c_init() print an appropriate error?
> Will fix
> 
> >> +
> >> +     ret = request_irq(i2c_dev->irq, tegra_i2c_isr, IRQF_DISABLED,
> >> +             pdev->name, i2c_dev);
> >
> > hmm, do you really want to be running IRQF_DISABLED?
> No, I'll drop it.
> 
> >> +     if (ret) {
> >> +             dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
> >> +             goto err_free;
> >> +     }
> >> +
> >> +     clk_enable(i2c_dev->i2c_clk);
> >> +
> >> +     i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
> >> +     i2c_dev->adapter.owner = THIS_MODULE;
> >> +     i2c_dev->adapter.class = I2C_CLASS_HWMON;
> >> +     strlcpy(i2c_dev->adapter.name, "Tegra I2C adapter",
> >> +             sizeof(i2c_dev->adapter.name));
> >> +     i2c_dev->adapter.algo = &tegra_i2c_algo;
> >> +     i2c_dev->adapter.dev.parent = &pdev->dev;
> >> +     i2c_dev->adapter.nr = pdev->id;
> >> +
> >> +     ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
> >> +     if (ret) {
> >> +             dev_err(&pdev->dev, "Failed to add I2C adapter\n");
> >> +             goto err_free_irq;
> >> +     }
> >> +
> >> +     return 0;
> >> +err_free_irq:
> >> +     free_irq(i2c_dev->irq, i2c_dev);
> >> +err_free:
> >> +     kfree(i2c_dev);
> >> +err_i2c_clk_put:
> >> +     clk_put(i2c_clk);
> >> +err_clk_put:
> >> +     clk_put(clk);
> >> +err_release_region:
> >> +     release_mem_region(iomem->start, resource_size(iomem));
> >> +err_iounmap:
> >> +     iounmap(base);
> >> +     return ret;
> >> +}
> >
> > [snip]
> >
> > how about MODULE_* decelerations at the end of this, with license, etc?
> Will fix
> >
> >> diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
> >> new file mode 100644
> >> index 0000000..9c85da4
> >> --- /dev/null
> >
> > looks ok.
> >
> > could do with a few tidies before inclusion.
> >
> > --
> > Ben
> >
> >

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

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH] i2c: tegra: Add i2c support

[Ben Dooks]

On Wed, Feb 16, 2011 at 09:37:23AM -0800, Stephen Warren wrote:
> On 08/02/11 12:44, Mark Brown wrote:
> > ...
> > +		goto err_iounmap;
> > +	}
> > +	irq = res->start;
> > +
> > +	clk = clk_get(&pdev->dev, NULL);
> > +	if (!clk) {
> 
> This should be:
> 
> if (!IS_ERR(clk)) {
> 
> > +		ret = -ENOMEM;
> 
> This should probably be:
> 
> ret = PTR_ERR(clk);
> 
> although Ben made some comments in his review re: certain error codes being
> ignored by the higher layers; I'm not sure if that applies here or not.

at this point, we should probably print an error at-least. IIRC, it is
-ENODEV and possibly -EIO that get ignored, everything else should be
dealt with as an device-exists-but-is-not-working.
 
> > +		goto err_release_region;
> > +	}
> > +

thanks for spotting this one.

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

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH] i2c: tegra: Add i2c support

[Colin Cross]

On Sun, Feb 20, 2011 at 3:38 PM, Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
<snip>

>> > you know, there's a readsl() function that does this.
>> I think readsl can't handle the possibly unaligned buf pointer.
>
> it does. see arch/arm/lib/io-writesl.S for proof.
You're right, I traced the wrong definition of readsl/writesl

>> > this whole function gives me the creeps, is there any reason why
>> > we can't use the readsl or similar functions for this?
>> Same here - readsl can't handle the alignment requirements, readsb
>> can't  handle the required 32 bit register read, and the bytes in the
>> same word but after the end buf may not be part of buf, so byte writes
>> to buf are required.
>
> You'll find it can, too. arch/arm/lib/io-readsl.S.
I still think readsl doesn't work here.  It may need to read 7 bytes,
requiring 2 calls to readl, but 1 word write and 3 byte writes to buf.
 Using readsl for the whole buffer would overwrite the 8th byte.  I
can use readsl for the main loop, but I will still need to write the
last 1-3 bytes separately.  I could use readl and memcpy to do the
last writes.

Re: [PATCH] i2c: tegra: Add i2c support

[Colin Cross]

On Sun, Feb 20, 2011 at 3:57 PM, Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org> wrote:
> On Sun, Feb 20, 2011 at 3:38 PM, Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
> <snip>
>
>>> > you know, there's a readsl() function that does this.
>>> I think readsl can't handle the possibly unaligned buf pointer.
>>
>> it does. see arch/arm/lib/io-writesl.S for proof.
> You're right, I traced the wrong definition of readsl/writesl
>
>>> > this whole function gives me the creeps, is there any reason why
>>> > we can't use the readsl or similar functions for this?
>>> Same here - readsl can't handle the alignment requirements, readsb
>>> can't  handle the required 32 bit register read, and the bytes in the
>>> same word but after the end buf may not be part of buf, so byte writes
>>> to buf are required.
>>
>> You'll find it can, too. arch/arm/lib/io-readsl.S.
> I still think readsl doesn't work here.  It may need to read 7 bytes,
> requiring 2 calls to readl, but 1 word write and 3 byte writes to buf.
>  Using readsl for the whole buffer would overwrite the 8th byte.  I
> can use readsl for the main loop, but I will still need to write the
> last 1-3 bytes separately.  I could use readl and memcpy to do the
> last writes.

Actually, the same problem applies to writesl.  If the buffer is not
aligned, reading an entire word to get the partial word at the end of
the buffer may cross a page boundary and fault.  I'll have to use
writesl for the whole words and then handle the last partial word with
memcpy and writel.

[PATCH v2] i2c: tegra: Add i2c support

[Colin Cross]

Adds I2C bus driver for nVidia Tegra SoCs.  Tegra includes 4 I2C
controllers, one of which is inside the Dynamic Voltage Controller
and has a slightly different register map.

Signed-off-by: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
---
v2: fixes comments from Ben Dooks and Stephen Warren

 drivers/i2c/busses/Kconfig     |    7 +
 drivers/i2c/busses/Makefile    |    1 +
 drivers/i2c/busses/i2c-tegra.c |  700 ++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-tegra.h      |   25 ++
 4 files changed, 733 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-tegra.c
 create mode 100644 include/linux/i2c-tegra.h

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 113505a..a5f48dc 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -607,6 +607,13 @@ config I2C_STU300
 	  This driver can also be built as a module. If so, the module
 	  will be called i2c-stu300.
 
+config I2C_TEGRA
+	tristate "NVIDIA Tegra internal I2C controller"
+	depends on ARCH_TEGRA
+	help
+	  If you say yes to this option, support will be included for the
+	  I2C controller embedded in NVIDIA Tegra SOCs
+
 config I2C_VERSATILE
 	tristate "ARM Versatile/Realview I2C bus support"
 	depends on ARCH_VERSATILE || ARCH_REALVIEW || ARCH_VEXPRESS
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 9d2d0ec..a0d4afe 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_I2C_SH7760)	+= i2c-sh7760.o
 obj-$(CONFIG_I2C_SH_MOBILE)	+= i2c-sh_mobile.o
 obj-$(CONFIG_I2C_SIMTEC)	+= i2c-simtec.o
 obj-$(CONFIG_I2C_STU300)	+= i2c-stu300.o
+obj-$(CONFIG_I2C_TEGRA)		+= i2c-tegra.o
 obj-$(CONFIG_I2C_VERSATILE)	+= i2c-versatile.o
 obj-$(CONFIG_I2C_OCTEON)	+= i2c-octeon.o
 obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
new file mode 100644
index 0000000..3921f66
--- /dev/null
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -0,0 +1,700 @@
+/*
+ * drivers/i2c/busses/i2c-tegra.c
+ *
+ * Copyright (C) 2010 Google, Inc.
+ * Author: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/i2c-tegra.h>
+
+#include <asm/unaligned.h>
+
+#include <mach/clk.h>
+
+#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
+#define BYTES_PER_FIFO_WORD 4
+
+#define I2C_CNFG				0x000
+#define I2C_CNFG_PACKET_MODE_EN			(1<<10)
+#define I2C_CNFG_NEW_MASTER_FSM			(1<<11)
+#define I2C_SL_CNFG				0x020
+#define I2C_SL_CNFG_NEWSL			(1<<2)
+#define I2C_SL_ADDR1				0x02c
+#define I2C_TX_FIFO				0x050
+#define I2C_RX_FIFO				0x054
+#define I2C_PACKET_TRANSFER_STATUS		0x058
+#define I2C_FIFO_CONTROL			0x05c
+#define I2C_FIFO_CONTROL_TX_FLUSH		(1<<1)
+#define I2C_FIFO_CONTROL_RX_FLUSH		(1<<0)
+#define I2C_FIFO_CONTROL_TX_TRIG_SHIFT		5
+#define I2C_FIFO_CONTROL_RX_TRIG_SHIFT		2
+#define I2C_FIFO_STATUS				0x060
+#define I2C_FIFO_STATUS_TX_MASK			0xF0
+#define I2C_FIFO_STATUS_TX_SHIFT		4
+#define I2C_FIFO_STATUS_RX_MASK			0x0F
+#define I2C_FIFO_STATUS_RX_SHIFT		0
+#define I2C_INT_MASK				0x064
+#define I2C_INT_STATUS				0x068
+#define I2C_INT_PACKET_XFER_COMPLETE		(1<<7)
+#define I2C_INT_ALL_PACKETS_XFER_COMPLETE	(1<<6)
+#define I2C_INT_TX_FIFO_OVERFLOW		(1<<5)
+#define I2C_INT_RX_FIFO_UNDERFLOW		(1<<4)
+#define I2C_INT_NO_ACK				(1<<3)
+#define I2C_INT_ARBITRATION_LOST		(1<<2)
+#define I2C_INT_TX_FIFO_DATA_REQ		(1<<1)
+#define I2C_INT_RX_FIFO_DATA_REQ		(1<<0)
+#define I2C_CLK_DIVISOR				0x06c
+
+#define DVC_CTRL_REG1				0x000
+#define DVC_CTRL_REG1_INTR_EN			(1<<10)
+#define DVC_CTRL_REG2				0x004
+#define DVC_CTRL_REG3				0x008
+#define DVC_CTRL_REG3_SW_PROG			(1<<26)
+#define DVC_CTRL_REG3_I2C_DONE_INTR_EN		(1<<30)
+#define DVC_STATUS				0x00c
+#define DVC_STATUS_I2C_DONE_INTR		(1<<30)
+
+#define I2C_ERR_NONE				0x00
+#define I2C_ERR_NO_ACK				0x01
+#define I2C_ERR_ARBITRATION_LOST		0x02
+
+#define PACKET_HEADER0_HEADER_SIZE_SHIFT	28
+#define PACKET_HEADER0_PACKET_ID_SHIFT		16
+#define PACKET_HEADER0_CONT_ID_SHIFT		12
+#define PACKET_HEADER0_PROTOCOL_I2C		(1<<4)
+
+#define I2C_HEADER_HIGHSPEED_MODE		(1<<22)
+#define I2C_HEADER_CONT_ON_NAK			(1<<21)
+#define I2C_HEADER_SEND_START_BYTE		(1<<20)
+#define I2C_HEADER_READ				(1<<19)
+#define I2C_HEADER_10BIT_ADDR			(1<<18)
+#define I2C_HEADER_IE_ENABLE			(1<<17)
+#define I2C_HEADER_REPEAT_START			(1<<16)
+#define I2C_HEADER_MASTER_ADDR_SHIFT		12
+#define I2C_HEADER_SLAVE_ADDR_SHIFT		1
+
+/**
+ * struct tegra_i2c_dev	- per device i2c context
+ * @dev: device reference for power management
+ * @adapter: core i2c layer adapter information
+ * @clk: clock reference for i2c controller
+ * @i2c_clk: clock reference for i2c bus
+ * @iomem: memory resource for registers
+ * @base: ioremapped registers cookie
+ * @cont_id: i2c controller id, used for for packet header
+ * @irq: irq number of transfer complete interrupt
+ * @is_dvc: identifies the DVC i2c controller, has a different register layout
+ * @msg_complete: transfer completion notifier
+ * @msg_err: error code for completed message
+ * @msg_buf: pointer to current message data
+ * @msg_buf_remaining: size of unsent data in the message buffer
+ * @msg_read: identifies read transfers
+ * @bus_clk_rate: current i2c bus clock rate
+ * @is_suspended: prevents i2c controller accesses after suspend is called
+ */
+struct tegra_i2c_dev {
+	struct device *dev;
+	struct i2c_adapter adapter;
+	struct clk *clk;
+	struct clk *i2c_clk;
+	struct resource *iomem;
+	void __iomem *base;
+	int cont_id;
+	int irq;
+	int is_dvc;
+	struct completion msg_complete;
+	int msg_err;
+	u8 *msg_buf;
+	size_t msg_buf_remaining;
+	int msg_read;
+	unsigned long bus_clk_rate;
+	bool is_suspended;
+};
+
+static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
+{
+	writel(val, i2c_dev->base + reg);
+}
+
+static u32 dvc_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
+{
+	return readl(i2c_dev->base + reg);
+}
+
+/*
+ * i2c_writel and i2c_readl will offset the register if necessary to talk
+ * to the I2C block inside the DVC block
+ */
+static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
+	unsigned long reg)
+{
+	if (i2c_dev->is_dvc)
+		reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
+	return reg;
+}
+
+static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
+	unsigned long reg)
+{
+	writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
+}
+
+static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
+{
+	return readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
+}
+
+static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
+	unsigned long reg, int len)
+{
+	writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
+}
+
+static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
+	unsigned long reg, int len)
+{
+	readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
+}
+
+static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
+{
+	u32 int_mask = i2c_readl(i2c_dev, I2C_INT_MASK);
+	int_mask &= ~mask;
+	i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
+}
+
+static void tegra_i2c_unmask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
+{
+	u32 int_mask = i2c_readl(i2c_dev, I2C_INT_MASK);
+	int_mask |= mask;
+	i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
+}
+
+static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
+{
+	unsigned long timeout = jiffies + HZ;
+	u32 val = i2c_readl(i2c_dev, I2C_FIFO_CONTROL);
+	val |= I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH;
+	i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
+
+	while (i2c_readl(i2c_dev, I2C_FIFO_CONTROL) &
+		(I2C_FIFO_CONTROL_TX_FLUSH | I2C_FIFO_CONTROL_RX_FLUSH)) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(i2c_dev->dev, "timeout waiting for fifo flush\n");
+			return -ETIMEDOUT;
+		}
+		msleep(1);
+	}
+	return 0;
+}
+
+static int tegra_i2c_empty_rx_fifo(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val;
+	int rx_fifo_avail;
+	u8 *buf = i2c_dev->msg_buf;
+	size_t buf_remaining = i2c_dev->msg_buf_remaining;
+	int words_to_transfer;
+
+	val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
+	rx_fifo_avail = (val & I2C_FIFO_STATUS_RX_MASK) >>
+		I2C_FIFO_STATUS_RX_SHIFT;
+
+	/* Rounds down to not include partial word at the end of buf */
+	words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
+	if (words_to_transfer > rx_fifo_avail)
+		words_to_transfer = rx_fifo_avail;
+
+	i2c_readsl(i2c_dev, buf, I2C_RX_FIFO, words_to_transfer);
+
+	buf += words_to_transfer * BYTES_PER_FIFO_WORD;
+	buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD;
+	rx_fifo_avail -= words_to_transfer;
+
+	/*
+	 * If there is a partial word at the end of buf, handle it manually to
+	 * prevent overwriting past the end of buf
+	 */
+	if (rx_fifo_avail > 0 && buf_remaining > 0) {
+		BUG_ON(buf_remaining > 3);
+		val = i2c_readl(i2c_dev, I2C_RX_FIFO);
+		memcpy(buf, &val, buf_remaining);
+		buf_remaining = 0;
+		rx_fifo_avail--;
+	}
+
+	BUG_ON(rx_fifo_avail > 0 && buf_remaining > 0);
+	i2c_dev->msg_buf_remaining = buf_remaining;
+	i2c_dev->msg_buf = buf;
+	return 0;
+}
+
+static int tegra_i2c_fill_tx_fifo(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val;
+	int tx_fifo_avail;
+	u8 *buf = i2c_dev->msg_buf;
+	size_t buf_remaining = i2c_dev->msg_buf_remaining;
+	int words_to_transfer;
+
+	val = i2c_readl(i2c_dev, I2C_FIFO_STATUS);
+	tx_fifo_avail = (val & I2C_FIFO_STATUS_TX_MASK) >>
+		I2C_FIFO_STATUS_TX_SHIFT;
+
+	/* Rounds down to not include partial word at the end of buf */
+	words_to_transfer = buf_remaining / BYTES_PER_FIFO_WORD;
+	if (words_to_transfer > tx_fifo_avail)
+		words_to_transfer = tx_fifo_avail;
+
+	i2c_writesl(i2c_dev, buf, I2C_TX_FIFO, words_to_transfer);
+
+	buf += words_to_transfer * BYTES_PER_FIFO_WORD;
+	buf_remaining -= words_to_transfer * BYTES_PER_FIFO_WORD;
+	tx_fifo_avail -= words_to_transfer;
+
+	/*
+	 * If there is a partial word at the end of buf, handle it manually to
+	 * prevent reading past the end of buf, which could cross a page
+	 * boundary and fault.
+	 */
+	if (tx_fifo_avail > 0 && buf_remaining > 0) {
+		BUG_ON(buf_remaining > 3);
+		memcpy(&val, buf, buf_remaining);
+		i2c_writel(i2c_dev, val, I2C_TX_FIFO);
+		buf_remaining = 0;
+		tx_fifo_avail--;
+	}
+
+	BUG_ON(tx_fifo_avail > 0 && buf_remaining > 0);
+	i2c_dev->msg_buf_remaining = buf_remaining;
+	i2c_dev->msg_buf = buf;
+	return 0;
+}
+
+/*
+ * One of the Tegra I2C blocks is inside the DVC (Digital Voltage Controller)
+ * block.  This block is identical to the rest of the I2C blocks, except that
+ * it only supports master mode, it has registers moved around, and it needs
+ * some extra init to get it into I2C mode.  The register moves are handled
+ * by i2c_readl and i2c_writel
+ */
+static void tegra_dvc_init(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val = 0;
+	val = dvc_readl(i2c_dev, DVC_CTRL_REG3);
+	val |= DVC_CTRL_REG3_SW_PROG;
+	val |= DVC_CTRL_REG3_I2C_DONE_INTR_EN;
+	dvc_writel(i2c_dev, val, DVC_CTRL_REG3);
+
+	val = dvc_readl(i2c_dev, DVC_CTRL_REG1);
+	val |= DVC_CTRL_REG1_INTR_EN;
+	dvc_writel(i2c_dev, val, DVC_CTRL_REG1);
+}
+
+static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val;
+	int err = 0;
+
+	clk_enable(i2c_dev->clk);
+
+	tegra_periph_reset_assert(i2c_dev->clk);
+	udelay(2);
+	tegra_periph_reset_deassert(i2c_dev->clk);
+
+	if (i2c_dev->is_dvc)
+		tegra_dvc_init(i2c_dev);
+
+	val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN;
+	i2c_writel(i2c_dev, val, I2C_CNFG);
+	i2c_writel(i2c_dev, 0, I2C_INT_MASK);
+	clk_set_rate(i2c_dev->clk, i2c_dev->bus_clk_rate * 8);
+
+	val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
+		0 << I2C_FIFO_CONTROL_RX_TRIG_SHIFT;
+	i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
+
+	if (tegra_i2c_flush_fifos(i2c_dev))
+		err = -ETIMEDOUT;
+
+	clk_disable(i2c_dev->clk);
+	return err;
+}
+
+static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
+{
+	u32 status;
+	const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+	struct tegra_i2c_dev *i2c_dev = dev_id;
+
+	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
+
+	if (status == 0) {
+		dev_warn(i2c_dev->dev, "interrupt with no status\n");
+		return IRQ_NONE;
+	}
+
+	if (unlikely(status & status_err)) {
+		if (status & I2C_INT_NO_ACK)
+			i2c_dev->msg_err |= I2C_ERR_NO_ACK;
+		if (status & I2C_INT_ARBITRATION_LOST)
+			i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
+		complete(&i2c_dev->msg_complete);
+		goto err;
+	}
+
+	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
+		if (i2c_dev->msg_buf_remaining)
+			tegra_i2c_empty_rx_fifo(i2c_dev);
+		else
+			BUG();
+	}
+
+	if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
+		if (i2c_dev->msg_buf_remaining)
+			tegra_i2c_fill_tx_fifo(i2c_dev);
+		else
+			tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
+	}
+
+	if ((status & I2C_INT_PACKET_XFER_COMPLETE) &&
+			!i2c_dev->msg_buf_remaining)
+		complete(&i2c_dev->msg_complete);
+
+	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
+	if (i2c_dev->is_dvc)
+		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
+	return IRQ_HANDLED;
+err:
+	/* An error occured, mask all interrupts */
+	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
+		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
+		I2C_INT_RX_FIFO_DATA_REQ);
+	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
+	return IRQ_HANDLED;
+}
+
+static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
+	struct i2c_msg *msg, int stop)
+{
+	u32 packet_header;
+	u32 int_mask;
+	int ret;
+
+	tegra_i2c_flush_fifos(i2c_dev);
+	i2c_writel(i2c_dev, 0xFF, I2C_INT_STATUS);
+
+	if (msg->len == 0)
+		return -EINVAL;
+
+	i2c_dev->msg_buf = msg->buf;
+	i2c_dev->msg_buf_remaining = msg->len;
+	i2c_dev->msg_err = I2C_ERR_NONE;
+	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
+	INIT_COMPLETION(i2c_dev->msg_complete);
+
+	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
+			PACKET_HEADER0_PROTOCOL_I2C |
+			(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
+			(1 << PACKET_HEADER0_PACKET_ID_SHIFT);
+	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+
+	packet_header = msg->len - 1;
+	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+
+	packet_header = msg->addr << I2C_HEADER_SLAVE_ADDR_SHIFT;
+	packet_header |= I2C_HEADER_IE_ENABLE;
+	if (msg->flags & I2C_M_TEN)
+		packet_header |= I2C_HEADER_10BIT_ADDR;
+	if (msg->flags & I2C_M_IGNORE_NAK)
+		packet_header |= I2C_HEADER_CONT_ON_NAK;
+	if (msg->flags & I2C_M_NOSTART)
+		packet_header |= I2C_HEADER_REPEAT_START;
+	if (msg->flags & I2C_M_RD)
+		packet_header |= I2C_HEADER_READ;
+	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+
+	if (!(msg->flags & I2C_M_RD))
+		tegra_i2c_fill_tx_fifo(i2c_dev);
+
+	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+	if (msg->flags & I2C_M_RD)
+		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
+	else if (i2c_dev->msg_buf_remaining)
+		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+	tegra_i2c_unmask_irq(i2c_dev, int_mask);
+	dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
+		i2c_readl(i2c_dev, I2C_INT_MASK));
+
+	ret = wait_for_completion_timeout(&i2c_dev->msg_complete, TEGRA_I2C_TIMEOUT);
+	tegra_i2c_mask_irq(i2c_dev, int_mask);
+
+	if (WARN_ON(ret == 0)) {
+		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
+
+		tegra_i2c_init(i2c_dev);
+		return -ETIMEDOUT;
+	}
+
+	dev_dbg(i2c_dev->dev, "transfer complete: %d %d %d\n",
+		ret, completion_done(&i2c_dev->msg_complete), i2c_dev->msg_err);
+
+	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
+		return 0;
+
+	tegra_i2c_init(i2c_dev);
+	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
+		if (msg->flags & I2C_M_IGNORE_NAK)
+			return 0;
+		return -EREMOTEIO;
+	}
+
+	return -EIO;
+}
+
+static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+	int num)
+{
+	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
+	int i;
+	int ret = 0;
+
+	if (i2c_dev->is_suspended)
+		return -EBUSY;
+
+	clk_enable(i2c_dev->clk);
+	for (i = 0; i < num; i++) {
+		int stop = (i == (num - 1)) ? 1  : 0;
+		ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], stop);
+		if (ret)
+			break;
+	}
+	clk_disable(i2c_dev->clk);
+	return ret ?: i;
+}
+
+static u32 tegra_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C;
+}
+
+static const struct i2c_algorithm tegra_i2c_algo = {
+	.master_xfer	= tegra_i2c_xfer,
+	.functionality	= tegra_i2c_func,
+};
+
+static int tegra_i2c_probe(struct platform_device *pdev)
+{
+	struct tegra_i2c_dev *i2c_dev;
+	struct tegra_i2c_platform_data *pdata = pdev->dev.platform_data;
+	struct resource *res;
+	struct resource *iomem;
+	struct clk *clk;
+	struct clk *i2c_clk;
+	void *base;
+	int irq;
+	int ret = 0;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no mem resource\n");
+		return -EINVAL;
+	}
+	iomem = request_mem_region(res->start, resource_size(res), pdev->name);
+	if (!iomem) {
+		dev_err(&pdev->dev, "I2C region already claimed\n");
+		return -EBUSY;
+	}
+
+	base = ioremap(iomem->start, resource_size(iomem));
+	if (!base) {
+		dev_err(&pdev->dev, "Cannot ioremap I2C region\n");
+		return -ENOMEM;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "no irq resource\n");
+		ret = -EINVAL;
+		goto err_iounmap;
+	}
+	irq = res->start;
+
+	clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(&pdev->dev, "missing controller clock");
+		ret = PTR_ERR(clk);
+		goto err_release_region;
+	}
+
+	i2c_clk = clk_get(&pdev->dev, "i2c");
+	if (IS_ERR(i2c_clk)) {
+		dev_err(&pdev->dev, "missing bus clock");
+		ret = PTR_ERR(i2c_clk);
+		goto err_clk_put;
+	}
+
+	i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
+	if (!i2c_dev) {
+		ret = -ENOMEM;
+		goto err_i2c_clk_put;
+	}
+
+	i2c_dev->base = base;
+	i2c_dev->clk = clk;
+	i2c_dev->i2c_clk = i2c_clk;
+	i2c_dev->iomem = iomem;
+	i2c_dev->adapter.algo = &tegra_i2c_algo;
+	i2c_dev->irq = irq;
+	i2c_dev->cont_id = pdev->id;
+	i2c_dev->dev = &pdev->dev;
+	i2c_dev->bus_clk_rate = pdata ? pdata->bus_clk_rate : 100000;
+
+	if (pdev->id == 3)
+		i2c_dev->is_dvc = 1;
+	init_completion(&i2c_dev->msg_complete);
+
+	platform_set_drvdata(pdev, i2c_dev);
+
+	ret = tegra_i2c_init(i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to initialize i2c controller");
+		goto err_free;
+	}
+
+	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, 0, pdev->name, i2c_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+		goto err_free;
+	}
+
+	clk_enable(i2c_dev->i2c_clk);
+
+	i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
+	i2c_dev->adapter.owner = THIS_MODULE;
+	i2c_dev->adapter.class = I2C_CLASS_HWMON;
+	strlcpy(i2c_dev->adapter.name, "Tegra I2C adapter",
+		sizeof(i2c_dev->adapter.name));
+	i2c_dev->adapter.algo = &tegra_i2c_algo;
+	i2c_dev->adapter.dev.parent = &pdev->dev;
+	i2c_dev->adapter.nr = pdev->id;
+
+	ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to add I2C adapter\n");
+		goto err_free_irq;
+	}
+
+	return 0;
+err_free_irq:
+	free_irq(i2c_dev->irq, i2c_dev);
+err_free:
+	kfree(i2c_dev);
+err_i2c_clk_put:
+	clk_put(i2c_clk);
+err_clk_put:
+	clk_put(clk);
+err_release_region:
+	release_mem_region(iomem->start, resource_size(iomem));
+err_iounmap:
+	iounmap(base);
+	return ret;
+}
+
+static int tegra_i2c_remove(struct platform_device *pdev)
+{
+	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+	i2c_del_adapter(&i2c_dev->adapter);
+	free_irq(i2c_dev->irq, i2c_dev);
+	clk_put(i2c_dev->i2c_clk);
+	clk_put(i2c_dev->clk);
+	release_mem_region(i2c_dev->iomem->start,
+		resource_size(i2c_dev->iomem));
+	iounmap(i2c_dev->base);
+	kfree(i2c_dev);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int tegra_i2c_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	i2c_lock_adapter(&i2c_dev->adapter);
+	i2c_dev->is_suspended = true;
+	i2c_unlock_adapter(&i2c_dev->adapter);
+
+	return 0;
+}
+
+static int tegra_i2c_resume(struct platform_device *pdev)
+{
+	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+	int ret;
+
+	i2c_lock_adapter(&i2c_dev->adapter);
+
+	ret = tegra_i2c_init(i2c_dev);
+
+	if (ret) {
+		i2c_unlock_adapter(&i2c_dev->adapter);
+		return ret;
+	}
+
+	i2c_dev->is_suspended = false;
+
+	i2c_unlock_adapter(&i2c_dev->adapter);
+
+	return 0;
+}
+#endif
+
+static struct platform_driver tegra_i2c_driver = {
+	.probe   = tegra_i2c_probe,
+	.remove  = tegra_i2c_remove,
+#ifdef CONFIG_PM
+	.suspend = tegra_i2c_suspend,
+	.resume  = tegra_i2c_resume,
+#endif
+	.driver  = {
+		.name  = "tegra-i2c",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init tegra_i2c_init_driver(void)
+{
+	return platform_driver_register(&tegra_i2c_driver);
+}
+
+static void __exit tegra_i2c_exit_driver(void)
+{
+	platform_driver_unregister(&tegra_i2c_driver);
+}
+
+subsys_initcall(tegra_i2c_init_driver);
+module_exit(tegra_i2c_exit_driver);
+
+MODULE_DESCRIPTION("nVidia Tegra2 I2C Bus Controller driver");
+MODULE_AUTHOR("Colin Cross");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
new file mode 100644
index 0000000..9c85da4
--- /dev/null
+++ b/include/linux/i2c-tegra.h
@@ -0,0 +1,25 @@
+/*
+ * drivers/i2c/busses/i2c-tegra.c
+ *
+ * Copyright (C) 2010 Google, Inc.
+ * Author: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * 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.
+ *
+ */
+
+#ifndef _LINUX_I2C_TEGRA_H
+#define _LINUX_I2C_TEGRA_H
+
+struct tegra_i2c_platform_data {
+	unsigned long bus_clk_rate;
+};
+
+#endif /* _LINUX_I2C_TEGRA_H */
-- 
1.7.3.1

Re: [PATCH v2] i2c: tegra: Add i2c support

[Olof Johansson]

Hi,

Just a couple of minor nits. I wouldn't worry about respinning just
for these comments, but if you end up doing it for other feedback, see
below.


-Olof

On Sun, Feb 20, 2011 at 5:14 PM, Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org> wrote:

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> new file mode 100644
> index 0000000..3921f66
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -0,0 +1,700 @@
> +/*
> + * drivers/i2c/busses/i2c-tegra.c
> + *
> + * Copyright (C) 2010 Google, Inc.

2011?

[..]

> +/*
> + * i2c_writel and i2c_readl will offset the register if necessary to talk
> + * to the I2C block inside the DVC block
> + */
> +static unsigned long tegra_i2c_reg_addr(struct tegra_i2c_dev *i2c_dev,
> +       unsigned long reg)
> +{
> +       if (i2c_dev->is_dvc)
> +               reg += (reg >= I2C_TX_FIFO) ? 0x10 : 0x40;
> +       return reg;
> +}
> +
> +static void i2c_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
> +       unsigned long reg)
> +{
> +       writel(val, i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> +}
> +
> +static u32 i2c_readl(struct tegra_i2c_dev *i2c_dev, unsigned long reg)
> +{
> +       return readl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg));
> +}
> +
> +static void i2c_writesl(struct tegra_i2c_dev *i2c_dev, void *data,
> +       unsigned long reg, int len)
> +{
> +       writesl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
> +}
> +
> +static void i2c_readsl(struct tegra_i2c_dev *i2c_dev, void *data,
> +       unsigned long reg, int len)
> +{
> +       readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg), data, len);
> +}


All of the above pass in i2c_dev to tegra_i2c_reg_addr, might as well
do the base addition in there.

> diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
> new file mode 100644
> index 0000000..9c85da4
> --- /dev/null
> +++ b/include/linux/i2c-tegra.h
> @@ -0,0 +1,25 @@
> +/*
> + * drivers/i2c/busses/i2c-tegra.c
> + *
> + * Copyright (C) 2010 Google, Inc.

2011?

Re: [PATCH v2] i2c: tegra: Add i2c support

[Colin Cross]

On Sun, Feb 20, 2011 at 5:14 PM, Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org> wrote:
> Adds I2C bus driver for nVidia Tegra SoCs.  Tegra includes 4 I2C
> controllers, one of which is inside the Dynamic Voltage Controller
> and has a slightly different register map.
>
> Signed-off-by: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>

Ping?  Can this go in 2.6.39?

Re: [PATCH] i2c: tegra: Add i2c support

[Ben Dooks]

On Sun, Feb 20, 2011 at 04:28:46PM -0800, Colin Cross wrote:
> On Sun, Feb 20, 2011 at 3:57 PM, Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org> wrote:
> > On Sun, Feb 20, 2011 at 3:38 PM, Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
> > <snip>
> >
> >>> > you know, there's a readsl() function that does this.
> >>> I think readsl can't handle the possibly unaligned buf pointer.
> >>
> >> it does. see arch/arm/lib/io-writesl.S for proof.
> > You're right, I traced the wrong definition of readsl/writesl
> >
> >>> > this whole function gives me the creeps, is there any reason why
> >>> > we can't use the readsl or similar functions for this?
> >>> Same here - readsl can't handle the alignment requirements, readsb
> >>> can't  handle the required 32 bit register read, and the bytes in the
> >>> same word but after the end buf may not be part of buf, so byte writes
> >>> to buf are required.
> >>
> >> You'll find it can, too. arch/arm/lib/io-readsl.S.
> > I still think readsl doesn't work here.  It may need to read 7 bytes,
> > requiring 2 calls to readl, but 1 word write and 3 byte writes to buf.
> >  Using readsl for the whole buffer would overwrite the 8th byte.  I
> > can use readsl for the main loop, but I will still need to write the
> > last 1-3 bytes separately.  I could use readl and memcpy to do the
> > last writes.
> 
> Actually, the same problem applies to writesl.  If the buffer is not
> aligned, reading an entire word to get the partial word at the end of
> the buffer may cross a page boundary and fault.  I'll have to use
> writesl for the whole words and then handle the last partial word with
> memcpy and writel.

Hmm, that's an interesting one, however I suspect this is something
that will never happen as the kernel over-allocates (rounds up) buffers
when allocating memory blocks.

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

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH v2] i2c: tegra: Add i2c support

[Ben Dooks]

On Sun, Feb 20, 2011 at 05:14:21PM -0800, Colin Cross wrote:
> Adds I2C bus driver for nVidia Tegra SoCs.  Tegra includes 4 I2C
> controllers, one of which is inside the Dynamic Voltage Controller
> and has a slightly different register map.
> 
> Signed-off-by: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>

Would have been helpful to start a new thread, missed this the first time
around.

> +	i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
> +	if (!i2c_dev) {

no error print here.

> +		ret = -ENOMEM;
> +		goto err_i2c_clk_put;
> +	}

> +	if (pdev->id == 3)
> +		i2c_dev->is_dvc = 1;
> +	init_completion(&i2c_dev->msg_complete);

you might want to think about having a seperate platform bus name for
this case, and switching the is_dvc field on that. It'll make life much
easier if someone decides that what the next tegra i2c needs is 5 i2c
controllers.

I'll consider putting this (as is) into the -next tree, but would like
to see the issues with readsl/writesl sorted out and any other review
comments sorted.

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

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH v2] i2c: tegra: Add i2c support

[Colin Cross]

On Tue, Feb 22, 2011 at 4:20 PM, Ben Dooks <ben-i2c-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> wrote:
> On Sun, Feb 20, 2011 at 05:14:21PM -0800, Colin Cross wrote:
>> Adds I2C bus driver for nVidia Tegra SoCs.  Tegra includes 4 I2C
>> controllers, one of which is inside the Dynamic Voltage Controller
>> and has a slightly different register map.
>>
>> Signed-off-by: Colin Cross <ccross-z5hGa2qSFaRBDgjK7y7TUQ@public.gmane.org>
>
> Would have been helpful to start a new thread, missed this the first time
> around.
>
>> +     i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
>> +     if (!i2c_dev) {
>
> no error print here.
>
>> +             ret = -ENOMEM;
>> +             goto err_i2c_clk_put;
>> +     }
>
>> +     if (pdev->id == 3)
>> +             i2c_dev->is_dvc = 1;
>> +     init_completion(&i2c_dev->msg_complete);
>
> you might want to think about having a seperate platform bus name for
> this case, and switching the is_dvc field on that. It'll make life much
> easier if someone decides that what the next tegra i2c needs is 5 i2c
> controllers.
>
> I'll consider putting this (as is) into the -next tree, but would like
> to see the issues with readsl/writesl sorted out and any other review
> comments sorted.
>
Thanks for merging it.  Can I merge your i2c-tegra tree into the tegra
for-next tree to allow some board changes that depend on the header to
go on top?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html