i2c: tegra: Add i2c support

From: Colin Cross <[email protected]>

drivers/i2c/busses/i2c-tegra.c new file +21 -0

new file mode 100644
index 0000000..cfa0084
@@ -0,0 +1,665 @@
+/*
+ * drivers/i2c/busses/i2c-tegra.c
+ *
+ * Copyright (C) 2010 Google, Inc.
+ * Author: Colin Cross <[email protected]>
+ *
+ * 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>

drivers/i2c/busses/i2c-tegra.c new file +10 -0

+
+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;

drivers/i2c/busses/i2c-tegra.c new file +7 -0

+/*
+ * 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)

drivers/i2c/busses/i2c-tegra.c new file +32 -0

+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;

drivers/i2c/busses/i2c-tegra.c new file +8 -0

+
+	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;

drivers/i2c/busses/i2c-tegra.c new file +31 -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++)

drivers/i2c/busses/i2c-tegra.c new file +19 -0

+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);
+

drivers/i2c/busses/i2c-tegra.c new file +7 -0

+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;

drivers/i2c/busses/i2c-tegra.c new file +19 -0

+		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;

drivers/i2c/busses/i2c-tegra.c new file +27 -0

+	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);

drivers/i2c/busses/i2c-tegra.c new file +3 -0

+		goto err_release_region;
+	}
+

Quoted 7 lines · Mark Brown

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

This should be:

if (!IS_ERR(clk)) {

Quoted 1 line

+             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.

Quoted 3 lines

+             goto err_release_region;
+     }
+

--
nvpublic

drivers/i2c/busses/i2c-tegra.c line 516

Quoted 7 lines · Mark Brown

...

drivers/i2c/busses/i2c-tegra.c new file +6 -0

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

This should be:

if (!IS_ERR(clk)) {

Review

Quoted 1 line

(quoted diff) +1 -0

+		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.

drivers/i2c/busses/i2c-tegra.c line 520

Quoted 3 lines

drivers/i2c/busses/i2c-tegra.c new file +3 -0

+		goto err_release_region;
+	}
+

Quoted 1 line · Mark Brown

From: Colin Cross <[email protected]>

Some sort of explanation here would have been nice.

drivers/i2c/busses/Kconfig

Quoted 15 lines

Signed-off-by Colin Cross <[email protected]>

Signed-off-by Mark Brown <broonie-yzvPICuk2AATkU/[email protected]>

---
 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

drivers/i2c/busses/Kconfig +0 -0

index 30f8dbd..b76a2a4 100644

Fine.

drivers/i2c/busses/Makefile

Quoted 4 lines

drivers/i2c/busses/Makefile +0 -0

index 3c630b7..f505253 100644

Fine.

Quoted 31 lines

drivers/i2c/busses/i2c-tegra.c new file +25 -0

new file mode 100644
index 0000000..cfa0084
@@ -0,0 +1,665 @@
+/*
+ * drivers/i2c/busses/i2c-tegra.c
+ *
+ * Copyright (C) 2010 Google, Inc.
+ * Author: Colin Cross <[email protected]>
+ *
+ * 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>

drivers/i2c/busses/i2c-tegra.c line 31

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c new file +6 -0

+#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

Quoted 14 lines

drivers/i2c/busses/i2c-tegra.c new file +14 -0

+
+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;

drivers/i2c/busses/i2c-tegra.c line 113

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c new file +6 -0

+     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.

Quoted 12 lines

drivers/i2c/busses/i2c-tegra.c new file +12 -0

+/*
+ * 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)

drivers/i2c/busses/i2c-tegra.c line 141

Quoted 5 lines

drivers/i2c/busses/i2c-tegra.c new file +5 -0

+{
+     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]

drivers/i2c/busses/i2c-tegra.c line 160

Quoted 4 lines

drivers/i2c/busses/i2c-tegra.c new file +4 -0

+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?

Quoted 36 lines

drivers/i2c/busses/i2c-tegra.c new file +36 -0

+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++) {

Quoted 6 lines

+             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.

Quoted 13 lines

+
+     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--;
+     }

Quoted 5 lines

+     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?

Quoted 35 lines

drivers/i2c/busses/i2c-tegra.c new file +35 -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--;

Quoted 6 lines

+     }
+     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.

Quoted 23 lines

drivers/i2c/busses/i2c-tegra.c new file +23 -0

+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);
+

drivers/i2c/busses/i2c-tegra.c line 313

Quoted 6 lines

+     if (tegra_i2c_flush_fifos(i2c_dev))
+             err = -ETIMEDOUT;

drivers/i2c/busses/i2c-tegra.c new file +4 -0

+
+     clk_disable(i2c_dev->clk);
+     return 0;
+}

err never gets returned. please remove or return.

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c new file +6 -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;
+

Quoted 6 lines

+     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?

Quoted 1 line

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

suppose a WARN_ON() is OK here.

drivers/i2c/busses/i2c-tegra.c line 432

Quoted 7 lines

+             dev_err(i2c_dev->dev, "i2c transfer timed out\n");

drivers/i2c/busses/i2c-tegra.c new file +6 -0

+
+             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()?

drivers/i2c/busses/i2c-tegra.c line 445

Quoted 12 lines

+     if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
+             return 0;

drivers/i2c/busses/i2c-tegra.c new file +10 -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]

Quoted 12 lines

drivers/i2c/busses/i2c-tegra.c new file +12 -0

+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;
+

Quoted 5 lines

+     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.

Quoted 9 lines

+     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

Quoted 6 lines

+             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.

Quoted 1 line

+             ret = -ENODEV;

and see again.

Quoted 6 lines

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

you've departed from no dev_err() here.

Quoted 8 lines

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

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

see again.

Quoted 23 lines

+             ret = -ENOMEM;
+             goto err_clk_put;
+     }

drivers/i2c/busses/i2c-tegra.c new file +20 -0

+
+     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);

drivers/i2c/busses/i2c-tegra.c line 551

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c new file +6 -0

+
+     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?

Quoted 3 lines

+
+     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?

Quoted 37 lines

+     if (ret) {
+             dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+             goto err_free;
+     }

drivers/i2c/busses/i2c-tegra.c new file +33 -0

+
+     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?

include/linux/i2c-tegra.h

Quoted 5 lines

include/linux/i2c-tegra.h new file +0 -1

new file mode 100644
index 0000000..9c85da4
--- /dev/null

looks ok.

could do with a few tidies before inclusion.

--
Ben

Quoted 4 lines · Ben Dooks

Quoted 1 line · Mark Brown

From: Colin Cross <[email protected]>

Some sort of explanation here would have been nice.

Will fix

Quoted 56 lines

drivers/i2c/busses/Kconfig

Quoted 15 lines

Signed-off-by Colin Cross <[email protected]>

Signed-off-by Mark Brown <broonie-yzvPICuk2AATkU/[email protected]>

---
 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

drivers/i2c/busses/Kconfig +0 -0

index 30f8dbd..b76a2a4 100644

Fine.

drivers/i2c/busses/Makefile

Quoted 4 lines

drivers/i2c/busses/Makefile +0 -0

index 3c630b7..f505253 100644

Fine.

Quoted 31 lines

drivers/i2c/busses/i2c-tegra.c new file +25 -0

new file mode 100644
index 0000000..cfa0084
@@ -0,0 +1,665 @@
+/*
+ * drivers/i2c/busses/i2c-tegra.c
+ *
+ * Copyright (C) 2010 Google, Inc.
+ * Author: Colin Cross <[email protected]>
+ *
+ * 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>

Quoted 8 lines

drivers/i2c/busses/i2c-tegra.c line 31

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c new file +6 -0

+#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.

Quoted 14 lines

Quoted 14 lines

drivers/i2c/busses/i2c-tegra.c new file +14 -0

+
+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;

Quoted 8 lines

drivers/i2c/busses/i2c-tegra.c line 113

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c new file +6 -0

+     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

Quoted 12 lines

Quoted 12 lines

drivers/i2c/busses/i2c-tegra.c new file +12 -0

+/*
+ * 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)

Quoted 8 lines

drivers/i2c/busses/i2c-tegra.c line 141

Quoted 5 lines

drivers/i2c/busses/i2c-tegra.c new file +5 -0

+{
+     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

Quoted 8 lines

[snip]

drivers/i2c/busses/i2c-tegra.c line 160

Quoted 4 lines

drivers/i2c/busses/i2c-tegra.c new file +4 -0

+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

Quoted 36 lines

Quoted 36 lines

drivers/i2c/busses/i2c-tegra.c new file +36 -0

+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++) {

Quoted 8 lines

Quoted 6 lines

+             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.

Quoted 2 lines

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.

Quoted 23 lines

Quoted 23 lines

drivers/i2c/busses/i2c-tegra.c new file +23 -0

+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);
+

Quoted 8 lines

drivers/i2c/busses/i2c-tegra.c line 313

Quoted 6 lines

+     if (tegra_i2c_flush_fifos(i2c_dev))
+             err = -ETIMEDOUT;

drivers/i2c/busses/i2c-tegra.c new file +4 -0

+
+     clk_disable(i2c_dev->clk);
+     return 0;
+}

err never gets returned. please remove or return.

Should be returned, will fix.

Quoted 6 lines

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c new file +6 -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;
+

Quoted 8 lines

Quoted 6 lines

+     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.

Quoted 5 lines

Quoted 1 line

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

suppose a WARN_ON() is OK here.

Quoted 9 lines

drivers/i2c/busses/i2c-tegra.c line 432

Quoted 7 lines

+             dev_err(i2c_dev->dev, "i2c transfer timed out\n");

drivers/i2c/busses/i2c-tegra.c new file +6 -0

+
+             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

Quoted 29 lines

drivers/i2c/busses/i2c-tegra.c line 445

Quoted 12 lines

+     if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
+             return 0;

drivers/i2c/busses/i2c-tegra.c new file +10 -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]

Quoted 12 lines

drivers/i2c/busses/i2c-tegra.c new file +12 -0

+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;
+

Quoted 8 lines

Quoted 5 lines

+     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

Quoted 11 lines

Quoted 9 lines

+     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

Quoted 8 lines

Quoted 6 lines

+             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

Quoted 3 lines

Quoted 1 line

+             ret = -ENODEV;

and see again.

Will fix

Quoted 8 lines

Quoted 6 lines

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

you've departed from no dev_err() here.

Will fix

Quoted 10 lines

Quoted 8 lines

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

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

see again.

Will fix

Quoted 23 lines

Quoted 23 lines

+             ret = -ENOMEM;
+             goto err_clk_put;
+     }

drivers/i2c/busses/i2c-tegra.c new file +20 -0

+
+     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);

Quoted 8 lines

drivers/i2c/busses/i2c-tegra.c line 551

Quoted 6 lines

drivers/i2c/busses/i2c-tegra.c new file +6 -0

+
+     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

Quoted 5 lines

Quoted 3 lines

+
+     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.

Quoted 40 lines

Quoted 37 lines

+     if (ret) {
+             dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+             goto err_free;
+     }

drivers/i2c/busses/i2c-tegra.c new file +33 -0

+
+     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]

Quoted 1 line

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

Will fix

Quoted 14 lines

include/linux/i2c-tegra.h

Quoted 5 lines

include/linux/i2c-tegra.h new file +0 -1

new file mode 100644
index 0000000..9c85da4
--- /dev/null

looks ok.

could do with a few tidies before inclusion.

--
Ben

Quoted 2 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 2 lines

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.

On Sun, Feb 20, 2011 at 3:38 PM, Ben Dooks <[email protected]> wrote:
<snip>

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 1 line

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.

drivers/i2c/busses/i2c-tegra.c new file +24 -0

+/*
+ * 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,

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 <[email protected]>

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 <[email protected]>

Quoted 5 lines · 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 <[email protected]>

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

Quoted 2 lines

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

no error print here.

Quoted 7 lines

+ ? ? ? ? ? ? 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.

Quoted 16 lines · Colin Cross

On Sun, Feb 20, 2011 at 3:38 PM, Ben Dooks <[email protected]> wrote:
<snip>

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

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

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 1 line

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.