[PATCH 1/5] I2C driver for MXC

[PATCH 1/5] I2C driver for MXC

[Darius]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: patch-i2c-imx --]
[-- Type: text/plain, Size: 20730 bytes --]

From: Darius Augulis <augulis.darius@gmail.com>

Implementation of I2C Adapter/Algorithm Driver for I2C Bus integrated
in Freescale's i.MX/MXC processors. Tested only on MX1, but should work on MX2
and MX3 too, because all three processors have the same I2C controller.

Signed-off-by: Darius Augulis <augulis.darius@gmail.com>

Index: linux-2.6.29-rc5/drivers/i2c/busses/i2c-imx.c
===================================================================
--- /dev/null
+++ linux-2.6.29-rc5/drivers/i2c/busses/i2c-imx.c
@@ -0,0 +1,624 @@
+/*
+ *	Copyright (C) 2002 Motorola GSG-China
+ *
+ *	This program is free software; you can redistribute it and/or
+ *	modify it under the terms of the GNU General Public License
+ *	as published by the Free Software Foundation; either version 2
+ *	of the License, or (at your option) any later version.
+ *
+ *	This program is distributed in the hope that it will be useful,
+ *	but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *	GNU General Public License for more details.
+ *
+ *	You should have received a copy of the GNU General Public License
+ *	along with this program; if not, write to the Free Software
+ *	Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307,
+ *	USA.
+ *
+ * Author:
+ *	Darius Augulis, Teltonika Inc.
+ *
+ * Desc.:
+ *	Implementation of I2C Adapter/Algorithm Driver
+ *	for I2C Bus integrated in Freescale i.MX/MXC processors
+ *
+ *	Derived from Motorola GSG China I2C example driver
+ *
+ *	Copyright (C) 2005 Torsten Koschorrek <koschorrek at synertronixx.de
+ *	Copyright (C) 2005 Matthias Blaschke <blaschke at synertronixx.de
+ *	Copyright (C) 2007 RightHand Technologies, Inc.
+ *	Copyright (C) 2008 Darius Augulis <darius.augulis at teltonika.lt>
+ *
+ */
+
+/** Includes *******************************************************************
+*******************************************************************************/
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/io.h>
+#include <linux/sched.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+
+#include <mach/irqs.h>
+#include <mach/hardware.h>
+#include <mach/i2c.h>
+
+/** Defines ********************************************************************
+*******************************************************************************/
+
+/* This will be the driver name the kernel reports */
+#define DRIVER_NAME "imx-i2c"
+
+/* Default value */
+#define IMX_I2C_BIT_RATE	100000	/* 100kHz */
+
+/* IMX I2C registers */
+#define IMX_I2C_IADR	0x00	/* i2c slave address */
+#define IMX_I2C_IFDR	0x04	/* i2c frequency divider */
+#define IMX_I2C_I2CR	0x08	/* i2c control */
+#define IMX_I2C_I2SR	0x0C	/* i2c status */
+#define IMX_I2C_I2DR	0x10	/* i2c transfer data */
+
+/* Bits of IMX I2C registers */
+#define I2SR_RXAK	0x01
+#define I2SR_IIF	0x02
+#define I2SR_SRW	0x04
+#define I2SR_IAL	0x10
+#define I2SR_IBB	0x20
+#define I2SR_IAAS	0x40
+#define I2SR_ICF	0x80
+#define I2CR_RSTA	0x04
+#define I2CR_TXAK	0x08
+#define I2CR_MTX	0x10
+#define I2CR_MSTA	0x20
+#define I2CR_IIEN	0x40
+#define I2CR_IEN	0x80
+
+/** Variables ******************************************************************
+*******************************************************************************/
+
+static unsigned int disable_delay;	/* Dummy delay */
+
+/*
+ * sorted list of clock divider, register value pairs
+ * taken from table 26-5, p.26-9, Freescale i.MX
+ * Integrated Portable System Processor Reference Manual
+ * Document Number: MC9328MXLRM, Rev. 5.1, 06/2007
+ *
+ * Duplicated divider values removed from list
+ */
+
+static u16 __initdata i2c_clk_div[50][2] = {
+	{ 22,	0x20 }, { 24,	0x21 }, { 26,	0x22 }, { 28,	0x23 },
+	{ 30,	0x00 },	{ 32,	0x24 }, { 36,	0x25 }, { 40,	0x26 },
+	{ 42,	0x03 }, { 44,	0x27 },	{ 48,	0x28 }, { 52,	0x05 },
+	{ 56,	0x29 }, { 60,	0x06 }, { 64,	0x2A },	{ 72,	0x2B },
+	{ 80,	0x2C }, { 88,	0x09 }, { 96,	0x2D }, { 104,	0x0A },
+	{ 112,	0x2E }, { 128,	0x2F }, { 144,	0x0C }, { 160,	0x30 },
+	{ 192,	0x31 },	{ 224,	0x32 }, { 240,	0x0F }, { 256,	0x33 },
+	{ 288,	0x10 }, { 320,	0x34 },	{ 384,	0x35 }, { 448,	0x36 },
+	{ 480,	0x13 }, { 512,	0x37 }, { 576,	0x14 },	{ 640,	0x38 },
+	{ 768,	0x39 }, { 896,	0x3A }, { 960,	0x17 }, { 1024,	0x3B },
+	{ 1152,	0x18 }, { 1280,	0x3C }, { 1536,	0x3D }, { 1792,	0x3E },
+	{ 1920,	0x1B },	{ 2048,	0x3F }, { 2304,	0x1C }, { 2560,	0x1D },
+	{ 3072,	0x1E }, { 3840,	0x1F }
+};
+
+struct imx_i2c_struct {
+	struct i2c_adapter	adapter;
+	struct resource		*res;
+	struct clk		*clk;
+	void __iomem		*base;
+	int			irq;
+	wait_queue_head_t	queue;
+	unsigned long		i2csr;
+};
+
+/** Functions for IMX I2C adapter driver ***************************************
+*******************************************************************************/
+
+static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
+{
+	unsigned long orig_jiffies = jiffies;
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+
+	/* wait for bus not busy */
+	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
+		if (signal_pending(current)) {
+			dev_dbg(&i2c_imx->adapter.dev,
+				"<%s> I2C Interrupted\n", __func__);
+			return -EINTR;
+		}
+		if (time_after(jiffies, orig_jiffies + HZ / 1000)) {
+			dev_dbg(&i2c_imx->adapter.dev,
+				"<%s> I2C bus is busy\n", __func__);
+			return -EIO;
+		}
+		schedule();
+	}
+
+	return 0;
+}
+
+static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
+{
+	int result;
+
+	result = wait_event_interruptible_timeout(i2c_imx->queue,
+		i2c_imx->i2csr & I2SR_IIF, HZ / 10);
+
+	if (unlikely(result < 0)) {
+		dev_dbg(&i2c_imx->adapter.dev, "<%s> result < 0\n", __func__);
+		return result;
+	} else if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
+		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
+		return -ETIMEDOUT;
+	}
+	dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__);
+	i2c_imx->i2csr = 0;
+	return 0;
+}
+
+static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
+{
+	if (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
+		dev_dbg(&i2c_imx->adapter.dev, "<%s> No ACK\n", __func__);
+		return -EIO;  /* No ACK */
+	}
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s> ACK received\n", __func__);
+	return 0;
+}
+
+static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
+{
+	unsigned int temp = 0;
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+
+	/* Enable I2C controller */
+	writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
+	/* Start I2C transaction */
+	temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+	temp |= I2CR_MSTA;
+	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+}
+
+static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
+{
+	unsigned int temp = 0;
+
+	/* Stop I2C transaction */
+	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+	temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+	temp &= ~I2CR_MSTA;
+	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+	/* setup chip registers to defaults */
+	writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
+	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
+	/*
+	 * This delay caused by an i.MXL hardware bug.
+	 * If no (or too short) delay, no "STOP" bit will be generated.
+	 */
+	udelay(disable_delay);
+	/* Disable I2C controller */
+	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
+}
+
+static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
+							unsigned int rate)
+{
+	unsigned int i2c_clk_rate;
+	unsigned int div;
+	int i;
+
+	/* Divider value calculation */
+	i2c_clk_rate = clk_get_rate(i2c_imx->clk);
+	div = (i2c_clk_rate + rate - 1) / rate;
+	if (div < i2c_clk_div[0][0])
+		i = 0;
+	else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
+		i = ARRAY_SIZE(i2c_clk_div) - 1;
+	else
+		for (i = 0; i2c_clk_div[i][0] < div; i++);
+
+	/* Write divider value to register */
+	writeb(i2c_clk_div[i][1], i2c_imx->base + IMX_I2C_IFDR);
+
+	/*
+	 * There dummy delay is calculated.
+	 * It should be about one I2C clock period long.
+	 * This delay is used in I2C bus disable function
+	 * to fix chip hardware bug.
+	 */
+	disable_delay = (500000U * i2c_clk_div[i][0]
+		+ (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
+
+	/* dev_dbg() can't be used, because adapter is not yet registered */
+#ifdef CONFIG_I2C_DEBUG_BUS
+	printk(KERN_DEBUG "I2C: <%s> I2C_CLK=%d, REQ DIV=%d\n",
+		__func__, i2c_clk_rate, div);
+	printk(KERN_DEBUG "I2C: <%s> IFDR[IC]=0x%x, REAL DIV=%d\n",
+		__func__, i2c_clk_div[i][1], i2c_clk_div[i][0]);
+#endif
+}
+
+static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
+{
+	struct imx_i2c_struct *i2c_imx = dev_id;
+	unsigned int temp;
+
+	temp = readb(i2c_imx->base + IMX_I2C_I2SR);
+	if (temp & I2SR_IIF) {
+		/* save status register */
+		i2c_imx->i2csr = temp;
+		temp &= ~I2SR_IIF;
+		writeb(temp, i2c_imx->base + IMX_I2C_I2SR);
+		wake_up_interruptible(&i2c_imx->queue);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+{
+	int i, result;
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
+		__func__, msgs->addr << 1);
+
+	/* write slave address */
+	writeb(msgs->addr << 1, i2c_imx->base + IMX_I2C_I2DR);
+	result = i2c_imx_trx_complete(i2c_imx);
+	if (result)
+		return result;
+	result = i2c_imx_acked(i2c_imx);
+	if (result)
+		return result;
+	dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
+
+	/* write data */
+	for (i = 0; i < msgs->len; i++) {
+		dev_dbg(&i2c_imx->adapter.dev,
+			"<%s> write byte: B%d=0x%X\n",
+			__func__, i, msgs->buf[i]);
+		writeb(msgs->buf[i], i2c_imx->base + IMX_I2C_I2DR);
+		result = i2c_imx_trx_complete(i2c_imx);
+		if (result)
+			return result;
+		result = i2c_imx_acked(i2c_imx);
+		if (result)
+			return result;
+	}
+	return 0;
+}
+
+static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+{
+	int i, result;
+	unsigned int temp;
+
+	dev_dbg(&i2c_imx->adapter.dev,
+		"<%s> write slave address: addr=0x%x\n",
+		__func__, (msgs->addr << 1) | 0x01);
+
+	/* write slave address */
+	writeb((msgs->addr << 1) | 0x01, i2c_imx->base + IMX_I2C_I2DR);
+	result = i2c_imx_trx_complete(i2c_imx);
+	if (result)
+		return result;
+	result = i2c_imx_acked(i2c_imx);
+	if (result)
+		return result;
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s> setup bus\n", __func__);
+
+	/* setup bus to read data */
+	temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+	temp &= ~I2CR_MTX;
+	if (msgs->len - 1)
+		temp &= ~I2CR_TXAK;
+	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+	readb(i2c_imx->base + IMX_I2C_I2DR); /* dummy read */
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__);
+
+	/* read data */
+	for (i = 0; i < msgs->len; i++) {
+		result = i2c_imx_trx_complete(i2c_imx);
+		if (result)
+			return result;
+		if (i == (msgs->len - 1)) {
+			dev_dbg(&i2c_imx->adapter.dev,
+				"<%s> clear MSTA\n", __func__);
+			temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+			temp &= ~I2CR_MSTA;
+			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+		} else if (i == (msgs->len - 2)) {
+			dev_dbg(&i2c_imx->adapter.dev,
+				"<%s> set TXAK\n", __func__);
+			temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+			temp |= I2CR_TXAK;
+			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+		}
+		msgs->buf[i] = readb(i2c_imx->base + IMX_I2C_I2DR);
+		dev_dbg(&i2c_imx->adapter.dev,
+			"<%s> read byte: B%d=0x%X\n",
+			__func__, i, msgs->buf[i]);
+	}
+	return 0;
+}
+
+static int i2c_imx_xfer(struct i2c_adapter *adapter,
+						struct i2c_msg *msgs, int num)
+{
+	unsigned int i, temp;
+	int result;
+	struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter);
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
+
+	/* Check or i2c bus is not busy */
+	result = i2c_imx_bus_busy(i2c_imx);
+	if (result)
+		goto fail0;
+
+	/* Start I2C transfer */
+	i2c_imx_start(i2c_imx);
+
+	/* read/write data */
+	for (i = 0; i < num; i++) {
+		if (i) {
+			dev_dbg(&i2c_imx->adapter.dev,
+				"<%s> repeated start\n", __func__);
+			temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+			temp |= I2CR_RSTA;
+			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+		}
+		dev_dbg(&i2c_imx->adapter.dev,
+			"<%s> transfer message: %d\n", __func__, i);
+		/* write/read data */
+#ifdef CONFIG_I2C_DEBUG_BUS
+		temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+		dev_dbg(&i2c_imx->adapter.dev, "<%s> CONTROL: IEN=%d, IIEN=%d, "
+			"MSTA=%d, MTX=%d, TXAK=%d, RSTA=%d\n", __func__,
+			(temp & I2CR_IEN ? 1 : 0), (temp & I2CR_IIEN ? 1 : 0),
+			(temp & I2CR_MSTA ? 1 : 0), (temp & I2CR_MTX ? 1 : 0),
+			(temp & I2CR_TXAK ? 1 : 0), (temp & I2CR_RSTA ? 1 : 0));
+		temp = readb(i2c_imx->base + IMX_I2C_I2SR);
+		dev_dbg(&i2c_imx->adapter.dev,
+			"<%s> STATUS: ICF=%d, IAAS=%d, IBB=%d, "
+			"IAL=%d, SRW=%d, IIF=%d, RXAK=%d\n", __func__,
+			(temp & I2SR_ICF ? 1 : 0), (temp & I2SR_IAAS ? 1 : 0),
+			(temp & I2SR_IBB ? 1 : 0), (temp & I2SR_IAL ? 1 : 0),
+			(temp & I2SR_SRW ? 1 : 0), (temp & I2SR_IIF ? 1 : 0),
+			(temp & I2SR_RXAK ? 1 : 0));
+#endif
+		if (msgs[i].flags & I2C_M_RD)
+			result = i2c_imx_read(i2c_imx, &msgs[i]);
+		else
+			result = i2c_imx_write(i2c_imx, &msgs[i]);
+	}
+
+fail0:
+	/* Stop I2C transfer */
+	i2c_imx_stop(i2c_imx);
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
+		(result < 0) ? "error" : "success msg",
+			(result < 0) ? result : num);
+	return (result < 0) ? result : num;
+}
+
+static u32 i2c_imx_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm i2c_imx_algo = {
+	.master_xfer	= i2c_imx_xfer,
+	.functionality	= i2c_imx_func,
+};
+
+static int __init i2c_imx_probe(struct platform_device *pdev)
+{
+	struct imx_i2c_struct *i2c_imx;
+	struct resource *res;
+	struct imxi2c_platform_data *pdata;
+	void __iomem *base;
+	resource_size_t res_size;
+	int irq;
+	int ret;
+
+	dev_dbg(&pdev->dev, "<%s>\n", __func__);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "can't get device resources\n");
+		return -ENODEV;
+	}
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "can't get irq number\n");
+		return -ENODEV;
+	}
+
+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		dev_err(&pdev->dev, "I2C driver needs platform data\n");
+		return -ENODEV;
+	}
+
+	if (pdata->init) {
+		ret = pdata->init(&pdev->dev);
+		if (ret)
+			return ret;
+	}
+
+	res_size = resource_size(res);
+	base = ioremap(res->start, res_size);
+	if (!base) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		ret = -EIO;
+		goto fail0;
+	}
+
+	i2c_imx = kzalloc(sizeof(struct imx_i2c_struct), GFP_KERNEL);
+	if (!i2c_imx) {
+		dev_err(&pdev->dev, "can't allocate interface\n");
+		ret = -ENOMEM;
+		goto fail1;
+	}
+
+	/* Setup i2c_imx driver structure */
+	strcpy(i2c_imx->adapter.name, pdev->name);
+	i2c_imx->adapter.owner		= THIS_MODULE;
+	i2c_imx->adapter.algo		= &i2c_imx_algo;
+	i2c_imx->adapter.dev.parent	= &pdev->dev;
+	i2c_imx->adapter.nr 		= pdev->id;
+	i2c_imx->irq			= irq;
+	i2c_imx->base			= base;
+	i2c_imx->res			= res;
+
+	/* Get I2C clock */
+	i2c_imx->clk = clk_get(&pdev->dev, "i2c_clk");
+	if (IS_ERR(i2c_imx->clk)) {
+		ret = PTR_ERR(i2c_imx->clk);
+		dev_err(&pdev->dev, "can't get I2C clock\n");
+		goto fail2;
+	}
+	clk_enable(i2c_imx->clk);
+
+	/* Request IRQ */
+	ret = request_irq(i2c_imx->irq, i2c_imx_isr, 0, pdev->name, i2c_imx);
+	if (ret) {
+		dev_err(&pdev->dev, "can't claim irq %d\n", i2c_imx->irq);
+		goto fail3;
+	}
+
+	/* Init queue */
+	init_waitqueue_head(&i2c_imx->queue);
+
+	/* Set up adapter data */
+	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
+
+	/* Set up clock divider */
+	if (pdata->bitrate)
+		i2c_imx_set_clk(i2c_imx, pdata->bitrate);
+	else
+		i2c_imx_set_clk(i2c_imx, IMX_I2C_BIT_RATE);
+
+	/* Set up chip registers to defaults */
+	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
+	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
+
+	/* Add I2C adapter */
+	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "registration failed\n");
+		goto fail4;
+	}
+
+	/* Set up platform driver data */
+	platform_set_drvdata(pdev, i2c_imx);
+
+	dev_dbg(&i2c_imx->adapter.dev, "claimed irq %d\n", i2c_imx->irq);
+	dev_dbg(&i2c_imx->adapter.dev, "device resources from 0x%x to 0x%x\n",
+		i2c_imx->res->start, i2c_imx->res->end);
+	dev_dbg(&i2c_imx->adapter.dev, "allocated %d bytes at 0x%x \n",
+		res_size, i2c_imx->res->start);
+	dev_dbg(&i2c_imx->adapter.dev, "adapter name: \"%s\"\n",
+		i2c_imx->adapter.name);
+	dev_dbg(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
+
+	return 0;   /* Return OK */
+
+fail4:
+	free_irq(i2c_imx->irq, i2c_imx);
+fail3:
+	clk_disable(i2c_imx->clk);
+	clk_put(i2c_imx->clk);
+fail2:
+	kfree(i2c_imx);
+fail1:
+	iounmap(base);
+fail0:
+	if (pdata->exit)
+		pdata->exit(&pdev->dev);
+	return ret; /* Return error number */
+}
+
+static int __exit i2c_imx_remove(struct platform_device *pdev)
+{
+	struct imx_i2c_struct *i2c_imx = platform_get_drvdata(pdev);
+	struct imxi2c_platform_data *pdata = pdev->dev.platform_data;
+
+	/* remove adapter */
+	dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
+	i2c_del_adapter(&i2c_imx->adapter);
+	platform_set_drvdata(pdev, NULL);
+
+	/* free interrupt */
+	free_irq(i2c_imx->irq, i2c_imx);
+
+	/* setup chip registers to defaults */
+	writeb(0, i2c_imx->base + IMX_I2C_IADR);
+	writeb(0, i2c_imx->base + IMX_I2C_IFDR);
+	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
+	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
+
+	/* Shut down hardware */
+	if (pdata->exit)
+		pdata->exit(&pdev->dev);
+
+	/* Disable I2C clock */
+	clk_disable(i2c_imx->clk);
+	clk_put(i2c_imx->clk);
+
+	/* Release memory */
+	release_mem_region(i2c_imx->res->start,
+		i2c_imx->res->end - i2c_imx->res->start + 1);
+	iounmap(i2c_imx->base);
+	kfree(i2c_imx);
+	return 0;
+}
+
+static struct platform_driver i2c_imx_driver = {
+	.probe		= i2c_imx_probe,
+	.remove		= __exit_p(i2c_imx_remove),
+	.driver	= {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+	}
+};
+
+static int __init i2c_adap_imx_init(void)
+{
+	return platform_driver_probe(&i2c_imx_driver, i2c_imx_probe);
+}
+
+static void __exit i2c_adap_imx_exit(void)
+{
+	platform_driver_unregister(&i2c_imx_driver);
+}
+
+module_init(i2c_adap_imx_init);
+module_exit(i2c_adap_imx_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Darius Augulis");
+MODULE_DESCRIPTION("I2C adapter driver for IMX I2C bus");
+MODULE_ALIAS("platform:" DRIVER_NAME);
Index: linux-2.6.29-rc5/drivers/i2c/busses/Kconfig
===================================================================
--- linux-2.6.29-rc5.orig/drivers/i2c/busses/Kconfig
+++ linux-2.6.29-rc5/drivers/i2c/busses/Kconfig
@@ -355,6 +355,16 @@ config I2C_IBM_IIC
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-ibm_iic.
 
+config I2C_IMX
+	tristate "IMX I2C interface"
+	depends on ARCH_MXC
+	help
+	  Say Y here if you want to use the IIC bus controller on
+	  the Freescale i.MX/MXC processors.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-imx.
+
 config I2C_IOP3XX
 	tristate "Intel IOPx3xx and IXP4xx on-chip I2C interface"
 	depends on ARCH_IOP32X || ARCH_IOP33X || ARCH_IXP4XX || ARCH_IOP13XX
Index: linux-2.6.29-rc5/drivers/i2c/busses/Makefile
===================================================================
--- linux-2.6.29-rc5.orig/drivers/i2c/busses/Makefile
+++ linux-2.6.29-rc5/drivers/i2c/busses/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_I2C_DAVINCI)	+= i2c-davinci
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
 obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
 obj-$(CONFIG_I2C_IBM_IIC)	+= i2c-ibm_iic.o
+obj-$(CONFIG_I2C_IMX)		+= i2c-imx.o
 obj-$(CONFIG_I2C_IOP3XX)	+= i2c-iop3xx.o
 obj-$(CONFIG_I2C_IXP2000)	+= i2c-ixp2000.o
 obj-$(CONFIG_I2C_MPC)		+= i2c-mpc.o
Index: linux-2.6.29-rc5/arch/arm/plat-mxc/include/mach/i2c.h
===================================================================
--- /dev/null
+++ linux-2.6.29-rc5/arch/arm/plat-mxc/include/mach/i2c.h
@@ -0,0 +1,23 @@
+/*
+ * i2c.h - i.MX I2C driver header file
+ *
+ * Copyright (c) 2008, Darius Augulis <augulis.darius@gmail.com>
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __ASM_ARCH_I2C_H_
+#define __ASM_ARCH_I2C_H_
+
+/*
+ * @init: Initialise gpio's and other board specific things
+ * @exit: Free everything initialised by @init
+ *
+ */
+struct imxi2c_platform_data {
+	int (*init)(struct device *dev);
+	void (*exit)(struct device *dev);
+	int bitrate;
+};
+
+#endif /* __ASM_ARCH_I2C_H_ */

[-- Attachment #3: Type: text/plain, Size: 271 bytes --]

-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 1/5] I2C driver for MXC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

> Implementation of I2C Adapter/Algorithm Driver for I2C Bus integrated
> in Freescale's i.MX/MXC processors. Tested only on MX1, but should work on MX2
> and MX3 too, because all three processors have the same I2C controller.
> 
> Signed-off-by: Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Is this the latest version? There was one where you already received
additional reviewed and tested tags, so I am confused...

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/5] I2C driver for MXC

[Darius]

Wolfram Sang wrote:
>> Implementation of I2C Adapter/Algorithm Driver for I2C Bus integrated
>> in Freescale's i.MX/MXC processors. Tested only on MX1, but should work on MX2
>> and MX3 too, because all three processors have the same I2C controller.
>>
>> Signed-off-by: Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Is this the latest version? There was one where you already received
> additional reviewed and tested tags, so I am confused...

yes, it is the same V5 version posted on I2C list.

> 
> Regards,
> 
>    Wolfram
> 

Re: [PATCH 1/5] I2C driver for MXC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On Tue, Mar 24, 2009 at 11:46:13AM +0200, Darius wrote:

> > Is this the latest version? There was one where you already received
> > additional reviewed and tested tags, so I am confused...
> 
> yes, it is the same V5 version posted on I2C list.

So, is there a reason why you dropped these tags?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Wolfram Sang wrote:
> On Tue, Mar 24, 2009 at 11:46:13AM +0200, Darius wrote:
>
>   
>>> Is this the latest version? There was one where you already received
>>> additional reviewed and tested tags, so I am confused...
>>>       
>> yes, it is the same V5 version posted on I2C list.
>>     
>
> So, is there a reason why you dropped these tags?
>
>   
Because I hurry to submit this, because merge window is open.
I sent all new MXC patches to Sascha and ML in one serie, to be easy 
read and merge them.
However, this I2C patch has very long story and lot of version numbers.
Maintainer should probably take care to merge it now, because I don't 
have time for this furthermore.

Re: [PATCH 1/5] I2C driver for MXC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]


> Because I hurry to submit this, because merge window is open.

Well, adding tags usually helps getting patches accepted. Plus, it is
fair to those who spent time on reviewing them. Simply add those tags to
the patch header next time. Will ack the I2C patch again in this case.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/5] I2C driver for MXC

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1647 bytes --]

On Wed, 25 Mar 2009, Wolfram Sang wrote:

> > Because I hurry to submit this, because merge window is open.

Ok, I waited for this driver, I really did, and I really prefer having one 
driver for as many hardware (SoC) variants as possible instead of having 
different drivers, and I really hoped its new version will work 
out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
it didn't. I've spent more than a full working day trying to get it to 
work, but I didn't succeed so far. It works with the on-board rtc, but it 
doesn't work with the camera, connected over a flex cable.

Attached to this email is a version of the i2c driver for i.mx31 that I've 
been using with my setup for about half a year now. I adjusted it slightly 
to accept the same platform data, so, it is really a drop-in replacement 
for i2c-imx.c. Of course, you have to adjust or extend the Makefile. I 
actually added a new Kconfig entry for this driver, so I could easily 
compare them during the tests.

The source of "my" version does look more logical and more clean to me on 
quite a few occasions. So, maybe someone could give it a quick spin on 
other *mx* SoCs and see if we can use this one instead? You might have to 
replace {read,write}w with readb and writeb, so, it might be a good idea 
to abstract them into inlines or macros. Otherwise, I think, it might work 
for other CPUs straight away.

I just would like to avoid committing a driver and having to spend hours 
or days fixing it afterwards when a possibly more mature version exists.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

[-- Attachment #2: Type: TEXT/X-CSRC, Size: 20440 bytes --]

/*
 * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
 *
 * The code contained herein is licensed under the GNU General Public
 * License. You may obtain a copy of the GNU General Public License
 * Version 2 or later at the following locations:
 *
 * http://www.opensource.org/licenses/gpl-license.html
 * http://www.gnu.org/copyleft/gpl.html
 */

#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
#include <linux/platform_device.h>
#include <linux/i2c.h>
#include <linux/clk.h>
#include <linux/err.h>

#include <asm/irq.h>
#include <asm/io.h>

#include <mach/i2c.h>
#include <mach/iomux-mx3.h>

/* Address offsets of the I2C registers */
#define MXC_IADR                0x00	/* Address Register */
#define MXC_IFDR                0x04	/* Freq div register */
#define MXC_I2CR                0x08	/* Control regsiter */
#define MXC_I2SR                0x0C	/* Status register */
#define MXC_I2DR                0x10	/* Data I/O register */

/* Bit definitions of I2CR */
#define MXC_I2CR_IEN            0x0080
#define MXC_I2CR_IIEN           0x0040
#define MXC_I2CR_MSTA           0x0020
#define MXC_I2CR_MTX            0x0010
#define MXC_I2CR_TXAK           0x0008
#define MXC_I2CR_RSTA           0x0004

/* Bit definitions of I2SR */
#define MXC_I2SR_ICF            0x0080
#define MXC_I2SR_IAAS           0x0040
#define MXC_I2SR_IBB            0x0020
#define MXC_I2SR_IAL            0x0010
#define MXC_I2SR_SRW            0x0004
#define MXC_I2SR_IIF            0x0002
#define MXC_I2SR_RXAK           0x0001

#define MXC_ADAPTER_NAME        "MXC I2C Adapter"

/*
 * In case the MXC device has multiple I2C modules, this structure is used to
 * store information specific to each I2C module.
 */
struct mxc_i2c_device {
	struct i2c_adapter adap;
	wait_queue_head_t wq;		/* Transfer completion wait queue */
	void __iomem *membase;
	int irq;
	unsigned int clkdiv;		/* Default clock divider */
	struct clk *clk;
	bool low_power;			/* Current power state */
	bool transfer_done;
	bool tx_success;		/* ACK received */
	struct resource *res;
};

struct clk_div_table {
	int reg_value;
	int div;
};

static const struct clk_div_table i2c_clk_table[] = {
	{0x20, 22}, {0x21, 24}, {0x22, 26}, {0x23, 28},
	{0, 30}, {1, 32}, {0x24, 32}, {2, 36},
	{0x25, 36}, {0x26, 40}, {3, 42}, {0x27, 44},
	{4, 48}, {0x28, 48}, {5, 52}, {0x29, 56},
	{6, 60}, {0x2A, 64}, {7, 72}, {0x2B, 72},
	{8, 80}, {0x2C, 80}, {9, 88}, {0x2D, 96},
	{0xA, 104}, {0x2E, 112}, {0xB, 128}, {0x2F, 128},
	{0xC, 144}, {0xD, 160}, {0x30, 160}, {0xE, 192},
	{0x31, 192}, {0x32, 224}, {0xF, 240}, {0x33, 256},
	{0x10, 288}, {0x11, 320}, {0x34, 320}, {0x12, 384},
	{0x35, 384}, {0x36, 448}, {0x13, 480}, {0x37, 512},
	{0x14, 576}, {0x15, 640}, {0x38, 640}, {0x16, 768},
	{0x39, 768}, {0x3A, 896}, {0x17, 960}, {0x3B, 1024},
	{0x18, 1152}, {0x19, 1280}, {0x3C, 1280}, {0x1A, 1536},
	{0x3D, 1536}, {0x3E, 1792}, {0x1B, 1920}, {0x3F, 2048},
	{0x1C, 2304}, {0x1D, 2560}, {0x1E, 3072}, {0x1F, 3840},
	{0, 0}
};

/**
 * Transmit a \b STOP signal to the slave device.
 *
 * @param   dev   the mxc i2c structure used to get to the right i2c device
 */
static void mxc_i2c_stop(struct mxc_i2c_device * dev)
{
	unsigned int cr;
	int retry = 200;

	cr = readw(dev->membase + MXC_I2CR);
	cr &= ~(MXC_I2CR_MSTA | MXC_I2CR_MTX);
	writew(cr, dev->membase + MXC_I2CR);

	/*
	 * Make sure STOP meets setup requirement.
	 */
	for (;;) {
		unsigned int sr = readw(dev->membase + MXC_I2SR);
		if ((sr & MXC_I2SR_IBB) == 0)
			break;
		if (retry-- <= 0) {
			printk(KERN_DEBUG "Bus busy\n");
			break;
		}
		udelay(3);
	}
}

/**
 * Wait for the transmission of the data byte to complete. This function waits
 * till we get a signal from the interrupt service routine indicating completion
 * of the address cycle or we time out.
 *
 * @param   dev         the mxc i2c structure used to get to the right i2c device
 * @param   trans_flag  transfer flag
 *
 *
 * @return  The function returns 0 on success or -1 if an ack was not received
 */

static int mxc_i2c_wait_for_tc(struct mxc_i2c_device *dev, int trans_flag)
{
	int retry = 16;

	while (retry-- && !dev->transfer_done)
		wait_event_interruptible_timeout(dev->wq,
						 dev->transfer_done,
						 dev->adap.timeout);

	dev->transfer_done = false;

	if (retry <= 0) {
		/* Unable to send data */
		dev_warn(&dev->adap.dev, "Data not transmitted\n");
		return -ETIMEDOUT;
	}

	if (!dev->tx_success) {
		/* An ACK was not received for transmitted byte */
		dev_dbg(&dev->adap.dev, "ACK not received \n");
		return -EREMOTEIO;
	}

	return 0;
}

/**
 * Transmit a \b START signal to the slave device.
 *
 * @param   dev   the mxc i2c structure used to get to the right i2c device
 * @param   *msg  pointer to a message structure that contains the slave
 *                address
 */
static void mxc_i2c_start(struct mxc_i2c_device *dev, struct i2c_msg *msg)
{
	unsigned int cr, sr;
	unsigned int addr_trans;
	int retry = 16;

	/*
	 * Set the slave address and the requested transfer mode
	 * in the data register
	 */
	addr_trans = msg->addr << 1;
	if (msg->flags & I2C_M_RD)
		addr_trans |= 0x01;

	dev_dbg(&dev->adap.dev, "%s: start %x\n", __func__, msg->addr);

	/* Set the Master bit */
	cr = readw(dev->membase + MXC_I2CR);
	cr |= MXC_I2CR_MSTA;
	writew(cr, dev->membase + MXC_I2CR);

	/* Wait till the Bus Busy bit is set */
	sr = readw(dev->membase + MXC_I2SR);
	while (retry-- && (!(sr & MXC_I2SR_IBB))) {
		udelay(3);
		sr = readw(dev->membase + MXC_I2SR);
	}
	if (retry <= 0)
		dev_warn(&dev->adap.dev, "Could not grab Bus ownership\n");

	/* Set the Transmit bit */
	cr = readw(dev->membase + MXC_I2CR);
	cr |= MXC_I2CR_MTX;
	writew(cr, dev->membase + MXC_I2CR);

	writew(addr_trans, dev->membase + MXC_I2DR);
}

/**
 * Transmit a \b REPEAT START to the slave device
 *
 * @param   dev   the mxc i2c structure used to get to the right i2c device
 * @param   *msg  pointer to a message structure that contains the slave
 *                address
 */
static void mxc_i2c_repstart(struct mxc_i2c_device *dev, struct i2c_msg *msg)
{
	unsigned int cr;
	unsigned int addr_trans;

	/*
	 * Set the slave address and the requested transfer mode
	 * in the data register
	 */
	addr_trans = msg->addr << 1;
	if (msg->flags & I2C_M_RD)
		addr_trans |= 0x01;

	dev_dbg(&dev->adap.dev, "%s: repeat start %x\n", __func__, msg->addr);

	cr = readw(dev->membase + MXC_I2CR);
	cr |= MXC_I2CR_RSTA;
	writew(cr, dev->membase + MXC_I2CR);
	udelay(3);
	writew(addr_trans, dev->membase + MXC_I2DR);
}

/**
 * Read the received data. The function waits till data is available or times
 * out. Generates a stop signal if this is the last message to be received.
 * Sends an ack for all the bytes received except the last byte.
 *
 * @param  dev       the mxc i2c structure used to get to the right i2c device
 * @param  *msg      pointer to a message structure that contains the slave
 *                   address and a pointer to the receive buffer
 * @param  last      indicates that this is the last message to be received
 * @param  addr_comp flag indicates that we just finished the address cycle
 *
 * @return  The function returns the number of bytes read or -1 on time out.
 */
static int mxc_i2c_readbytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
			     int last, int addr_comp)
{
	int i;
	char *buf = msg->buf;
	int len = msg->len;
	unsigned int cr;

	dev_dbg(&dev->adap.dev, "%s: last %d, addr_comp %d\n",
		__func__, last, addr_comp);

	cr = readw(dev->membase + MXC_I2CR);
	/*
	 * Clear MTX to switch to receive mode.
	 */
	cr &= ~MXC_I2CR_MTX;
	/*
	 * Clear the TXAK bit to gen an ack when receiving only one byte.
	 */
	if (len == 1)
		cr |= MXC_I2CR_TXAK;
	else
		cr &= ~MXC_I2CR_TXAK;

	writew(cr, dev->membase + MXC_I2CR);
	/*
	 * Dummy read only at the end of an address cycle
	 */
	if (addr_comp > 0)
		readw(dev->membase + MXC_I2DR);

	for (i = 0; i < len; i++) {
		int ret;
		/*
		 * Wait for data transmission to complete
		 */
		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
		if (ret < 0) {
			dev_err(&dev->adap.dev, "%s: rx #%d of %d failed!\n",
				__func__, i, len);
			mxc_i2c_stop(dev);
			return ret;
		}
		/*
		 * Do not generate an ACK for the last byte
		 */
		if (i == len - 2) {
			cr = readw(dev->membase + MXC_I2CR);
			cr |= MXC_I2CR_TXAK;
			writew(cr, dev->membase + MXC_I2CR);
		} else if (i == len - 1) {
			if (last)
				mxc_i2c_stop(dev);
		}
		/* Read the data */
		*buf++ = readw(dev->membase + MXC_I2DR);
	}

	return i;
}

/**
 * Write the data to the data register. Generates a stop signal if this is
 * the last message to be sent or if no ack was received for the data sent.
 *
 * @param   dev   the mxc i2c structure used to get to the right i2c device
 * @param   *msg  pointer to a message structure that contains the slave
 *                address and data to be sent
 * @param   last  indicates that this is the last message to be received
 *
 * @return  The function returns the number of bytes written or -1 on time out
 *          or if no ack was received for the data that was sent.
 */
static int mxc_i2c_writebytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
			      int last)
{
	int i;
	char *buf = msg->buf;
	int len = msg->len;
	unsigned int cr;

	dev_dbg(&dev->adap.dev, "%s: last %d\n", __func__, last);

	cr = readw(dev->membase + MXC_I2CR);
	/* Set MTX to switch to transmit mode */
	writew(cr | MXC_I2CR_MTX, dev->membase + MXC_I2CR);

	for (i = 0; i < len; i++) {
		int ret;
		/*
		 * Write the data
		 */
		writew(*buf++, dev->membase + MXC_I2DR);
		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
		if (ret < 0) {
			dev_err(&dev->adap.dev, "%s: tx #%d of %d timed out!\n",
				__func__, i, len);
			mxc_i2c_stop(dev);
			return ret;
		}
	}
	if (last > 0)
		mxc_i2c_stop(dev);

	return i;
}

/**
 * Function enables the I2C module and initializes the registers.
 *
 * @param   dev   the mxc i2c structure used to get to the right i2c device
 * @param   trans_flag  transfer flag
 */
static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
{
	clk_enable(dev->clk);
	/* Set the frequency divider */
	writew(dev->clkdiv, dev->membase + MXC_IFDR);
	/* Clear the status register */
	writew(0x0, dev->membase + MXC_I2SR);
	/* Enable I2C and its interrupts */
	writew(MXC_I2CR_IEN, dev->membase + MXC_I2CR);
	writew(MXC_I2CR_IEN | MXC_I2CR_IIEN, dev->membase + MXC_I2CR);
}

/**
 * Disables the I2C module.
 *
 * @param   dev   the mxc i2c structure used to get to the right i2c device
 */
static void mxc_i2c_module_dis(struct mxc_i2c_device * dev)
{
	writew(0x0, dev->membase + MXC_I2CR);
	clk_disable(dev->clk);
}

/**
 * The function is registered in the adapter structure. It is called when an MXC
 * driver wishes to transfer data to a device connected to the I2C device.
 *
 * @param   adap   adapter structure for the MXC i2c device
 * @param   msgs[] array of messages to be transferred to the device
 * @param   num    number of messages to be transferred to the device
 *
 * @return  The function returns the number of messages transferred,
 *          \b -EREMOTEIO on I2C failure and a 0 if the num argument is
 *          less than 0.
 */
static int mxc_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
{
	struct mxc_i2c_device *dev = i2c_get_adapdata(adap);
	int i, ret = 0, addr_comp = 0;
	unsigned int sr;

	if (dev->low_power) {
		dev_warn(&dev->adap.dev, "I2C Device in low power mode\n");
		return -EREMOTEIO;
	}

	if (num < 1)
		return 0;

	dev_dbg(&dev->adap.dev, "%s: xfer %d msgs\n", __func__, num);

	mxc_i2c_module_en(dev, msgs[0].flags);
	sr = readw(dev->membase + MXC_I2SR);
	/*
	 * Check bus state
	 */
	if (sr & MXC_I2SR_IBB) {
		mxc_i2c_module_dis(dev);
		printk(KERN_DEBUG "Bus busy\n");
		return -EREMOTEIO;
	}

	dev->transfer_done = false;
	dev->tx_success = false;
	for (i = 0; i < num && ret >= 0; i++) {
		addr_comp = 0;
		/*
		 * Send the slave address and transfer direction in the
		 * address cycle
		 */
		if (i == 0) {
			/*
			 * Send a start or repeat start signal
			 */
			mxc_i2c_start(dev, &msgs[0]);
			/* Wait for the address cycle to complete */
			if (mxc_i2c_wait_for_tc(dev, msgs[0].flags)) {
				mxc_i2c_stop(dev);
				mxc_i2c_module_dis(dev);
				dev_err(&dev->adap.dev,
					"%s: Address failed!\n", __func__);
				return -EREMOTEIO;
			}
			addr_comp = 1;
		} else {
			/*
			 * Generate repeat start only if required i.e. the
			 * address changed or the transfer direction changed
			 */
			if ((msgs[i].addr != msgs[i - 1].addr) ||
			    ((msgs[i].flags & I2C_M_RD) !=
			     (msgs[i - 1].flags & I2C_M_RD))) {
				mxc_i2c_repstart(dev, &msgs[i]);
				/* Wait for the address cycle to complete */
				if (mxc_i2c_wait_for_tc(dev, msgs[i].flags)) {
					mxc_i2c_stop(dev);
					mxc_i2c_module_dis(dev);
					dev_err(&dev->adap.dev,
						"%s: Address repeat failed!\n",
						__func__);
					return -EREMOTEIO;
				}
				addr_comp = 1;
			}
		}

		/* Transfer the data */
		if (msgs[i].flags & I2C_M_RD) {
			/* Read the data */
			ret = mxc_i2c_readbytes(dev, &msgs[i], i + 1 == num,
						addr_comp);
			if (ret < 0) {
				dev_err(&dev->adap.dev, "mxc_i2c_readbytes: fail.\n");
				break;
			}
		} else {
			/* Write the data */
			ret = mxc_i2c_writebytes(dev, &msgs[i], i + 1 == num);
			if (ret < 0) {
				dev_err(&dev->adap.dev, "mxc_i2c_writebytes: fail.\n");
				break;
			}
		}
	}

	mxc_i2c_module_dis(dev);

	dev_dbg(&dev->adap.dev, "%s: %d msgs success\n", __func__, i);

	/*
	 * Decrease by 1 as we do not want Start message to be included in
	 * the count
	 */
	return i;
}

/**
 * Returns the i2c functionality supported by this driver.
 *
 * @param   adap adapter structure for this i2c device
 *
 * @return Returns the functionality that is supported.
 */
static u32 mxc_i2c_func(struct i2c_adapter *adap)
{
	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
}

static struct i2c_algorithm mxc_i2c_algorithm = {
	.master_xfer = mxc_i2c_xfer,
	.functionality = mxc_i2c_func
};

/*
 * Interrupt Service Routine. It signals to the process about the data transfer
 * completion. Also sets a flag if bus arbitration is lost.
 */
static irqreturn_t mxc_i2c_handler(int irq, void *dev_id)
{
	struct mxc_i2c_device *dev = dev_id;
	unsigned int sr, cr;

	sr = readw(dev->membase + MXC_I2SR);
	cr = readw(dev->membase + MXC_I2CR);

	/*
	 * Clear the interrupt bit
	 */
	writew(0x0, dev->membase + MXC_I2SR);

	if (sr & MXC_I2SR_IAL) {
		printk(KERN_DEBUG "Bus Arbitration lost\n");
	} else {
		/* Interrupt due byte transfer completion */
		dev->tx_success = true;
		/* Check if RXAK is received in Transmit mode */
		if ((cr & MXC_I2CR_MTX) && (sr & MXC_I2SR_RXAK))
			dev->tx_success = false;

		dev->transfer_done = true;
		wake_up_interruptible(&dev->wq);
	}

	return IRQ_HANDLED;
}

static int mxci2c_suspend(struct platform_device *pdev, pm_message_t state)
{
	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
	unsigned int sr;

	if (mxcdev == NULL)
		return -ENODEV;

	/* Prevent further calls to be processed */
	mxcdev->low_power = true;
	/* Wait till we finish the current transfer */
	sr = readw(mxcdev->membase + MXC_I2SR);
	while (sr & MXC_I2SR_IBB) {
		msleep(10);
		sr = readw(mxcdev->membase + MXC_I2SR);
	}

	if (i2c_plat_data->exit)
		i2c_plat_data->exit(&pdev->dev);

	return 0;
}

static int mxci2c_resume(struct platform_device *pdev)
{
	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;

	if (mxcdev == NULL)
		return -ENODEV;

	mxcdev->low_power = false;
	if (i2c_plat_data->init)
		i2c_plat_data->init(&pdev->dev);

	return 0;
}

static int __init mxci2c_probe(struct platform_device *pdev)
{
	struct mxc_i2c_device *mxc_i2c;
	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
	struct resource *res;
	int id = pdev->id;
	u32 clk_freq;
	int ret;
	int i;

	mxc_i2c = kzalloc(sizeof(struct mxc_i2c_device), GFP_KERNEL);
	if (!mxc_i2c)
		return -ENOMEM;

	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
	if (res == NULL) {
		ret = -ENODEV;
		goto egetres;
	}

	if (!request_mem_region(res->start, resource_size(res), res->name)) {
		ret = -ENOMEM;
		goto ereqmemr;
	}

	mxc_i2c->membase = ioremap(res->start, resource_size(res));
	if (!mxc_i2c->membase) {
		ret = -ENOMEM;
		goto eiomap;
	}

	mxc_i2c->res = res;

	/*
	 * Request the I2C interrupt
	 */
	mxc_i2c->irq = platform_get_irq(pdev, 0);
	if (mxc_i2c->irq < 0) {
		ret = mxc_i2c->irq;
		goto epgirq;
	}

	ret = request_irq(mxc_i2c->irq, mxc_i2c_handler,
			  0, pdev->name, mxc_i2c);
	if (ret < 0)
		goto ereqirq;

	init_waitqueue_head(&mxc_i2c->wq);

	mxc_i2c->low_power	= false;
	mxc_i2c->transfer_done	= false;
	mxc_i2c->tx_success	= false;

	if (i2c_plat_data->init)
		i2c_plat_data->init(&pdev->dev);

	mxc_i2c->clk = clk_get(&pdev->dev, "i2c_clk");
	if (IS_ERR(mxc_i2c->clk)) {
		ret = PTR_ERR(mxc_i2c->clk);
		dev_err(&pdev->dev, "can't get I2C clock\n");
		goto eclkget;
	}
	clk_freq = clk_get_rate(mxc_i2c->clk);

	mxc_i2c->clkdiv = -1;
	if (i2c_plat_data->bitrate) {
		/* Calculate divider and round up any fractional part */
		int div = (clk_freq + i2c_plat_data->bitrate - 1) /
			i2c_plat_data->bitrate;
		for (i = 0; i2c_clk_table[i].div != 0; i++) {
			if (i2c_clk_table[i].div >= div) {
				mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
				break;
			}
		}
	}
	if (mxc_i2c->clkdiv == -1) {
		i = ARRAY_SIZE(i2c_clk_table) - 2;
		/* Use max divider */
		mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
	}
	dev_dbg(&pdev->dev, "i2c speed is %d/%d = %d bps, reg val = 0x%02X\n",
		clk_freq, i2c_clk_table[i].div, clk_freq / i2c_clk_table[i].div,
		mxc_i2c->clkdiv);

	/*
	 * Set the adapter information
	 */
	strcpy(mxc_i2c->adap.name, MXC_ADAPTER_NAME);
	mxc_i2c->adap.nr	= id;
	mxc_i2c->adap.algo	= &mxc_i2c_algorithm;
	mxc_i2c->adap.timeout	= 1;
	mxc_i2c->adap.dev.parent= &pdev->dev;
	mxc_i2c->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
	platform_set_drvdata(pdev, mxc_i2c);
	i2c_set_adapdata(&mxc_i2c->adap, mxc_i2c);
	if ((ret = i2c_add_numbered_adapter(&mxc_i2c->adap)) < 0)
		goto eaddadap;

	return 0;

eaddadap:
	platform_set_drvdata(pdev, NULL);
	clk_put(mxc_i2c->clk);
eclkget:
	if (i2c_plat_data->exit)
		i2c_plat_data->exit(&pdev->dev);
	free_irq(mxc_i2c->irq, mxc_i2c);
ereqirq:
epgirq:
	iounmap(mxc_i2c->membase);
eiomap:
	release_mem_region(res->start, resource_size(res));
ereqmemr:
egetres:
	kfree(mxc_i2c);
	dev_err(&pdev->dev, "failed to probe i2c adapter\n");
	return ret;
}

static int __exit mxci2c_remove(struct platform_device *pdev)
{
	struct mxc_i2c_device *mxc_i2c = platform_get_drvdata(pdev);
	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;

	free_irq(mxc_i2c->irq, mxc_i2c);
	i2c_del_adapter(&mxc_i2c->adap);
	if (i2c_plat_data->exit)
		i2c_plat_data->exit(&pdev->dev);
	clk_put(mxc_i2c->clk);
	platform_set_drvdata(pdev, NULL);
	iounmap(mxc_i2c->membase);
	release_mem_region(mxc_i2c->res->start, resource_size(mxc_i2c->res));
	kfree(mxc_i2c);
	return 0;
}

static struct platform_driver mxci2c_driver = {
	.driver = {
		   .name = "imx-i2c",
		   .owner = THIS_MODULE,
	},
	.remove = __exit_p(mxci2c_remove),
	.suspend = mxci2c_suspend,
	.resume = mxci2c_resume,
};

static int __init mxc_i2c_init(void)
{
	return platform_driver_probe(&mxci2c_driver, mxci2c_probe);
}

static void __exit mxc_i2c_exit(void)
{
	platform_driver_unregister(&mxci2c_driver);
}

subsys_initcall(mxc_i2c_init);
module_exit(mxc_i2c_exit);

MODULE_AUTHOR("Freescale Semiconductor, Inc.");
MODULE_DESCRIPTION("MXC I2C driver");
MODULE_LICENSE("GPL");

Re: [PATCH 1/5] I2C driver for MXC

[Ben Dooks]

On Sat, Mar 28, 2009 at 10:36:50PM +0100, Guennadi Liakhovetski wrote:
> On Wed, 25 Mar 2009, Wolfram Sang wrote:
> 
> > > Because I hurry to submit this, because merge window is open.
> 
> Ok, I waited for this driver, I really did, and I really prefer having one 
> driver for as many hardware (SoC) variants as possible instead of having 
> different drivers, and I really hoped its new version will work 
> out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
> it didn't. I've spent more than a full working day trying to get it to 
> work, but I didn't succeed so far. It works with the on-board rtc, but it 
> doesn't work with the camera, connected over a flex cable.
> 
> Attached to this email is a version of the i2c driver for i.mx31 that I've 
> been using with my setup for about half a year now. I adjusted it slightly 
> to accept the same platform data, so, it is really a drop-in replacement 
> for i2c-imx.c. Of course, you have to adjust or extend the Makefile. I 
> actually added a new Kconfig entry for this driver, so I could easily 
> compare them during the tests.
> 
> The source of "my" version does look more logical and more clean to me on 
> quite a few occasions. So, maybe someone could give it a quick spin on 
> other *mx* SoCs and see if we can use this one instead? You might have to 
> replace {read,write}w with readb and writeb, so, it might be a good idea 
> to abstract them into inlines or macros. Otherwise, I think, it might work 
> for other CPUs straight away.
> 
> I just would like to avoid committing a driver and having to spend hours 
> or days fixing it afterwards when a possibly more mature version exists.

I'm going to hold off pushing my current tree until this/tommorow
evening so that people have some time to get back to me on what is
the best way to proceed.
 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> /*
>  * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
>  *
>  * The code contained herein is licensed under the GNU General Public
>  * License. You may obtain a copy of the GNU General Public License
>  * Version 2 or later at the following locations:
>  *
>  * http://www.opensource.org/licenses/gpl-license.html
>  * http://www.gnu.org/copyleft/gpl.html
>  */
> 
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/delay.h>
> #include <linux/interrupt.h>
> #include <linux/platform_device.h>
> #include <linux/i2c.h>
> #include <linux/clk.h>
> #include <linux/err.h>
> 
> #include <asm/irq.h>
> #include <asm/io.h>
> 
> #include <mach/i2c.h>
> #include <mach/iomux-mx3.h>
> 
> /* Address offsets of the I2C registers */
> #define MXC_IADR                0x00	/* Address Register */
> #define MXC_IFDR                0x04	/* Freq div register */
> #define MXC_I2CR                0x08	/* Control regsiter */
> #define MXC_I2SR                0x0C	/* Status register */
> #define MXC_I2DR                0x10	/* Data I/O register */
> 
> /* Bit definitions of I2CR */
> #define MXC_I2CR_IEN            0x0080
> #define MXC_I2CR_IIEN           0x0040
> #define MXC_I2CR_MSTA           0x0020
> #define MXC_I2CR_MTX            0x0010
> #define MXC_I2CR_TXAK           0x0008
> #define MXC_I2CR_RSTA           0x0004
> 
> /* Bit definitions of I2SR */
> #define MXC_I2SR_ICF            0x0080
> #define MXC_I2SR_IAAS           0x0040
> #define MXC_I2SR_IBB            0x0020
> #define MXC_I2SR_IAL            0x0010
> #define MXC_I2SR_SRW            0x0004
> #define MXC_I2SR_IIF            0x0002
> #define MXC_I2SR_RXAK           0x0001
> 
> #define MXC_ADAPTER_NAME        "MXC I2C Adapter"
> 
> /*
>  * In case the MXC device has multiple I2C modules, this structure is used to
>  * store information specific to each I2C module.
>  */
> struct mxc_i2c_device {
> 	struct i2c_adapter adap;
> 	wait_queue_head_t wq;		/* Transfer completion wait queue */
> 	void __iomem *membase;
> 	int irq;
> 	unsigned int clkdiv;		/* Default clock divider */
> 	struct clk *clk;
> 	bool low_power;			/* Current power state */
> 	bool transfer_done;
> 	bool tx_success;		/* ACK received */
> 	struct resource *res;
> };
> 
> struct clk_div_table {
> 	int reg_value;
> 	int div;
> };
> 
> static const struct clk_div_table i2c_clk_table[] = {
> 	{0x20, 22}, {0x21, 24}, {0x22, 26}, {0x23, 28},
> 	{0, 30}, {1, 32}, {0x24, 32}, {2, 36},
> 	{0x25, 36}, {0x26, 40}, {3, 42}, {0x27, 44},
> 	{4, 48}, {0x28, 48}, {5, 52}, {0x29, 56},
> 	{6, 60}, {0x2A, 64}, {7, 72}, {0x2B, 72},
> 	{8, 80}, {0x2C, 80}, {9, 88}, {0x2D, 96},
> 	{0xA, 104}, {0x2E, 112}, {0xB, 128}, {0x2F, 128},
> 	{0xC, 144}, {0xD, 160}, {0x30, 160}, {0xE, 192},
> 	{0x31, 192}, {0x32, 224}, {0xF, 240}, {0x33, 256},
> 	{0x10, 288}, {0x11, 320}, {0x34, 320}, {0x12, 384},
> 	{0x35, 384}, {0x36, 448}, {0x13, 480}, {0x37, 512},
> 	{0x14, 576}, {0x15, 640}, {0x38, 640}, {0x16, 768},
> 	{0x39, 768}, {0x3A, 896}, {0x17, 960}, {0x3B, 1024},
> 	{0x18, 1152}, {0x19, 1280}, {0x3C, 1280}, {0x1A, 1536},
> 	{0x3D, 1536}, {0x3E, 1792}, {0x1B, 1920}, {0x3F, 2048},
> 	{0x1C, 2304}, {0x1D, 2560}, {0x1E, 3072}, {0x1F, 3840},
> 	{0, 0}
> };
> 
> /**
>  * Transmit a \b STOP signal to the slave device.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  */
> static void mxc_i2c_stop(struct mxc_i2c_device * dev)
> {
> 	unsigned int cr;
> 	int retry = 200;
> 
> 	cr = readw(dev->membase + MXC_I2CR);
> 	cr &= ~(MXC_I2CR_MSTA | MXC_I2CR_MTX);
> 	writew(cr, dev->membase + MXC_I2CR);
> 
> 	/*
> 	 * Make sure STOP meets setup requirement.
> 	 */
> 	for (;;) {
> 		unsigned int sr = readw(dev->membase + MXC_I2SR);
> 		if ((sr & MXC_I2SR_IBB) == 0)
> 			break;
> 		if (retry-- <= 0) {
> 			printk(KERN_DEBUG "Bus busy\n");
> 			break;
> 		}
> 		udelay(3);
> 	}
> }
> 
> /**
>  * Wait for the transmission of the data byte to complete. This function waits
>  * till we get a signal from the interrupt service routine indicating completion
>  * of the address cycle or we time out.
>  *
>  * @param   dev         the mxc i2c structure used to get to the right i2c device
>  * @param   trans_flag  transfer flag
>  *
>  *
>  * @return  The function returns 0 on success or -1 if an ack was not received
>  */
> 
> static int mxc_i2c_wait_for_tc(struct mxc_i2c_device *dev, int trans_flag)
> {
> 	int retry = 16;
> 
> 	while (retry-- && !dev->transfer_done)
> 		wait_event_interruptible_timeout(dev->wq,
> 						 dev->transfer_done,
> 						 dev->adap.timeout);
> 
> 	dev->transfer_done = false;
> 
> 	if (retry <= 0) {
> 		/* Unable to send data */
> 		dev_warn(&dev->adap.dev, "Data not transmitted\n");
> 		return -ETIMEDOUT;
> 	}
> 
> 	if (!dev->tx_success) {
> 		/* An ACK was not received for transmitted byte */
> 		dev_dbg(&dev->adap.dev, "ACK not received \n");
> 		return -EREMOTEIO;
> 	}
> 
> 	return 0;
> }
> 
> /**
>  * Transmit a \b START signal to the slave device.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  * @param   *msg  pointer to a message structure that contains the slave
>  *                address
>  */
> static void mxc_i2c_start(struct mxc_i2c_device *dev, struct i2c_msg *msg)
> {
> 	unsigned int cr, sr;
> 	unsigned int addr_trans;
> 	int retry = 16;
> 
> 	/*
> 	 * Set the slave address and the requested transfer mode
> 	 * in the data register
> 	 */
> 	addr_trans = msg->addr << 1;
> 	if (msg->flags & I2C_M_RD)
> 		addr_trans |= 0x01;
> 
> 	dev_dbg(&dev->adap.dev, "%s: start %x\n", __func__, msg->addr);
> 
> 	/* Set the Master bit */
> 	cr = readw(dev->membase + MXC_I2CR);
> 	cr |= MXC_I2CR_MSTA;
> 	writew(cr, dev->membase + MXC_I2CR);
> 
> 	/* Wait till the Bus Busy bit is set */
> 	sr = readw(dev->membase + MXC_I2SR);
> 	while (retry-- && (!(sr & MXC_I2SR_IBB))) {
> 		udelay(3);
> 		sr = readw(dev->membase + MXC_I2SR);
> 	}
> 	if (retry <= 0)
> 		dev_warn(&dev->adap.dev, "Could not grab Bus ownership\n");
> 
> 	/* Set the Transmit bit */
> 	cr = readw(dev->membase + MXC_I2CR);
> 	cr |= MXC_I2CR_MTX;
> 	writew(cr, dev->membase + MXC_I2CR);
> 
> 	writew(addr_trans, dev->membase + MXC_I2DR);
> }
> 
> /**
>  * Transmit a \b REPEAT START to the slave device
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  * @param   *msg  pointer to a message structure that contains the slave
>  *                address
>  */
> static void mxc_i2c_repstart(struct mxc_i2c_device *dev, struct i2c_msg *msg)
> {
> 	unsigned int cr;
> 	unsigned int addr_trans;
> 
> 	/*
> 	 * Set the slave address and the requested transfer mode
> 	 * in the data register
> 	 */
> 	addr_trans = msg->addr << 1;
> 	if (msg->flags & I2C_M_RD)
> 		addr_trans |= 0x01;
> 
> 	dev_dbg(&dev->adap.dev, "%s: repeat start %x\n", __func__, msg->addr);
> 
> 	cr = readw(dev->membase + MXC_I2CR);
> 	cr |= MXC_I2CR_RSTA;
> 	writew(cr, dev->membase + MXC_I2CR);
> 	udelay(3);
> 	writew(addr_trans, dev->membase + MXC_I2DR);
> }
> 
> /**
>  * Read the received data. The function waits till data is available or times
>  * out. Generates a stop signal if this is the last message to be received.
>  * Sends an ack for all the bytes received except the last byte.
>  *
>  * @param  dev       the mxc i2c structure used to get to the right i2c device
>  * @param  *msg      pointer to a message structure that contains the slave
>  *                   address and a pointer to the receive buffer
>  * @param  last      indicates that this is the last message to be received
>  * @param  addr_comp flag indicates that we just finished the address cycle
>  *
>  * @return  The function returns the number of bytes read or -1 on time out.
>  */
> static int mxc_i2c_readbytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
> 			     int last, int addr_comp)
> {
> 	int i;
> 	char *buf = msg->buf;
> 	int len = msg->len;
> 	unsigned int cr;
> 
> 	dev_dbg(&dev->adap.dev, "%s: last %d, addr_comp %d\n",
> 		__func__, last, addr_comp);
> 
> 	cr = readw(dev->membase + MXC_I2CR);
> 	/*
> 	 * Clear MTX to switch to receive mode.
> 	 */
> 	cr &= ~MXC_I2CR_MTX;
> 	/*
> 	 * Clear the TXAK bit to gen an ack when receiving only one byte.
> 	 */
> 	if (len == 1)
> 		cr |= MXC_I2CR_TXAK;
> 	else
> 		cr &= ~MXC_I2CR_TXAK;
> 
> 	writew(cr, dev->membase + MXC_I2CR);
> 	/*
> 	 * Dummy read only at the end of an address cycle
> 	 */
> 	if (addr_comp > 0)
> 		readw(dev->membase + MXC_I2DR);
> 
> 	for (i = 0; i < len; i++) {
> 		int ret;
> 		/*
> 		 * Wait for data transmission to complete
> 		 */
> 		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
> 		if (ret < 0) {
> 			dev_err(&dev->adap.dev, "%s: rx #%d of %d failed!\n",
> 				__func__, i, len);
> 			mxc_i2c_stop(dev);
> 			return ret;
> 		}
> 		/*
> 		 * Do not generate an ACK for the last byte
> 		 */
> 		if (i == len - 2) {
> 			cr = readw(dev->membase + MXC_I2CR);
> 			cr |= MXC_I2CR_TXAK;
> 			writew(cr, dev->membase + MXC_I2CR);
> 		} else if (i == len - 1) {
> 			if (last)
> 				mxc_i2c_stop(dev);
> 		}
> 		/* Read the data */
> 		*buf++ = readw(dev->membase + MXC_I2DR);
> 	}
> 
> 	return i;
> }
> 
> /**
>  * Write the data to the data register. Generates a stop signal if this is
>  * the last message to be sent or if no ack was received for the data sent.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  * @param   *msg  pointer to a message structure that contains the slave
>  *                address and data to be sent
>  * @param   last  indicates that this is the last message to be received
>  *
>  * @return  The function returns the number of bytes written or -1 on time out
>  *          or if no ack was received for the data that was sent.
>  */
> static int mxc_i2c_writebytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
> 			      int last)
> {
> 	int i;
> 	char *buf = msg->buf;
> 	int len = msg->len;
> 	unsigned int cr;
> 
> 	dev_dbg(&dev->adap.dev, "%s: last %d\n", __func__, last);
> 
> 	cr = readw(dev->membase + MXC_I2CR);
> 	/* Set MTX to switch to transmit mode */
> 	writew(cr | MXC_I2CR_MTX, dev->membase + MXC_I2CR);
> 
> 	for (i = 0; i < len; i++) {
> 		int ret;
> 		/*
> 		 * Write the data
> 		 */
> 		writew(*buf++, dev->membase + MXC_I2DR);
> 		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
> 		if (ret < 0) {
> 			dev_err(&dev->adap.dev, "%s: tx #%d of %d timed out!\n",
> 				__func__, i, len);
> 			mxc_i2c_stop(dev);
> 			return ret;
> 		}
> 	}
> 	if (last > 0)
> 		mxc_i2c_stop(dev);
> 
> 	return i;
> }
> 
> /**
>  * Function enables the I2C module and initializes the registers.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  * @param   trans_flag  transfer flag
>  */
> static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
> {
> 	clk_enable(dev->clk);
> 	/* Set the frequency divider */
> 	writew(dev->clkdiv, dev->membase + MXC_IFDR);
> 	/* Clear the status register */
> 	writew(0x0, dev->membase + MXC_I2SR);
> 	/* Enable I2C and its interrupts */
> 	writew(MXC_I2CR_IEN, dev->membase + MXC_I2CR);
> 	writew(MXC_I2CR_IEN | MXC_I2CR_IIEN, dev->membase + MXC_I2CR);
> }
> 
> /**
>  * Disables the I2C module.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  */
> static void mxc_i2c_module_dis(struct mxc_i2c_device * dev)
> {
> 	writew(0x0, dev->membase + MXC_I2CR);
> 	clk_disable(dev->clk);
> }
> 
> /**
>  * The function is registered in the adapter structure. It is called when an MXC
>  * driver wishes to transfer data to a device connected to the I2C device.
>  *
>  * @param   adap   adapter structure for the MXC i2c device
>  * @param   msgs[] array of messages to be transferred to the device
>  * @param   num    number of messages to be transferred to the device
>  *
>  * @return  The function returns the number of messages transferred,
>  *          \b -EREMOTEIO on I2C failure and a 0 if the num argument is
>  *          less than 0.
>  */
> static int mxc_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> {
> 	struct mxc_i2c_device *dev = i2c_get_adapdata(adap);
> 	int i, ret = 0, addr_comp = 0;
> 	unsigned int sr;
> 
> 	if (dev->low_power) {
> 		dev_warn(&dev->adap.dev, "I2C Device in low power mode\n");
> 		return -EREMOTEIO;
> 	}
> 
> 	if (num < 1)
> 		return 0;
> 
> 	dev_dbg(&dev->adap.dev, "%s: xfer %d msgs\n", __func__, num);
> 
> 	mxc_i2c_module_en(dev, msgs[0].flags);
> 	sr = readw(dev->membase + MXC_I2SR);
> 	/*
> 	 * Check bus state
> 	 */
> 	if (sr & MXC_I2SR_IBB) {
> 		mxc_i2c_module_dis(dev);
> 		printk(KERN_DEBUG "Bus busy\n");
> 		return -EREMOTEIO;
> 	}
> 
> 	dev->transfer_done = false;
> 	dev->tx_success = false;
> 	for (i = 0; i < num && ret >= 0; i++) {
> 		addr_comp = 0;
> 		/*
> 		 * Send the slave address and transfer direction in the
> 		 * address cycle
> 		 */
> 		if (i == 0) {
> 			/*
> 			 * Send a start or repeat start signal
> 			 */
> 			mxc_i2c_start(dev, &msgs[0]);
> 			/* Wait for the address cycle to complete */
> 			if (mxc_i2c_wait_for_tc(dev, msgs[0].flags)) {
> 				mxc_i2c_stop(dev);
> 				mxc_i2c_module_dis(dev);
> 				dev_err(&dev->adap.dev,
> 					"%s: Address failed!\n", __func__);
> 				return -EREMOTEIO;
> 			}
> 			addr_comp = 1;
> 		} else {
> 			/*
> 			 * Generate repeat start only if required i.e. the
> 			 * address changed or the transfer direction changed
> 			 */
> 			if ((msgs[i].addr != msgs[i - 1].addr) ||
> 			    ((msgs[i].flags & I2C_M_RD) !=
> 			     (msgs[i - 1].flags & I2C_M_RD))) {
> 				mxc_i2c_repstart(dev, &msgs[i]);
> 				/* Wait for the address cycle to complete */
> 				if (mxc_i2c_wait_for_tc(dev, msgs[i].flags)) {
> 					mxc_i2c_stop(dev);
> 					mxc_i2c_module_dis(dev);
> 					dev_err(&dev->adap.dev,
> 						"%s: Address repeat failed!\n",
> 						__func__);
> 					return -EREMOTEIO;
> 				}
> 				addr_comp = 1;
> 			}
> 		}
> 
> 		/* Transfer the data */
> 		if (msgs[i].flags & I2C_M_RD) {
> 			/* Read the data */
> 			ret = mxc_i2c_readbytes(dev, &msgs[i], i + 1 == num,
> 						addr_comp);
> 			if (ret < 0) {
> 				dev_err(&dev->adap.dev, "mxc_i2c_readbytes: fail.\n");
> 				break;
> 			}
> 		} else {
> 			/* Write the data */
> 			ret = mxc_i2c_writebytes(dev, &msgs[i], i + 1 == num);
> 			if (ret < 0) {
> 				dev_err(&dev->adap.dev, "mxc_i2c_writebytes: fail.\n");
> 				break;
> 			}
> 		}
> 	}
> 
> 	mxc_i2c_module_dis(dev);
> 
> 	dev_dbg(&dev->adap.dev, "%s: %d msgs success\n", __func__, i);
> 
> 	/*
> 	 * Decrease by 1 as we do not want Start message to be included in
> 	 * the count
> 	 */
> 	return i;
> }
> 
> /**
>  * Returns the i2c functionality supported by this driver.
>  *
>  * @param   adap adapter structure for this i2c device
>  *
>  * @return Returns the functionality that is supported.
>  */
> static u32 mxc_i2c_func(struct i2c_adapter *adap)
> {
> 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> }
> 
> static struct i2c_algorithm mxc_i2c_algorithm = {
> 	.master_xfer = mxc_i2c_xfer,
> 	.functionality = mxc_i2c_func
> };
> 
> /*
>  * Interrupt Service Routine. It signals to the process about the data transfer
>  * completion. Also sets a flag if bus arbitration is lost.
>  */
> static irqreturn_t mxc_i2c_handler(int irq, void *dev_id)
> {
> 	struct mxc_i2c_device *dev = dev_id;
> 	unsigned int sr, cr;
> 
> 	sr = readw(dev->membase + MXC_I2SR);
> 	cr = readw(dev->membase + MXC_I2CR);
> 
> 	/*
> 	 * Clear the interrupt bit
> 	 */
> 	writew(0x0, dev->membase + MXC_I2SR);
> 
> 	if (sr & MXC_I2SR_IAL) {
> 		printk(KERN_DEBUG "Bus Arbitration lost\n");
> 	} else {
> 		/* Interrupt due byte transfer completion */
> 		dev->tx_success = true;
> 		/* Check if RXAK is received in Transmit mode */
> 		if ((cr & MXC_I2CR_MTX) && (sr & MXC_I2SR_RXAK))
> 			dev->tx_success = false;
> 
> 		dev->transfer_done = true;
> 		wake_up_interruptible(&dev->wq);
> 	}
> 
> 	return IRQ_HANDLED;
> }
> 
> static int mxci2c_suspend(struct platform_device *pdev, pm_message_t state)
> {
> 	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> 	unsigned int sr;
> 
> 	if (mxcdev == NULL)
> 		return -ENODEV;
> 
> 	/* Prevent further calls to be processed */
> 	mxcdev->low_power = true;
> 	/* Wait till we finish the current transfer */
> 	sr = readw(mxcdev->membase + MXC_I2SR);
> 	while (sr & MXC_I2SR_IBB) {
> 		msleep(10);
> 		sr = readw(mxcdev->membase + MXC_I2SR);
> 	}
> 
> 	if (i2c_plat_data->exit)
> 		i2c_plat_data->exit(&pdev->dev);
> 
> 	return 0;
> }
> 
> static int mxci2c_resume(struct platform_device *pdev)
> {
> 	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> 
> 	if (mxcdev == NULL)
> 		return -ENODEV;
> 
> 	mxcdev->low_power = false;
> 	if (i2c_plat_data->init)
> 		i2c_plat_data->init(&pdev->dev);
> 
> 	return 0;
> }
> 
> static int __init mxci2c_probe(struct platform_device *pdev)
> {
> 	struct mxc_i2c_device *mxc_i2c;
> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> 	struct resource *res;
> 	int id = pdev->id;
> 	u32 clk_freq;
> 	int ret;
> 	int i;
> 
> 	mxc_i2c = kzalloc(sizeof(struct mxc_i2c_device), GFP_KERNEL);
> 	if (!mxc_i2c)
> 		return -ENOMEM;
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	if (res == NULL) {
> 		ret = -ENODEV;
> 		goto egetres;
> 	}
> 
> 	if (!request_mem_region(res->start, resource_size(res), res->name)) {
> 		ret = -ENOMEM;
> 		goto ereqmemr;
> 	}
> 
> 	mxc_i2c->membase = ioremap(res->start, resource_size(res));
> 	if (!mxc_i2c->membase) {
> 		ret = -ENOMEM;
> 		goto eiomap;
> 	}
> 
> 	mxc_i2c->res = res;
> 
> 	/*
> 	 * Request the I2C interrupt
> 	 */
> 	mxc_i2c->irq = platform_get_irq(pdev, 0);
> 	if (mxc_i2c->irq < 0) {
> 		ret = mxc_i2c->irq;
> 		goto epgirq;
> 	}
> 
> 	ret = request_irq(mxc_i2c->irq, mxc_i2c_handler,
> 			  0, pdev->name, mxc_i2c);
> 	if (ret < 0)
> 		goto ereqirq;
> 
> 	init_waitqueue_head(&mxc_i2c->wq);
> 
> 	mxc_i2c->low_power	= false;
> 	mxc_i2c->transfer_done	= false;
> 	mxc_i2c->tx_success	= false;
> 
> 	if (i2c_plat_data->init)
> 		i2c_plat_data->init(&pdev->dev);
> 
> 	mxc_i2c->clk = clk_get(&pdev->dev, "i2c_clk");
> 	if (IS_ERR(mxc_i2c->clk)) {
> 		ret = PTR_ERR(mxc_i2c->clk);
> 		dev_err(&pdev->dev, "can't get I2C clock\n");
> 		goto eclkget;
> 	}
> 	clk_freq = clk_get_rate(mxc_i2c->clk);
> 
> 	mxc_i2c->clkdiv = -1;
> 	if (i2c_plat_data->bitrate) {
> 		/* Calculate divider and round up any fractional part */
> 		int div = (clk_freq + i2c_plat_data->bitrate - 1) /
> 			i2c_plat_data->bitrate;
> 		for (i = 0; i2c_clk_table[i].div != 0; i++) {
> 			if (i2c_clk_table[i].div >= div) {
> 				mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
> 				break;
> 			}
> 		}
> 	}
> 	if (mxc_i2c->clkdiv == -1) {
> 		i = ARRAY_SIZE(i2c_clk_table) - 2;
> 		/* Use max divider */
> 		mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
> 	}
> 	dev_dbg(&pdev->dev, "i2c speed is %d/%d = %d bps, reg val = 0x%02X\n",
> 		clk_freq, i2c_clk_table[i].div, clk_freq / i2c_clk_table[i].div,
> 		mxc_i2c->clkdiv);
> 
> 	/*
> 	 * Set the adapter information
> 	 */
> 	strcpy(mxc_i2c->adap.name, MXC_ADAPTER_NAME);
> 	mxc_i2c->adap.nr	= id;
> 	mxc_i2c->adap.algo	= &mxc_i2c_algorithm;
> 	mxc_i2c->adap.timeout	= 1;
> 	mxc_i2c->adap.dev.parent= &pdev->dev;
> 	mxc_i2c->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
> 	platform_set_drvdata(pdev, mxc_i2c);
> 	i2c_set_adapdata(&mxc_i2c->adap, mxc_i2c);
> 	if ((ret = i2c_add_numbered_adapter(&mxc_i2c->adap)) < 0)
> 		goto eaddadap;
> 
> 	return 0;
> 
> eaddadap:
> 	platform_set_drvdata(pdev, NULL);
> 	clk_put(mxc_i2c->clk);
> eclkget:
> 	if (i2c_plat_data->exit)
> 		i2c_plat_data->exit(&pdev->dev);
> 	free_irq(mxc_i2c->irq, mxc_i2c);
> ereqirq:
> epgirq:
> 	iounmap(mxc_i2c->membase);
> eiomap:
> 	release_mem_region(res->start, resource_size(res));
> ereqmemr:
> egetres:
> 	kfree(mxc_i2c);
> 	dev_err(&pdev->dev, "failed to probe i2c adapter\n");
> 	return ret;
> }
> 
> static int __exit mxci2c_remove(struct platform_device *pdev)
> {
> 	struct mxc_i2c_device *mxc_i2c = platform_get_drvdata(pdev);
> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> 
> 	free_irq(mxc_i2c->irq, mxc_i2c);
> 	i2c_del_adapter(&mxc_i2c->adap);
> 	if (i2c_plat_data->exit)
> 		i2c_plat_data->exit(&pdev->dev);
> 	clk_put(mxc_i2c->clk);
> 	platform_set_drvdata(pdev, NULL);
> 	iounmap(mxc_i2c->membase);
> 	release_mem_region(mxc_i2c->res->start, resource_size(mxc_i2c->res));
> 	kfree(mxc_i2c);
> 	return 0;
> }
> 
> static struct platform_driver mxci2c_driver = {
> 	.driver = {
> 		   .name = "imx-i2c",
> 		   .owner = THIS_MODULE,
> 	},
> 	.remove = __exit_p(mxci2c_remove),
> 	.suspend = mxci2c_suspend,
> 	.resume = mxci2c_resume,
> };
> 
> static int __init mxc_i2c_init(void)
> {
> 	return platform_driver_probe(&mxci2c_driver, mxci2c_probe);
> }
> 
> static void __exit mxc_i2c_exit(void)
> {
> 	platform_driver_unregister(&mxci2c_driver);
> }
> 
> subsys_initcall(mxc_i2c_init);
> module_exit(mxc_i2c_exit);
> 
> MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> MODULE_DESCRIPTION("MXC I2C driver");
> MODULE_LICENSE("GPL");


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

  'a smiley only costs 4 bytes'

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

I tested your driver on i.MXL with OV7670 and MT9V111 cameras. I confirm it
works.
In otherside, my driver is also tested with OV7670, MT9V111, OV7720 cameras,
PCF8575 expander and NT7651 LCD controller. All these devices work connected
to i.MXL.
If you have problems on MX3 only with single device, it may be caused by
some specific conditions.
I don't know it is good idea to replace one driver by another, because we
don't know where is the problem exactly.
Of course would be fine to watch your I2C bus with oscilloscope. So you
could see what is wrong.
IMO we should find what is wrong in my driver, to have good working driver
for all MXC SoC's.
If we replace it by your driver, we miss lot of ML comments fixed and
probably we will receive errors and problems in some different situation.

Darius A.


On Sun, Mar 29, 2009 at 12:36 AM, Guennadi Liakhovetski <
g.liakhovetski@pengutronix.de> wrote:

> On Wed, 25 Mar 2009, Wolfram Sang wrote:
>
> > > Because I hurry to submit this, because merge window is open.
>
> Ok, I waited for this driver, I really did, and I really prefer having one
> driver for as many hardware (SoC) variants as possible instead of having
> different drivers, and I really hoped its new version will work
> out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately,
> it didn't. I've spent more than a full working day trying to get it to
> work, but I didn't succeed so far. It works with the on-board rtc, but it
> doesn't work with the camera, connected over a flex cable.
>
> Attached to this email is a version of the i2c driver for i.mx31 that I've
> been using with my setup for about half a year now. I adjusted it slightly
> to accept the same platform data, so, it is really a drop-in replacement
> for i2c-imx.c. Of course, you have to adjust or extend the Makefile. I
> actually added a new Kconfig entry for this driver, so I could easily
> compare them during the tests.
>
> The source of "my" version does look more logical and more clean to me on
> quite a few occasions. So, maybe someone could give it a quick spin on
> other *mx* SoCs and see if we can use this one instead? You might have to
> replace {read,write}w with readb and writeb, so, it might be a good idea
> to abstract them into inlines or macros. Otherwise, I think, it might work
> for other CPUs straight away.
>
> I just would like to avoid committing a driver and having to spend hours
> or days fixing it afterwards when a possibly more mature version exists.
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 1/5] I2C driver for MXC

[Sascha Hauer]

Hi Guennadi,

On Sat, Mar 28, 2009 at 10:36:50PM +0100, Guennadi Liakhovetski wrote:
> On Wed, 25 Mar 2009, Wolfram Sang wrote:
> 
> > > Because I hurry to submit this, because merge window is open.
> 
> Ok, I waited for this driver, I really did, and I really prefer having one 
> driver for as many hardware (SoC) variants as possible instead of having 
> different drivers, and I really hoped its new version will work 
> out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
> it didn't. I've spent more than a full working day trying to get it to 
> work, but I didn't succeed so far. It works with the on-board rtc, but it 
> doesn't work with the camera, connected over a flex cable.
> 
> Attached to this email is a version of the i2c driver for i.mx31 that I've 
> been using with my setup for about half a year now.

Then why didn't you send this driver half a year ago??

> I adjusted it slightly 
> to accept the same platform data, so, it is really a drop-in replacement 
> for i2c-imx.c. Of course, you have to adjust or extend the Makefile. I 
> actually added a new Kconfig entry for this driver, so I could easily 
> compare them during the tests.
> 
> The source of "my" version does look more logical and more clean to me on 
> quite a few occasions. So, maybe someone could give it a quick spin on 
> other *mx* SoCs and see if we can use this one instead? You might have to 
> replace {read,write}w with readb and writeb, so, it might be a good idea 
> to abstract them into inlines or macros. Otherwise, I think, it might work 
> for other CPUs straight away.
> 
> I just would like to avoid committing a driver and having to spend hours 
> or days fixing it afterwards when a possibly more mature version exists.

I just had a quick look over this driver, some comments inline. I think
your driver has some pros, but given the fact that we are in the middle
of the merge window and this driver is not ready for direct inclusion
I'd say that you are simply too late.

Sascha

> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> /*
>  * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
>  *
>  * The code contained herein is licensed under the GNU General Public
>  * License. You may obtain a copy of the GNU General Public License
>  * Version 2 or later at the following locations:
>  *
>  * http://www.opensource.org/licenses/gpl-license.html
>  * http://www.gnu.org/copyleft/gpl.html
>  */
> 
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/delay.h>
> #include <linux/interrupt.h>
> #include <linux/platform_device.h>
> #include <linux/i2c.h>
> #include <linux/clk.h>
> #include <linux/err.h>
> 
> #include <asm/irq.h>
> #include <asm/io.h>
> 
> #include <mach/i2c.h>
> #include <mach/iomux-mx3.h>
> 
> /* Address offsets of the I2C registers */
> #define MXC_IADR                0x00	/* Address Register */
> #define MXC_IFDR                0x04	/* Freq div register */
> #define MXC_I2CR                0x08	/* Control regsiter */
> #define MXC_I2SR                0x0C	/* Status register */
> #define MXC_I2DR                0x10	/* Data I/O register */
> 
> /* Bit definitions of I2CR */
> #define MXC_I2CR_IEN            0x0080
> #define MXC_I2CR_IIEN           0x0040
> #define MXC_I2CR_MSTA           0x0020
> #define MXC_I2CR_MTX            0x0010
> #define MXC_I2CR_TXAK           0x0008
> #define MXC_I2CR_RSTA           0x0004
> 
> /* Bit definitions of I2SR */
> #define MXC_I2SR_ICF            0x0080
> #define MXC_I2SR_IAAS           0x0040
> #define MXC_I2SR_IBB            0x0020
> #define MXC_I2SR_IAL            0x0010
> #define MXC_I2SR_SRW            0x0004
> #define MXC_I2SR_IIF            0x0002
> #define MXC_I2SR_RXAK           0x0001
> 
> #define MXC_ADAPTER_NAME        "MXC I2C Adapter"
> 
> /*
>  * In case the MXC device has multiple I2C modules, this structure is used to
>  * store information specific to each I2C module.
>  */
> struct mxc_i2c_device {
> 	struct i2c_adapter adap;
> 	wait_queue_head_t wq;		/* Transfer completion wait queue */
> 	void __iomem *membase;
> 	int irq;
> 	unsigned int clkdiv;		/* Default clock divider */
> 	struct clk *clk;
> 	bool low_power;			/* Current power state */
> 	bool transfer_done;
> 	bool tx_success;		/* ACK received */
> 	struct resource *res;
> };
> 
> struct clk_div_table {
> 	int reg_value;
> 	int div;
> };
> 
> static const struct clk_div_table i2c_clk_table[] = {
> 	{0x20, 22}, {0x21, 24}, {0x22, 26}, {0x23, 28},
> 	{0, 30}, {1, 32}, {0x24, 32}, {2, 36},
> 	{0x25, 36}, {0x26, 40}, {3, 42}, {0x27, 44},
> 	{4, 48}, {0x28, 48}, {5, 52}, {0x29, 56},
> 	{6, 60}, {0x2A, 64}, {7, 72}, {0x2B, 72},
> 	{8, 80}, {0x2C, 80}, {9, 88}, {0x2D, 96},
> 	{0xA, 104}, {0x2E, 112}, {0xB, 128}, {0x2F, 128},
> 	{0xC, 144}, {0xD, 160}, {0x30, 160}, {0xE, 192},
> 	{0x31, 192}, {0x32, 224}, {0xF, 240}, {0x33, 256},
> 	{0x10, 288}, {0x11, 320}, {0x34, 320}, {0x12, 384},
> 	{0x35, 384}, {0x36, 448}, {0x13, 480}, {0x37, 512},
> 	{0x14, 576}, {0x15, 640}, {0x38, 640}, {0x16, 768},
> 	{0x39, 768}, {0x3A, 896}, {0x17, 960}, {0x3B, 1024},
> 	{0x18, 1152}, {0x19, 1280}, {0x3C, 1280}, {0x1A, 1536},
> 	{0x3D, 1536}, {0x3E, 1792}, {0x1B, 1920}, {0x3F, 2048},
> 	{0x1C, 2304}, {0x1D, 2560}, {0x1E, 3072}, {0x1F, 3840},
> 	{0, 0}
> };

Use of struct instead of a two dimensional array, 1:0 for your driver.

> 
> /**
>  * Transmit a \b STOP signal to the slave device.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  */
> static void mxc_i2c_stop(struct mxc_i2c_device * dev)
> {
> 	unsigned int cr;
> 	int retry = 200;
> 
> 	cr = readw(dev->membase + MXC_I2CR);
> 	cr &= ~(MXC_I2CR_MSTA | MXC_I2CR_MTX);
> 	writew(cr, dev->membase + MXC_I2CR);
> 
> 	/*
> 	 * Make sure STOP meets setup requirement.
> 	 */
> 	for (;;) {
> 		unsigned int sr = readw(dev->membase + MXC_I2SR);
> 		if ((sr & MXC_I2SR_IBB) == 0)
> 			break;
> 		if (retry-- <= 0) {
> 			printk(KERN_DEBUG "Bus busy\n");
> 			break;
> 		}
> 		udelay(3);
> 	}
> }
> 
> /**
>  * Wait for the transmission of the data byte to complete. This function waits
>  * till we get a signal from the interrupt service routine indicating completion
>  * of the address cycle or we time out.
>  *
>  * @param   dev         the mxc i2c structure used to get to the right i2c device
>  * @param   trans_flag  transfer flag
>  *
>  *
>  * @return  The function returns 0 on success or -1 if an ack was not received
>  */
> 
> static int mxc_i2c_wait_for_tc(struct mxc_i2c_device *dev, int trans_flag)
> {
> 	int retry = 16;
> 
> 	while (retry-- && !dev->transfer_done)
> 		wait_event_interruptible_timeout(dev->wq,
> 						 dev->transfer_done,
> 						 dev->adap.timeout);
> 
> 	dev->transfer_done = false;
> 
> 	if (retry <= 0) {
> 		/* Unable to send data */
> 		dev_warn(&dev->adap.dev, "Data not transmitted\n");
> 		return -ETIMEDOUT;
> 	}
> 
> 	if (!dev->tx_success) {
> 		/* An ACK was not received for transmitted byte */
> 		dev_dbg(&dev->adap.dev, "ACK not received \n");
> 		return -EREMOTEIO;
> 	}
> 
> 	return 0;
> }
> 
> /**
>  * Transmit a \b START signal to the slave device.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  * @param   *msg  pointer to a message structure that contains the slave
>  *                address
>  */
> static void mxc_i2c_start(struct mxc_i2c_device *dev, struct i2c_msg *msg)
> {
> 	unsigned int cr, sr;
> 	unsigned int addr_trans;
> 	int retry = 16;
> 
> 	/*
> 	 * Set the slave address and the requested transfer mode
> 	 * in the data register
> 	 */
> 	addr_trans = msg->addr << 1;
> 	if (msg->flags & I2C_M_RD)
> 		addr_trans |= 0x01;
> 
> 	dev_dbg(&dev->adap.dev, "%s: start %x\n", __func__, msg->addr);
> 
> 	/* Set the Master bit */
> 	cr = readw(dev->membase + MXC_I2CR);
> 	cr |= MXC_I2CR_MSTA;
> 	writew(cr, dev->membase + MXC_I2CR);
> 
> 	/* Wait till the Bus Busy bit is set */
> 	sr = readw(dev->membase + MXC_I2SR);
> 	while (retry-- && (!(sr & MXC_I2SR_IBB))) {
> 		udelay(3);
> 		sr = readw(dev->membase + MXC_I2SR);
> 	}
> 	if (retry <= 0)
> 		dev_warn(&dev->adap.dev, "Could not grab Bus ownership\n");
> 
> 	/* Set the Transmit bit */
> 	cr = readw(dev->membase + MXC_I2CR);
> 	cr |= MXC_I2CR_MTX;
> 	writew(cr, dev->membase + MXC_I2CR);
> 
> 	writew(addr_trans, dev->membase + MXC_I2DR);
> }
> 
> /**
>  * Transmit a \b REPEAT START to the slave device
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  * @param   *msg  pointer to a message structure that contains the slave
>  *                address
>  */
> static void mxc_i2c_repstart(struct mxc_i2c_device *dev, struct i2c_msg *msg)
> {
> 	unsigned int cr;
> 	unsigned int addr_trans;
> 
> 	/*
> 	 * Set the slave address and the requested transfer mode
> 	 * in the data register
> 	 */
> 	addr_trans = msg->addr << 1;
> 	if (msg->flags & I2C_M_RD)
> 		addr_trans |= 0x01;
> 
> 	dev_dbg(&dev->adap.dev, "%s: repeat start %x\n", __func__, msg->addr);
> 
> 	cr = readw(dev->membase + MXC_I2CR);
> 	cr |= MXC_I2CR_RSTA;
> 	writew(cr, dev->membase + MXC_I2CR);
> 	udelay(3);
> 	writew(addr_trans, dev->membase + MXC_I2DR);
> }
> 
> /**
>  * Read the received data. The function waits till data is available or times
>  * out. Generates a stop signal if this is the last message to be received.
>  * Sends an ack for all the bytes received except the last byte.
>  *
>  * @param  dev       the mxc i2c structure used to get to the right i2c device
>  * @param  *msg      pointer to a message structure that contains the slave
>  *                   address and a pointer to the receive buffer
>  * @param  last      indicates that this is the last message to be received
>  * @param  addr_comp flag indicates that we just finished the address cycle
>  *
>  * @return  The function returns the number of bytes read or -1 on time out.
>  */
> static int mxc_i2c_readbytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
> 			     int last, int addr_comp)
> {
> 	int i;
> 	char *buf = msg->buf;
> 	int len = msg->len;
> 	unsigned int cr;
> 
> 	dev_dbg(&dev->adap.dev, "%s: last %d, addr_comp %d\n",
> 		__func__, last, addr_comp);
> 
> 	cr = readw(dev->membase + MXC_I2CR);
> 	/*
> 	 * Clear MTX to switch to receive mode.
> 	 */
> 	cr &= ~MXC_I2CR_MTX;
> 	/*
> 	 * Clear the TXAK bit to gen an ack when receiving only one byte.
> 	 */
> 	if (len == 1)
> 		cr |= MXC_I2CR_TXAK;
> 	else
> 		cr &= ~MXC_I2CR_TXAK;
> 
> 	writew(cr, dev->membase + MXC_I2CR);
> 	/*
> 	 * Dummy read only at the end of an address cycle
> 	 */
> 	if (addr_comp > 0)
> 		readw(dev->membase + MXC_I2DR);
> 
> 	for (i = 0; i < len; i++) {
> 		int ret;
> 		/*
> 		 * Wait for data transmission to complete
> 		 */
> 		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
> 		if (ret < 0) {
> 			dev_err(&dev->adap.dev, "%s: rx #%d of %d failed!\n",
> 				__func__, i, len);
> 			mxc_i2c_stop(dev);
> 			return ret;
> 		}
> 		/*
> 		 * Do not generate an ACK for the last byte
> 		 */
> 		if (i == len - 2) {
> 			cr = readw(dev->membase + MXC_I2CR);
> 			cr |= MXC_I2CR_TXAK;
> 			writew(cr, dev->membase + MXC_I2CR);
> 		} else if (i == len - 1) {
> 			if (last)
> 				mxc_i2c_stop(dev);
> 		}
> 		/* Read the data */
> 		*buf++ = readw(dev->membase + MXC_I2DR);
> 	}
> 
> 	return i;
> }
> 
> /**
>  * Write the data to the data register. Generates a stop signal if this is
>  * the last message to be sent or if no ack was received for the data sent.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  * @param   *msg  pointer to a message structure that contains the slave
>  *                address and data to be sent
>  * @param   last  indicates that this is the last message to be received
>  *
>  * @return  The function returns the number of bytes written or -1 on time out
>  *          or if no ack was received for the data that was sent.
>  */
> static int mxc_i2c_writebytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
> 			      int last)
> {
> 	int i;
> 	char *buf = msg->buf;
> 	int len = msg->len;
> 	unsigned int cr;
> 
> 	dev_dbg(&dev->adap.dev, "%s: last %d\n", __func__, last);
> 
> 	cr = readw(dev->membase + MXC_I2CR);
> 	/* Set MTX to switch to transmit mode */
> 	writew(cr | MXC_I2CR_MTX, dev->membase + MXC_I2CR);
> 
> 	for (i = 0; i < len; i++) {
> 		int ret;
> 		/*
> 		 * Write the data
> 		 */
> 		writew(*buf++, dev->membase + MXC_I2DR);
> 		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
> 		if (ret < 0) {
> 			dev_err(&dev->adap.dev, "%s: tx #%d of %d timed out!\n",
> 				__func__, i, len);
> 			mxc_i2c_stop(dev);
> 			return ret;
> 		}
> 	}
> 	if (last > 0)
> 		mxc_i2c_stop(dev);
> 
> 	return i;
> }
> 
> /**
>  * Function enables the I2C module and initializes the registers.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  * @param   trans_flag  transfer flag
>  */
> static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
> {
> 	clk_enable(dev->clk);
> 	/* Set the frequency divider */
> 	writew(dev->clkdiv, dev->membase + MXC_IFDR);
> 	/* Clear the status register */
> 	writew(0x0, dev->membase + MXC_I2SR);
> 	/* Enable I2C and its interrupts */
> 	writew(MXC_I2CR_IEN, dev->membase + MXC_I2CR);
> 	writew(MXC_I2CR_IEN | MXC_I2CR_IIEN, dev->membase + MXC_I2CR);
> }
> 
> /**
>  * Disables the I2C module.
>  *
>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>  */
> static void mxc_i2c_module_dis(struct mxc_i2c_device * dev)
> {
> 	writew(0x0, dev->membase + MXC_I2CR);
> 	clk_disable(dev->clk);
> }
> 
> /**
>  * The function is registered in the adapter structure. It is called when an MXC
>  * driver wishes to transfer data to a device connected to the I2C device.
>  *
>  * @param   adap   adapter structure for the MXC i2c device
>  * @param   msgs[] array of messages to be transferred to the device
>  * @param   num    number of messages to be transferred to the device
>  *
>  * @return  The function returns the number of messages transferred,
>  *          \b -EREMOTEIO on I2C failure and a 0 if the num argument is
>  *          less than 0.
>  */
> static int mxc_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> {
> 	struct mxc_i2c_device *dev = i2c_get_adapdata(adap);
> 	int i, ret = 0, addr_comp = 0;
> 	unsigned int sr;
> 
> 	if (dev->low_power) {
> 		dev_warn(&dev->adap.dev, "I2C Device in low power mode\n");
> 		return -EREMOTEIO;
> 	}
> 
> 	if (num < 1)
> 		return 0;
> 
> 	dev_dbg(&dev->adap.dev, "%s: xfer %d msgs\n", __func__, num);
> 
> 	mxc_i2c_module_en(dev, msgs[0].flags);
> 	sr = readw(dev->membase + MXC_I2SR);
> 	/*
> 	 * Check bus state
> 	 */
> 	if (sr & MXC_I2SR_IBB) {
> 		mxc_i2c_module_dis(dev);
> 		printk(KERN_DEBUG "Bus busy\n");
> 		return -EREMOTEIO;
> 	}
> 
> 	dev->transfer_done = false;
> 	dev->tx_success = false;
> 	for (i = 0; i < num && ret >= 0; i++) {
> 		addr_comp = 0;
> 		/*
> 		 * Send the slave address and transfer direction in the
> 		 * address cycle
> 		 */
> 		if (i == 0) {
> 			/*
> 			 * Send a start or repeat start signal
> 			 */
> 			mxc_i2c_start(dev, &msgs[0]);
> 			/* Wait for the address cycle to complete */
> 			if (mxc_i2c_wait_for_tc(dev, msgs[0].flags)) {
> 				mxc_i2c_stop(dev);
> 				mxc_i2c_module_dis(dev);
> 				dev_err(&dev->adap.dev,
> 					"%s: Address failed!\n", __func__);
> 				return -EREMOTEIO;
> 			}
> 			addr_comp = 1;

This looks strange. Why is this block inside the loop?

> 		} else {
> 			/*
> 			 * Generate repeat start only if required i.e. the
> 			 * address changed or the transfer direction changed
> 			 */
> 			if ((msgs[i].addr != msgs[i - 1].addr) ||
> 			    ((msgs[i].flags & I2C_M_RD) !=
> 			     (msgs[i - 1].flags & I2C_M_RD))) {
> 				mxc_i2c_repstart(dev, &msgs[i]);
> 				/* Wait for the address cycle to complete */
> 				if (mxc_i2c_wait_for_tc(dev, msgs[i].flags)) {
> 					mxc_i2c_stop(dev);
> 					mxc_i2c_module_dis(dev);
> 					dev_err(&dev->adap.dev,
> 						"%s: Address repeat failed!\n",
> 						__func__);
> 					return -EREMOTEIO;
> 				}
> 				addr_comp = 1;
> 			}
> 		}
> 
> 		/* Transfer the data */
> 		if (msgs[i].flags & I2C_M_RD) {
> 			/* Read the data */
> 			ret = mxc_i2c_readbytes(dev, &msgs[i], i + 1 == num,
> 						addr_comp);
> 			if (ret < 0) {
> 				dev_err(&dev->adap.dev, "mxc_i2c_readbytes: fail.\n");
> 				break;
> 			}
> 		} else {
> 			/* Write the data */
> 			ret = mxc_i2c_writebytes(dev, &msgs[i], i + 1 == num);
> 			if (ret < 0) {
> 				dev_err(&dev->adap.dev, "mxc_i2c_writebytes: fail.\n");
> 				break;
> 			}
> 		}
> 	}
> 
> 	mxc_i2c_module_dis(dev);
> 
> 	dev_dbg(&dev->adap.dev, "%s: %d msgs success\n", __func__, i);
> 
> 	/*
> 	 * Decrease by 1 as we do not want Start message to be included in
> 	 * the count
> 	 */
> 	return i;

Original Freescale code returned  i - 1 here which was wrong. The code
is correct, but you should remove the comment.

> }
> 
> /**
>  * Returns the i2c functionality supported by this driver.
>  *
>  * @param   adap adapter structure for this i2c device
>  *
>  * @return Returns the functionality that is supported.
>  */
> static u32 mxc_i2c_func(struct i2c_adapter *adap)
> {
> 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> }
> 
> static struct i2c_algorithm mxc_i2c_algorithm = {
> 	.master_xfer = mxc_i2c_xfer,
> 	.functionality = mxc_i2c_func
> };
> 
> /*
>  * Interrupt Service Routine. It signals to the process about the data transfer
>  * completion. Also sets a flag if bus arbitration is lost.

This comment is not true. All it does is a printk. The other driver does
not handle this situation, too.

>  */
> static irqreturn_t mxc_i2c_handler(int irq, void *dev_id)
> {
> 	struct mxc_i2c_device *dev = dev_id;
> 	unsigned int sr, cr;
> 
> 	sr = readw(dev->membase + MXC_I2SR);
> 	cr = readw(dev->membase + MXC_I2CR);
> 
> 	/*
> 	 * Clear the interrupt bit
> 	 */
> 	writew(0x0, dev->membase + MXC_I2SR);
> 
> 	if (sr & MXC_I2SR_IAL) {
> 		printk(KERN_DEBUG "Bus Arbitration lost\n");
> 	} else {
> 		/* Interrupt due byte transfer completion */
> 		dev->tx_success = true;
> 		/* Check if RXAK is received in Transmit mode */
> 		if ((cr & MXC_I2CR_MTX) && (sr & MXC_I2SR_RXAK))
> 			dev->tx_success = false;
> 
> 		dev->transfer_done = true;
> 		wake_up_interruptible(&dev->wq);
> 	}
> 
> 	return IRQ_HANDLED;
> }
> 
> static int mxci2c_suspend(struct platform_device *pdev, pm_message_t state)
> {
> 	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> 	unsigned int sr;
> 
> 	if (mxcdev == NULL)
> 		return -ENODEV;
> 
> 	/* Prevent further calls to be processed */
> 	mxcdev->low_power = true;
> 	/* Wait till we finish the current transfer */
> 	sr = readw(mxcdev->membase + MXC_I2SR);
> 	while (sr & MXC_I2SR_IBB) {
> 		msleep(10);
> 		sr = readw(mxcdev->membase + MXC_I2SR);
> 	}
> 
> 	if (i2c_plat_data->exit)
> 		i2c_plat_data->exit(&pdev->dev);
> 
> 	return 0;
> }
> 
> static int mxci2c_resume(struct platform_device *pdev)
> {
> 	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> 
> 	if (mxcdev == NULL)
> 		return -ENODEV;
> 
> 	mxcdev->low_power = false;
> 	if (i2c_plat_data->init)
> 		i2c_plat_data->init(&pdev->dev);
> 
> 	return 0;
> }
> 
> static int __init mxci2c_probe(struct platform_device *pdev)
> {
> 	struct mxc_i2c_device *mxc_i2c;
> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> 	struct resource *res;
> 	int id = pdev->id;
> 	u32 clk_freq;
> 	int ret;
> 	int i;
> 
> 	mxc_i2c = kzalloc(sizeof(struct mxc_i2c_device), GFP_KERNEL);
> 	if (!mxc_i2c)
> 		return -ENOMEM;
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 	if (res == NULL) {
> 		ret = -ENODEV;
> 		goto egetres;
> 	}
> 
> 	if (!request_mem_region(res->start, resource_size(res), res->name)) {
> 		ret = -ENOMEM;
> 		goto ereqmemr;
> 	}
> 
> 	mxc_i2c->membase = ioremap(res->start, resource_size(res));
> 	if (!mxc_i2c->membase) {
> 		ret = -ENOMEM;
> 		goto eiomap;
> 	}
> 
> 	mxc_i2c->res = res;
> 
> 	/*
> 	 * Request the I2C interrupt
> 	 */
> 	mxc_i2c->irq = platform_get_irq(pdev, 0);
> 	if (mxc_i2c->irq < 0) {
> 		ret = mxc_i2c->irq;
> 		goto epgirq;
> 	}
> 
> 	ret = request_irq(mxc_i2c->irq, mxc_i2c_handler,
> 			  0, pdev->name, mxc_i2c);
> 	if (ret < 0)
> 		goto ereqirq;
> 
> 	init_waitqueue_head(&mxc_i2c->wq);
> 
> 	mxc_i2c->low_power	= false;
> 	mxc_i2c->transfer_done	= false;
> 	mxc_i2c->tx_success	= false;
> 
> 	if (i2c_plat_data->init)
> 		i2c_plat_data->init(&pdev->dev);
> 
> 	mxc_i2c->clk = clk_get(&pdev->dev, "i2c_clk");
> 	if (IS_ERR(mxc_i2c->clk)) {
> 		ret = PTR_ERR(mxc_i2c->clk);
> 		dev_err(&pdev->dev, "can't get I2C clock\n");
> 		goto eclkget;
> 	}
> 	clk_freq = clk_get_rate(mxc_i2c->clk);
> 
> 	mxc_i2c->clkdiv = -1;
> 	if (i2c_plat_data->bitrate) {
> 		/* Calculate divider and round up any fractional part */
> 		int div = (clk_freq + i2c_plat_data->bitrate - 1) /
> 			i2c_plat_data->bitrate;
> 		for (i = 0; i2c_clk_table[i].div != 0; i++) {

You could use ARRAY_SIZE here and remove the last {0, 0} entry from
i2c_clk_table.

> 			if (i2c_clk_table[i].div >= div) {
> 				mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
> 				break;
> 			}
> 		}
> 	}
> 	if (mxc_i2c->clkdiv == -1) {
> 		i = ARRAY_SIZE(i2c_clk_table) - 2;

Then we would not have this strange -2 here.

> 		/* Use max divider */
> 		mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
> 	}
> 	dev_dbg(&pdev->dev, "i2c speed is %d/%d = %d bps, reg val = 0x%02X\n",
> 		clk_freq, i2c_clk_table[i].div, clk_freq / i2c_clk_table[i].div,
> 		mxc_i2c->clkdiv);
> 
> 	/*
> 	 * Set the adapter information
> 	 */
> 	strcpy(mxc_i2c->adap.name, MXC_ADAPTER_NAME);
> 	mxc_i2c->adap.nr	= id;

id is used only here. pdev->id instead?

> 	mxc_i2c->adap.algo	= &mxc_i2c_algorithm;
> 	mxc_i2c->adap.timeout	= 1;
> 	mxc_i2c->adap.dev.parent= &pdev->dev;
> 	mxc_i2c->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
> 	platform_set_drvdata(pdev, mxc_i2c);
> 	i2c_set_adapdata(&mxc_i2c->adap, mxc_i2c);
> 	if ((ret = i2c_add_numbered_adapter(&mxc_i2c->adap)) < 0)
> 		goto eaddadap;
> 
> 	return 0;
> 
> eaddadap:
> 	platform_set_drvdata(pdev, NULL);
> 	clk_put(mxc_i2c->clk);
> eclkget:
> 	if (i2c_plat_data->exit)
> 		i2c_plat_data->exit(&pdev->dev);
> 	free_irq(mxc_i2c->irq, mxc_i2c);
> ereqirq:
> epgirq:
> 	iounmap(mxc_i2c->membase);
> eiomap:
> 	release_mem_region(res->start, resource_size(res));
> ereqmemr:
> egetres:
> 	kfree(mxc_i2c);
> 	dev_err(&pdev->dev, "failed to probe i2c adapter\n");
> 	return ret;
> }
> 
> static int __exit mxci2c_remove(struct platform_device *pdev)
> {
> 	struct mxc_i2c_device *mxc_i2c = platform_get_drvdata(pdev);
> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
> 
> 	free_irq(mxc_i2c->irq, mxc_i2c);
> 	i2c_del_adapter(&mxc_i2c->adap);
> 	if (i2c_plat_data->exit)
> 		i2c_plat_data->exit(&pdev->dev);
> 	clk_put(mxc_i2c->clk);
> 	platform_set_drvdata(pdev, NULL);
> 	iounmap(mxc_i2c->membase);
> 	release_mem_region(mxc_i2c->res->start, resource_size(mxc_i2c->res));
> 	kfree(mxc_i2c);
> 	return 0;
> }
> 
> static struct platform_driver mxci2c_driver = {
> 	.driver = {
> 		   .name = "imx-i2c",
> 		   .owner = THIS_MODULE,
> 	},
> 	.remove = __exit_p(mxci2c_remove),
> 	.suspend = mxci2c_suspend,
> 	.resume = mxci2c_resume,
> };
> 
> static int __init mxc_i2c_init(void)
> {
> 	return platform_driver_probe(&mxci2c_driver, mxci2c_probe);
> }
> 
> static void __exit mxc_i2c_exit(void)
> {
> 	platform_driver_unregister(&mxci2c_driver);
> }
> 
> subsys_initcall(mxc_i2c_init);
> module_exit(mxc_i2c_exit);
> 
> MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> MODULE_DESCRIPTION("MXC I2C driver");
> MODULE_LICENSE("GPL");


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Ben Dooks wrote:
> On Sat, Mar 28, 2009 at 10:36:50PM +0100, Guennadi Liakhovetski wrote:
>   
>> On Wed, 25 Mar 2009, Wolfram Sang wrote:
>>
>>     
>>>> Because I hurry to submit this, because merge window is open.
>>>>         
>> Ok, I waited for this driver, I really did, and I really prefer having one 
>> driver for as many hardware (SoC) variants as possible instead of having 
>> different drivers, and I really hoped its new version will work 
>> out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
>> it didn't. I've spent more than a full working day trying to get it to 
>> work, but I didn't succeed so far. It works with the on-board rtc, but it 
>> doesn't work with the camera, connected over a flex cable.
>>
>> Attached to this email is a version of the i2c driver for i.mx31 that I've 
>> been using with my setup for about half a year now. I adjusted it slightly 
>> to accept the same platform data, so, it is really a drop-in replacement 
>> for i2c-imx.c. Of course, you have to adjust or extend the Makefile. I 
>> actually added a new Kconfig entry for this driver, so I could easily 
>> compare them during the tests.
>>
>> The source of "my" version does look more logical and more clean to me on 
>> quite a few occasions. So, maybe someone could give it a quick spin on 
>> other *mx* SoCs and see if we can use this one instead? You might have to 
>> replace {read,write}w with readb and writeb, so, it might be a good idea 
>> to abstract them into inlines or macros. Otherwise, I think, it might work 
>> for other CPUs straight away.
>>
>> I just would like to avoid committing a driver and having to spend hours 
>> or days fixing it afterwards when a possibly more mature version exists.
I reply to Ben mail, because Guennadi sent code as attachment.
My driver has better debug possibilities and most of below comments fixed.
I suggest to find why do you get problems whit it on MX3 and fix it in 
my driver.

>> Thanks
>> Guennadi
>> ---
>> Guennadi Liakhovetski, Ph.D.
>> Freelance Open-Source Software Developer
>> /*
>>  * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
>>  *
>>  * The code contained herein is licensed under the GNU General Public
>>  * License. You may obtain a copy of the GNU General Public License
>>  * Version 2 or later at the following locations:
>>  *
>>  * http://www.opensource.org/licenses/gpl-license.html
>>  * http://www.gnu.org/copyleft/gpl.html
>>  */
>>
>> #include <linux/module.h>
>> #include <linux/kernel.h>
>> #include <linux/delay.h>
>> #include <linux/interrupt.h>
>> #include <linux/platform_device.h>
>> #include <linux/i2c.h>
>> #include <linux/clk.h>
>> #include <linux/err.h>
>>
>> #include <asm/irq.h>
>> #include <asm/io.h>
>>
>> #include <mach/i2c.h>
>> #include <mach/iomux-mx3.h>
>>
>> /* Address offsets of the I2C registers */
>> #define MXC_IADR                0x00	/* Address Register */
>> #define MXC_IFDR                0x04	/* Freq div register */
>> #define MXC_I2CR                0x08	/* Control regsiter */
>> #define MXC_I2SR                0x0C	/* Status register */
>> #define MXC_I2DR                0x10	/* Data I/O register */
>>
>> /* Bit definitions of I2CR */
>> #define MXC_I2CR_IEN            0x0080
>> #define MXC_I2CR_IIEN           0x0040
>> #define MXC_I2CR_MSTA           0x0020
>> #define MXC_I2CR_MTX            0x0010
>> #define MXC_I2CR_TXAK           0x0008
>> #define MXC_I2CR_RSTA           0x0004
>>
>> /* Bit definitions of I2SR */
>> #define MXC_I2SR_ICF            0x0080
>> #define MXC_I2SR_IAAS           0x0040
>> #define MXC_I2SR_IBB            0x0020
>> #define MXC_I2SR_IAL            0x0010
>> #define MXC_I2SR_SRW            0x0004
>> #define MXC_I2SR_IIF            0x0002
>> #define MXC_I2SR_RXAK           0x0001
>>
>> #define MXC_ADAPTER_NAME        "MXC I2C Adapter"
>>
>> /*
>>  * In case the MXC device has multiple I2C modules, this structure is used to
>>  * store information specific to each I2C module.
>>  */
>> struct mxc_i2c_device {
>> 	struct i2c_adapter adap;
>> 	wait_queue_head_t wq;		/* Transfer completion wait queue */
>> 	void __iomem *membase;
>> 	int irq;
>> 	unsigned int clkdiv;		/* Default clock divider */
>> 	struct clk *clk;
>> 	bool low_power;			/* Current power state */
>> 	bool transfer_done;
>> 	bool tx_success;		/* ACK received */
>> 	struct resource *res;
>> };
>>
>> struct clk_div_table {
>> 	int reg_value;
>> 	int div;
>> };
>>
>> static const struct clk_div_table i2c_clk_table[] = {
>> 	{0x20, 22}, {0x21, 24}, {0x22, 26}, {0x23, 28},
>> 	{0, 30}, {1, 32}, {0x24, 32}, {2, 36},
>> 	{0x25, 36}, {0x26, 40}, {3, 42}, {0x27, 44},
>> 	{4, 48}, {0x28, 48}, {5, 52}, {0x29, 56},
>> 	{6, 60}, {0x2A, 64}, {7, 72}, {0x2B, 72},
>> 	{8, 80}, {0x2C, 80}, {9, 88}, {0x2D, 96},
>> 	{0xA, 104}, {0x2E, 112}, {0xB, 128}, {0x2F, 128},
>> 	{0xC, 144}, {0xD, 160}, {0x30, 160}, {0xE, 192},
>> 	{0x31, 192}, {0x32, 224}, {0xF, 240}, {0x33, 256},
>> 	{0x10, 288}, {0x11, 320}, {0x34, 320}, {0x12, 384},
>> 	{0x35, 384}, {0x36, 448}, {0x13, 480}, {0x37, 512},
>> 	{0x14, 576}, {0x15, 640}, {0x38, 640}, {0x16, 768},
>> 	{0x39, 768}, {0x3A, 896}, {0x17, 960}, {0x3B, 1024},
>> 	{0x18, 1152}, {0x19, 1280}, {0x3C, 1280}, {0x1A, 1536},
>> 	{0x3D, 1536}, {0x3E, 1792}, {0x1B, 1920}, {0x3F, 2048},
>> 	{0x1C, 2304}, {0x1D, 2560}, {0x1E, 3072}, {0x1F, 3840},
>> 	{0, 0}
>> };
>>     
there you have lot of duplicated values. My driver has more clearly array.
>> /**
>>  * Transmit a \b STOP signal to the slave device.
>>  *
>>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>>  */
>> static void mxc_i2c_stop(struct mxc_i2c_device * dev)
>> {
>> 	unsigned int cr;
>> 	int retry = 200;
>>     
what means this magic number?


>> 	cr = readw(dev->membase + MXC_I2CR);
>> 	cr &= ~(MXC_I2CR_MSTA | MXC_I2CR_MTX);
>> 	writew(cr, dev->membase + MXC_I2CR);
>>     

no reason to use writew/readw, because all bits in I2C registers are 
lower 8 bits.

>> 	/*
>> 	 * Make sure STOP meets setup requirement.
>> 	 */
>> 	for (;;) {
>> 		unsigned int sr = readw(dev->membase + MXC_I2SR);
>> 		if ((sr & MXC_I2SR_IBB) == 0)
>>     

you test if I2C is busy in many loacations. It is worth to have separate 
function like "i2c_is_busy".


>> 			break;
>> 		if (retry-- <= 0) {
>> 			printk(KERN_DEBUG "Bus busy\n");
>> 			break;
>> 		}
>> 		udelay(3);
>>     

I got comments form ML few timer not to use udelay() and let other 
processes to run.
My driver does this more efficient.

>> 	}
>> }
>>
>> /**
>>  * Wait for the transmission of the data byte to complete. This function waits
>>  * till we get a signal from the interrupt service routine indicating completion
>>  * of the address cycle or we time out.
>>  *
>>  * @param   dev         the mxc i2c structure used to get to the right i2c device
>>  * @param   trans_flag  transfer flag
>>  *
>>  *
>>  * @return  The function returns 0 on success or -1 if an ack was not received
>>  */
>>
>> static int mxc_i2c_wait_for_tc(struct mxc_i2c_device *dev, int trans_flag)
>> {
>> 	int retry = 16;
>>     

again strange constant.

>> 	while (retry-- && !dev->transfer_done)
>> 		wait_event_interruptible_timeout(dev->wq,
>> 						 dev->transfer_done,
>> 						 dev->adap.timeout);
>>
>> 	dev->transfer_done = false;
>>
>> 	if (retry <= 0) {
>> 		/* Unable to send data */
>> 		dev_warn(&dev->adap.dev, "Data not transmitted\n");
>> 		return -ETIMEDOUT;
>> 	}
>>
>> 	if (!dev->tx_success) {
>> 		/* An ACK was not received for transmitted byte */
>> 		dev_dbg(&dev->adap.dev, "ACK not received \n");
>> 		return -EREMOTEIO;
>> 	}
>>
>> 	return 0;
>> }
>>
>> /**
>>  * Transmit a \b START signal to the slave device.
>>  *
>>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>>  * @param   *msg  pointer to a message structure that contains the slave
>>  *                address
>>  */
>> static void mxc_i2c_start(struct mxc_i2c_device *dev, struct i2c_msg *msg)
>> {
>> 	unsigned int cr, sr;
>> 	unsigned int addr_trans;
>> 	int retry = 16;
>>
>> 	/*
>> 	 * Set the slave address and the requested transfer mode
>> 	 * in the data register
>> 	 */
>> 	addr_trans = msg->addr << 1;
>> 	if (msg->flags & I2C_M_RD)
>> 		addr_trans |= 0x01;
>>
>> 	dev_dbg(&dev->adap.dev, "%s: start %x\n", __func__, msg->addr);
>>
>> 	/* Set the Master bit */
>> 	cr = readw(dev->membase + MXC_I2CR);
>> 	cr |= MXC_I2CR_MSTA;
>> 	writew(cr, dev->membase + MXC_I2CR);
>>
>> 	/* Wait till the Bus Busy bit is set */
>> 	sr = readw(dev->membase + MXC_I2SR);
>> 	while (retry-- && (!(sr & MXC_I2SR_IBB))) {
>> 		udelay(3);
>> 		sr = readw(dev->membase + MXC_I2SR);
>> 	}
>> 	if (retry <= 0)
>> 		dev_warn(&dev->adap.dev, "Could not grab Bus ownership\n");
>>
>> 	/* Set the Transmit bit */
>> 	cr = readw(dev->membase + MXC_I2CR);
>> 	cr |= MXC_I2CR_MTX;
>> 	writew(cr, dev->membase + MXC_I2CR);
>>
>> 	writew(addr_trans, dev->membase + MXC_I2DR);
>> }
>>
>> /**
>>  * Transmit a \b REPEAT START to the slave device
>>  *
>>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>>  * @param   *msg  pointer to a message structure that contains the slave
>>  *                address
>>  */
>> static void mxc_i2c_repstart(struct mxc_i2c_device *dev, struct i2c_msg *msg)
>> {
>> 	unsigned int cr;
>> 	unsigned int addr_trans;
>>
>> 	/*
>> 	 * Set the slave address and the requested transfer mode
>> 	 * in the data register
>> 	 */
>> 	addr_trans = msg->addr << 1;
>> 	if (msg->flags & I2C_M_RD)
>> 		addr_trans |= 0x01;
>>
>> 	dev_dbg(&dev->adap.dev, "%s: repeat start %x\n", __func__, msg->addr);
>>
>> 	cr = readw(dev->membase + MXC_I2CR);
>> 	cr |= MXC_I2CR_RSTA;
>> 	writew(cr, dev->membase + MXC_I2CR);
>> 	udelay(3);
>> 	writew(addr_trans, dev->membase + MXC_I2DR);
>> }
>>
>> /**
>>  * Read the received data. The function waits till data is available or times
>>  * out. Generates a stop signal if this is the last message to be received.
>>  * Sends an ack for all the bytes received except the last byte.
>>  *
>>  * @param  dev       the mxc i2c structure used to get to the right i2c device
>>  * @param  *msg      pointer to a message structure that contains the slave
>>  *                   address and a pointer to the receive buffer
>>  * @param  last      indicates that this is the last message to be received
>>  * @param  addr_comp flag indicates that we just finished the address cycle
>>  *
>>  * @return  The function returns the number of bytes read or -1 on time out.
>>  */
>> static int mxc_i2c_readbytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
>> 			     int last, int addr_comp)
>> {
>> 	int i;
>> 	char *buf = msg->buf;
>> 	int len = msg->len;
>> 	unsigned int cr;
>>
>> 	dev_dbg(&dev->adap.dev, "%s: last %d, addr_comp %d\n",
>> 		__func__, last, addr_comp);
>>
>> 	cr = readw(dev->membase + MXC_I2CR);
>> 	/*
>> 	 * Clear MTX to switch to receive mode.
>> 	 */
>> 	cr &= ~MXC_I2CR_MTX;
>> 	/*
>> 	 * Clear the TXAK bit to gen an ack when receiving only one byte.
>> 	 */
>> 	if (len == 1)
>> 		cr |= MXC_I2CR_TXAK;
>> 	else
>> 		cr &= ~MXC_I2CR_TXAK;
>>
>> 	writew(cr, dev->membase + MXC_I2CR);
>> 	/*
>> 	 * Dummy read only at the end of an address cycle
>> 	 */
>> 	if (addr_comp > 0)
>> 		readw(dev->membase + MXC_I2DR);
>>
>> 	for (i = 0; i < len; i++) {
>> 		int ret;
>> 		/*
>> 		 * Wait for data transmission to complete
>> 		 */
>> 		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
>> 		if (ret < 0) {
>> 			dev_err(&dev->adap.dev, "%s: rx #%d of %d failed!\n",
>> 				__func__, i, len);
>> 			mxc_i2c_stop(dev);
>> 			return ret;
>> 		}
>> 		/*
>> 		 * Do not generate an ACK for the last byte
>> 		 */
>> 		if (i == len - 2) {
>> 			cr = readw(dev->membase + MXC_I2CR);
>> 			cr |= MXC_I2CR_TXAK;
>> 			writew(cr, dev->membase + MXC_I2CR);
>> 		} else if (i == len - 1) {
>> 			if (last)
>> 				mxc_i2c_stop(dev);
>> 		}
>> 		/* Read the data */
>> 		*buf++ = readw(dev->membase + MXC_I2DR);
>> 	}
>>
>> 	return i;
>> }
>>
>> /**
>>  * Write the data to the data register. Generates a stop signal if this is
>>  * the last message to be sent or if no ack was received for the data sent.
>>  *
>>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>>  * @param   *msg  pointer to a message structure that contains the slave
>>  *                address and data to be sent
>>  * @param   last  indicates that this is the last message to be received
>>  *
>>  * @return  The function returns the number of bytes written or -1 on time out
>>  *          or if no ack was received for the data that was sent.
>>  */
>> static int mxc_i2c_writebytes(struct mxc_i2c_device *dev, struct i2c_msg *msg,
>> 			      int last)
>> {
>> 	int i;
>> 	char *buf = msg->buf;
>> 	int len = msg->len;
>> 	unsigned int cr;
>>
>> 	dev_dbg(&dev->adap.dev, "%s: last %d\n", __func__, last);
>>
>> 	cr = readw(dev->membase + MXC_I2CR);
>> 	/* Set MTX to switch to transmit mode */
>> 	writew(cr | MXC_I2CR_MTX, dev->membase + MXC_I2CR);
>>
>> 	for (i = 0; i < len; i++) {
>> 		int ret;
>> 		/*
>> 		 * Write the data
>> 		 */
>> 		writew(*buf++, dev->membase + MXC_I2DR);
>> 		ret = mxc_i2c_wait_for_tc(dev, msg->flags);
>> 		if (ret < 0) {
>> 			dev_err(&dev->adap.dev, "%s: tx #%d of %d timed out!\n",
>> 				__func__, i, len);
>> 			mxc_i2c_stop(dev);
>> 			return ret;
>> 		}
>> 	}
>> 	if (last > 0)
>> 		mxc_i2c_stop(dev);
>>
>> 	return i;
>> }
>>
>> /**
>>  * Function enables the I2C module and initializes the registers.
>>  *
>>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>>  * @param   trans_flag  transfer flag
>>  */
>> static void mxc_i2c_module_en(struct mxc_i2c_device *dev, int trans_flag)
>> {
>> 	clk_enable(dev->clk);
>> 	/* Set the frequency divider */
>> 	writew(dev->clkdiv, dev->membase + MXC_IFDR);
>> 	/* Clear the status register */
>> 	writew(0x0, dev->membase + MXC_I2SR);
>> 	/* Enable I2C and its interrupts */
>> 	writew(MXC_I2CR_IEN, dev->membase + MXC_I2CR);
>> 	writew(MXC_I2CR_IEN | MXC_I2CR_IIEN, dev->membase + MXC_I2CR);
>> }
>>
>> /**
>>  * Disables the I2C module.
>>  *
>>  * @param   dev   the mxc i2c structure used to get to the right i2c device
>>  */
>> static void mxc_i2c_module_dis(struct mxc_i2c_device * dev)
>> {
>> 	writew(0x0, dev->membase + MXC_I2CR);
>> 	clk_disable(dev->clk);
>> }
>>
>> /**
>>  * The function is registered in the adapter structure. It is called when an MXC
>>  * driver wishes to transfer data to a device connected to the I2C device.
>>  *
>>  * @param   adap   adapter structure for the MXC i2c device
>>  * @param   msgs[] array of messages to be transferred to the device
>>  * @param   num    number of messages to be transferred to the device
>>  *
>>  * @return  The function returns the number of messages transferred,
>>  *          \b -EREMOTEIO on I2C failure and a 0 if the num argument is
>>  *          less than 0.
>>  */
>> static int mxc_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>> {
>> 	struct mxc_i2c_device *dev = i2c_get_adapdata(adap);
>> 	int i, ret = 0, addr_comp = 0;
>> 	unsigned int sr;
>>
>> 	if (dev->low_power) {
>> 		dev_warn(&dev->adap.dev, "I2C Device in low power mode\n");
>> 		return -EREMOTEIO;
>> 	}
>>
>> 	if (num < 1)
>> 		return 0;
>>
>> 	dev_dbg(&dev->adap.dev, "%s: xfer %d msgs\n", __func__, num);
>>
>> 	mxc_i2c_module_en(dev, msgs[0].flags);
>> 	sr = readw(dev->membase + MXC_I2SR);
>> 	/*
>> 	 * Check bus state
>> 	 */
>> 	if (sr & MXC_I2SR_IBB) {
>> 		mxc_i2c_module_dis(dev);
>> 		printk(KERN_DEBUG "Bus busy\n");
>> 		return -EREMOTEIO;
>> 	}
>>
>> 	dev->transfer_done = false;
>> 	dev->tx_success = false;
>> 	for (i = 0; i < num && ret >= 0; i++) {
>> 		addr_comp = 0;
>> 		/*
>> 		 * Send the slave address and transfer direction in the
>> 		 * address cycle
>> 		 */
>> 		if (i == 0) {
>>     

this long if () else () statement could be replaced with very simple, 
like this one from my driver:

if (i) {
            dev_dbg(&i2c_imx->adapter.dev, "<%s> repeated start\n", 
__func__);
            temp = readb(i2c_imx->base + IMX_I2C_I2CR);
            temp |= I2CR_RSTA;
            writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
}

>> 			/*
>> 			 * Send a start or repeat start signal
>> 			 */
>> 			mxc_i2c_start(dev, &msgs[0]);
>>     

you don't send there repeat start signal (wrong comment).

>> 			/* Wait for the address cycle to complete */
>> 			if (mxc_i2c_wait_for_tc(dev, msgs[0].flags)) {
>> 				mxc_i2c_stop(dev);
>> 				mxc_i2c_module_dis(dev);
>> 				dev_err(&dev->adap.dev,
>> 					"%s: Address failed!\n", __func__);
>> 				return -EREMOTEIO;
>> 			}
>> 			addr_comp = 1;
>> 		} else {
>> 			/*
>> 			 * Generate repeat start only if required i.e. the
>> 			 * address changed or the transfer direction changed
>> 			 */
>> 			if ((msgs[i].addr != msgs[i - 1].addr) ||
>> 			    ((msgs[i].flags & I2C_M_RD) !=
>> 			     (msgs[i - 1].flags & I2C_M_RD))) {
>> 				mxc_i2c_repstart(dev, &msgs[i]);
>> 				/* Wait for the address cycle to complete */
>> 				if (mxc_i2c_wait_for_tc(dev, msgs[i].flags)) {
>> 					mxc_i2c_stop(dev);
>> 					mxc_i2c_module_dis(dev);
>> 					dev_err(&dev->adap.dev,
>> 						"%s: Address repeat failed!\n",
>> 						__func__);
>> 					return -EREMOTEIO;
>> 				}
>> 				addr_comp = 1;
>> 			}
>> 		}
>>
>> 		/* Transfer the data */
>> 		if (msgs[i].flags & I2C_M_RD) {
>> 			/* Read the data */
>> 			ret = mxc_i2c_readbytes(dev, &msgs[i], i + 1 == num,
>> 						addr_comp);
>> 			if (ret < 0) {
>> 				dev_err(&dev->adap.dev, "mxc_i2c_readbytes: fail.\n");
>> 				break;
>> 			}
>> 		} else {
>> 			/* Write the data */
>> 			ret = mxc_i2c_writebytes(dev, &msgs[i], i + 1 == num);
>> 			if (ret < 0) {
>> 				dev_err(&dev->adap.dev, "mxc_i2c_writebytes: fail.\n");
>> 				break;
>> 			}
>> 		}
>> 	}
>>     
My driver has a delay before disabling I2C module. I added this, because 
no STOP signal is generated otherwise.
Your driver works on i.MXL without this delay. So there could be one of 
possible bugs in my driver.

>> 	mxc_i2c_module_dis(dev);
>>
>> 	dev_dbg(&dev->adap.dev, "%s: %d msgs success\n", __func__, i);
>>
>> 	/*
>> 	 * Decrease by 1 as we do not want Start message to be included in
>> 	 * the count
>> 	 */
>> 	return i;
>> }
>>
>> /**
>>  * Returns the i2c functionality supported by this driver.
>>  *
>>  * @param   adap adapter structure for this i2c device
>>  *
>>  * @return Returns the functionality that is supported.
>>  */
>> static u32 mxc_i2c_func(struct i2c_adapter *adap)
>> {
>> 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> }
>>
>> static struct i2c_algorithm mxc_i2c_algorithm = {
>> 	.master_xfer = mxc_i2c_xfer,
>> 	.functionality = mxc_i2c_func
>> };
>>
>> /*
>>  * Interrupt Service Routine. It signals to the process about the data transfer
>>  * completion. Also sets a flag if bus arbitration is lost.
>>  */
>> static irqreturn_t mxc_i2c_handler(int irq, void *dev_id)
>> {
>> 	struct mxc_i2c_device *dev = dev_id;
>> 	unsigned int sr, cr;
>>
>> 	sr = readw(dev->membase + MXC_I2SR);
>> 	cr = readw(dev->membase + MXC_I2CR);
>>
>> 	/*
>> 	 * Clear the interrupt bit
>> 	 */
>> 	writew(0x0, dev->membase + MXC_I2SR);
>>     

there you clear all status register, not IFF bit.

>> 	if (sr & MXC_I2SR_IAL) {
>> 		printk(KERN_DEBUG "Bus Arbitration lost\n");
>> 	} else {
>> 		/* Interrupt due byte transfer completion */
>>     

probably you should check if IFF bit is set to return IRQ_HANDLED.

>> 		dev->tx_success = true;
>> 		/* Check if RXAK is received in Transmit mode */
>> 		if ((cr & MXC_I2CR_MTX) && (sr & MXC_I2SR_RXAK))
>> 			dev->tx_success = false;
>>
>> 		dev->transfer_done = true;
>> 		wake_up_interruptible(&dev->wq);
>> 	}
>>
>> 	return IRQ_HANDLED;
>> }
>>
>> static int mxci2c_suspend(struct platform_device *pdev, pm_message_t state)
>> {
>> 	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
>> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
>> 	unsigned int sr;
>>
>> 	if (mxcdev == NULL)
>> 		return -ENODEV;
>>
>> 	/* Prevent further calls to be processed */
>> 	mxcdev->low_power = true;
>> 	/* Wait till we finish the current transfer */
>> 	sr = readw(mxcdev->membase + MXC_I2SR);
>> 	while (sr & MXC_I2SR_IBB) {
>> 		msleep(10);
>> 		sr = readw(mxcdev->membase + MXC_I2SR);
>> 	}
>>
>> 	if (i2c_plat_data->exit)
>> 		i2c_plat_data->exit(&pdev->dev);
>>
>> 	return 0;
>> }
>>
>> static int mxci2c_resume(struct platform_device *pdev)
>> {
>> 	struct mxc_i2c_device *mxcdev = platform_get_drvdata(pdev);
>> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
>>
>> 	if (mxcdev == NULL)
>> 		return -ENODEV;
>>
>> 	mxcdev->low_power = false;
>> 	if (i2c_plat_data->init)
>> 		i2c_plat_data->init(&pdev->dev);
>>
>> 	return 0;
>> }
>>
>> static int __init mxci2c_probe(struct platform_device *pdev)
>> {
>> 	struct mxc_i2c_device *mxc_i2c;
>> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
>> 	struct resource *res;
>> 	int id = pdev->id;
>> 	u32 clk_freq;
>> 	int ret;
>> 	int i;
>>
>> 	mxc_i2c = kzalloc(sizeof(struct mxc_i2c_device), GFP_KERNEL);
>> 	if (!mxc_i2c)
>> 		return -ENOMEM;
>>
>> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> 	if (res == NULL) {
>> 		ret = -ENODEV;
>> 		goto egetres;
>> 	}
>>
>> 	if (!request_mem_region(res->start, resource_size(res), res->name)) {
>> 		ret = -ENOMEM;
>> 		goto ereqmemr;
>> 	}
>>
>> 	mxc_i2c->membase = ioremap(res->start, resource_size(res));
>> 	if (!mxc_i2c->membase) {
>> 		ret = -ENOMEM;
>> 		goto eiomap;
>> 	}
>>
>> 	mxc_i2c->res = res;
>>
>> 	/*
>> 	 * Request the I2C interrupt
>> 	 */
>> 	mxc_i2c->irq = platform_get_irq(pdev, 0);
>> 	if (mxc_i2c->irq < 0) {
>> 		ret = mxc_i2c->irq;
>> 		goto epgirq;
>> 	}
>>
>> 	ret = request_irq(mxc_i2c->irq, mxc_i2c_handler,
>> 			  0, pdev->name, mxc_i2c);
>> 	if (ret < 0)
>> 		goto ereqirq;
>>
>> 	init_waitqueue_head(&mxc_i2c->wq);
>>
>> 	mxc_i2c->low_power	= false;
>> 	mxc_i2c->transfer_done	= false;
>> 	mxc_i2c->tx_success	= false;
>>
>> 	if (i2c_plat_data->init)
>> 		i2c_plat_data->init(&pdev->dev);
>>
>> 	mxc_i2c->clk = clk_get(&pdev->dev, "i2c_clk");
>> 	if (IS_ERR(mxc_i2c->clk)) {
>> 		ret = PTR_ERR(mxc_i2c->clk);
>> 		dev_err(&pdev->dev, "can't get I2C clock\n");
>> 		goto eclkget;
>> 	}
>> 	clk_freq = clk_get_rate(mxc_i2c->clk);
>>
>> 	mxc_i2c->clkdiv = -1;
>> 	if (i2c_plat_data->bitrate) {
>> 		/* Calculate divider and round up any fractional part */
>> 		int div = (clk_freq + i2c_plat_data->bitrate - 1) /
>> 			i2c_plat_data->bitrate;
>> 		for (i = 0; i2c_clk_table[i].div != 0; i++) {
>> 			if (i2c_clk_table[i].div >= div) {
>> 				mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
>> 				break;
>> 			}
>> 		}
>> 	}
>> 	if (mxc_i2c->clkdiv == -1) {
>> 		i = ARRAY_SIZE(i2c_clk_table) - 2;
>> 		/* Use max divider */
>> 		mxc_i2c->clkdiv = i2c_clk_table[i].reg_value;
>> 	}
>>     

my drivers does this in separate function in more efficient and readable 
way.

>> 	dev_dbg(&pdev->dev, "i2c speed is %d/%d = %d bps, reg val = 0x%02X\n",
>> 		clk_freq, i2c_clk_table[i].div, clk_freq / i2c_clk_table[i].div,
>> 		mxc_i2c->clkdiv);
>>
>> 	/*
>> 	 * Set the adapter information
>> 	 */
>> 	strcpy(mxc_i2c->adap.name, MXC_ADAPTER_NAME);
>> 	mxc_i2c->adap.nr	= id;
>> 	mxc_i2c->adap.algo	= &mxc_i2c_algorithm;
>> 	mxc_i2c->adap.timeout	= 1;
>> 	mxc_i2c->adap.dev.parent= &pdev->dev;
>> 	mxc_i2c->adap.class	= I2C_CLASS_HWMON | I2C_CLASS_SPD;
>>     

I think adap.class should be removed at all.

>> 	platform_set_drvdata(pdev, mxc_i2c);
>> 	i2c_set_adapdata(&mxc_i2c->adap, mxc_i2c);
>> 	if ((ret = i2c_add_numbered_adapter(&mxc_i2c->adap)) < 0)
>> 		goto eaddadap;
>>
>> 	return 0;
>>
>> eaddadap:
>> 	platform_set_drvdata(pdev, NULL);
>> 	clk_put(mxc_i2c->clk);
>> eclkget:
>> 	if (i2c_plat_data->exit)
>> 		i2c_plat_data->exit(&pdev->dev);
>> 	free_irq(mxc_i2c->irq, mxc_i2c);
>> ereqirq:
>> epgirq:
>> 	iounmap(mxc_i2c->membase);
>> eiomap:
>> 	release_mem_region(res->start, resource_size(res));
>> ereqmemr:
>> egetres:
>> 	kfree(mxc_i2c);
>> 	dev_err(&pdev->dev, "failed to probe i2c adapter\n");
>> 	return ret;
>> }
>>
>> static int __exit mxci2c_remove(struct platform_device *pdev)
>> {
>> 	struct mxc_i2c_device *mxc_i2c = platform_get_drvdata(pdev);
>> 	struct imxi2c_platform_data *i2c_plat_data = pdev->dev.platform_data;
>>
>> 	free_irq(mxc_i2c->irq, mxc_i2c);
>> 	i2c_del_adapter(&mxc_i2c->adap);
>> 	if (i2c_plat_data->exit)
>> 		i2c_plat_data->exit(&pdev->dev);
>> 	clk_put(mxc_i2c->clk);
>> 	platform_set_drvdata(pdev, NULL);
>> 	iounmap(mxc_i2c->membase);
>> 	release_mem_region(mxc_i2c->res->start, resource_size(mxc_i2c->res));
>> 	kfree(mxc_i2c);
>> 	return 0;
>> }
>>
>> static struct platform_driver mxci2c_driver = {
>> 	.driver = {
>> 		   .name = "imx-i2c",
>> 		   .owner = THIS_MODULE,
>> 	},
>> 	.remove = __exit_p(mxci2c_remove),
>> 	.suspend = mxci2c_suspend,
>> 	.resume = mxci2c_resume,
>> };
>>
>> static int __init mxc_i2c_init(void)
>> {
>> 	return platform_driver_probe(&mxci2c_driver, mxci2c_probe);
>> }
>>
>> static void __exit mxc_i2c_exit(void)
>> {
>> 	platform_driver_unregister(&mxci2c_driver);
>> }
>>
>> subsys_initcall(mxc_i2c_init);
>> module_exit(mxc_i2c_exit);
>>
>> MODULE_AUTHOR("Freescale Semiconductor, Inc.");
>> MODULE_DESCRIPTION("MXC I2C driver");
>> MODULE_LICENSE("GPL");
>>     
>
>
>   

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Guennadi Liakhovetski wrote:
> On Mon, 30 Mar 2009, Sascha Hauer wrote:
>
>   
>> On Mon, Mar 30, 2009 at 11:26:31AM +0200, Guennadi Liakhovetski wrote:
>>     
>>> Well, I have been able to get your driver to at least pass the 
>>> initialisation with mt9t031 (other parts are missing yet for a complete 
>>> test). For that I used this silly patch:
>>>
>>> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
>>> index 3296380..46e1033 100644
>>> --- a/drivers/i2c/busses/i2c-imx.c
>>> +++ b/drivers/i2c/busses/i2c-imx.c
>>> @@ -371,6 +371,8 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>>>  	if (result)
>>>  		goto fail0;
>>>  
>>> +	msleep(2);
>>> +
>>>  	/* Start I2C transfer */
>>>  	i2c_imx_start(i2c_imx);
>>>
>>> As you understand, this cannot be the final fix. We have to understand why 
>>> a delay is needed there and how long it actually has to be...
>>>  
>>>       
>> Have you checked the value of disable_delay in Darius' driver or tried to
>> increase it?
>>     
>
> Why? That variable is addreccing a problem on a completely different 
> hardware:
>
> 	/*
> 	 * There dummy delay is calculated.
> 	 * It should be about one I2C clock period long.
> 	 * This delay is used in I2C bus disable function
> 	 * to fix chip hardware bug.
> 	 */
>
> and is used on a different path - during controller disable.
>   

Freescale datasheet don't specifies any timing requirement when 
disabling I2C module.
If I disable it immediately after generating STOP signal, it's not 
generated.
When one clock delay is used, problem is solved. So I guess it is 
hardware bug.
Strange, why your driver works without that.

>   
>> BTW I just realized that the handling of disable_delay in Darius' driver
>> is wrong. This should be a per device variable.
>>     
>
> Indeed.
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
>
>   

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Guennadi Liakhovetski wrote:
> On Mon, 30 Mar 2009, Mark Brown wrote:
> 
>> On Sat, Mar 28, 2009 at 10:36:50PM +0100, Guennadi Liakhovetski wrote:
>>> different drivers, and I really hoped its new version will work 
>>> out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
>>> it didn't. I've spent more than a full working day trying to get it to 
>>> work, but I didn't succeed so far. It works with the on-board rtc, but it 
>>> doesn't work with the camera, connected over a flex cable.
>> What problems are you experiencing?  I've been using versions of Darius'
>> driver for a while now with no problems on my system.
> 
> I couldn't read camera ID register during probing. As I mentioned, the 
> camera is connected over a, hm, limited-quality self-soldered (by 
> professionals) cable, and I have to reduce bus clock to 20kHz. Has anyone 
> tested this driver with lower frequencies?

yes, I tested this with low frequencies (tens of kHz) and high freq. (400kHz).

> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Guennadi Liakhovetski wrote:
 > Hi Darius,
 >
 > On Mon, 30 Mar 2009, Darius Augulis wrote:
 >
 >> I tested your driver on i.MXL with OV7670 and MT9V111 cameras. I confirm it
 >> works.
 >
 > Thanks for testing!
 >
 >> In otherside, my driver is also tested with OV7670, MT9V111, OV7720 cameras,
 >> PCF8575 expander and NT7651 LCD controller. All these devices work connected
 >> to i.MXL.
 >> If you have problems on MX3 only with single device, it may be caused by
 >> some specific conditions.
 >> I don't know it is good idea to replace one driver by another, because we
 >> don't know where is the problem exactly.
 >> Of course would be fine to watch your I2C bus with oscilloscope. So you
 >> could see what is wrong.
 >> IMO we should find what is wrong in my driver, to have good working driver
 >> for all MXC SoC's.
 >> If we replace it by your driver, we miss lot of ML comments fixed and
 >> probably we will receive errors and problems in some different situation.
 >
 > Well, I have been able to get your driver to at least pass the
 > initialisation with mt9t031 (other parts are missing yet for a complete
 > test). For that I used this silly patch:
 >
 > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
 > index 3296380..46e1033 100644
 > --- a/drivers/i2c/busses/i2c-imx.c
 > +++ b/drivers/i2c/busses/i2c-imx.c
 > @@ -371,6 +371,8 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 >  	if (result)
 >  		goto fail0;
 >
 > +	msleep(2);
 > +
 >  	/* Start I2C transfer */
 >  	i2c_imx_start(i2c_imx);
 >
 > As you understand, this cannot be the final fix. We have to understand why
 > a delay is needed there and how long it actually has to be...

It's strange. You delay the beginning of I2C transfer.
It is only one thing, which makes my driver working in your application?


 >
 > Thanks
 > Guennadi
 > ---
 > Guennadi Liakhovetski, Ph.D.
 > Freelance Open-Source Software Developer

Re: [PATCH 1/5] I2C driver for MXC

[Guennadi Liakhovetski]

Hi Sascha

On Mon, 30 Mar 2009, Sascha Hauer wrote:

> Hi Guennadi,
> 
> On Sat, Mar 28, 2009 at 10:36:50PM +0100, Guennadi Liakhovetski wrote:
> > On Wed, 25 Mar 2009, Wolfram Sang wrote:
> > 
> > > > Because I hurry to submit this, because merge window is open.
> > 
> > Ok, I waited for this driver, I really did, and I really prefer having one 
> > driver for as many hardware (SoC) variants as possible instead of having 
> > different drivers, and I really hoped its new version will work 
> > out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
> > it didn't. I've spent more than a full working day trying to get it to 
> > work, but I didn't succeed so far. It works with the on-board rtc, but it 
> > doesn't work with the camera, connected over a flex cable.
> > 
> > Attached to this email is a version of the i2c driver for i.mx31 that I've 
> > been using with my setup for about half a year now.
> 
> Then why didn't you send this driver half a year ago??

I did, and you were on cc and even reviewed it:

http://marc.info/?t=122608095600001&r=1&w=2

but then came a reply from Darius, saying he had a driver for i.MX1 ready. 
I then tested his version and pointed out at problems I had on i.MX31, but 
they were not resolved until now...

> > I adjusted it slightly 
> > to accept the same platform data, so, it is really a drop-in replacement 
> > for i2c-imx.c. Of course, you have to adjust or extend the Makefile. I 
> > actually added a new Kconfig entry for this driver, so I could easily 
> > compare them during the tests.
> > 
> > The source of "my" version does look more logical and more clean to me on 
> > quite a few occasions. So, maybe someone could give it a quick spin on 
> > other *mx* SoCs and see if we can use this one instead? You might have to 
> > replace {read,write}w with readb and writeb, so, it might be a good idea 
> > to abstract them into inlines or macros. Otherwise, I think, it might work 
> > for other CPUs straight away.
> > 
> > I just would like to avoid committing a driver and having to spend hours 
> > or days fixing it afterwards when a possibly more mature version exists.
> 
> I just had a quick look over this driver, some comments inline. I think
> your driver has some pros, but given the fact that we are in the middle
> of the merge window and this driver is not ready for direct inclusion
> I'd say that you are simply too late.

The comments that you provided are all correct and easily fixable. Just 
one correction - this is not "my" driver, I am not personally attached to 
it:-) I just want a _good_ _solid_ well-written driver, that's it.

But the main advantage that I see in "my" version of Freescale's driver is 
its internal design. E.g., look at the i2c_imx_xfer() resp. mxc_i2c_xfer() 
functions at how they start a transfer. "my" version does the following:

enable the module
check bus busy, if busy - disable again and abort (multiple masters?)
set the master bit and - _wait_ until the bus becomes busy
then set the transmit bit

the imx version does

wait for not busy - _before_ enabling the controller!
enable the controller and set master and transmit _and_ the TXAK (!) 
	bits immediately...

Well, as I said, I am not personally attached to any of these drivers, so, 
it's up to maintainers to decide how to procede. I'll reply to Darius' 
previous mail shortly.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 1/5] I2C driver for MXC

[Mark Brown]

On Sat, Mar 28, 2009 at 10:36:50PM +0100, Guennadi Liakhovetski wrote:
> 
> different drivers, and I really hoped its new version will work 
> out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
> it didn't. I've spent more than a full working day trying to get it to 
> work, but I didn't succeed so far. It works with the on-board rtc, but it 
> doesn't work with the camera, connected over a flex cable.

What problems are you experiencing?  I've been using versions of Darius'
driver for a while now with no problems on my system.

Re: [PATCH 1/5] I2C driver for MXC

[Guennadi Liakhovetski]

Hi Darius,

On Mon, 30 Mar 2009, Darius Augulis wrote:

> I tested your driver on i.MXL with OV7670 and MT9V111 cameras. I confirm it
> works.

Thanks for testing!

> In otherside, my driver is also tested with OV7670, MT9V111, OV7720 cameras,
> PCF8575 expander and NT7651 LCD controller. All these devices work connected
> to i.MXL.
> If you have problems on MX3 only with single device, it may be caused by
> some specific conditions.
> I don't know it is good idea to replace one driver by another, because we
> don't know where is the problem exactly.
> Of course would be fine to watch your I2C bus with oscilloscope. So you
> could see what is wrong.
> IMO we should find what is wrong in my driver, to have good working driver
> for all MXC SoC's.
> If we replace it by your driver, we miss lot of ML comments fixed and
> probably we will receive errors and problems in some different situation.

Well, I have been able to get your driver to at least pass the 
initialisation with mt9t031 (other parts are missing yet for a complete 
test). For that I used this silly patch:

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 3296380..46e1033 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -371,6 +371,8 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 	if (result)
 		goto fail0;
 
+	msleep(2);
+
 	/* Start I2C transfer */
 	i2c_imx_start(i2c_imx);

As you understand, this cannot be the final fix. We have to understand why 
a delay is needed there and how long it actually has to be...
 
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 1/5] I2C driver for MXC

[Guennadi Liakhovetski]

On Mon, 30 Mar 2009, Mark Brown wrote:

> On Sat, Mar 28, 2009 at 10:36:50PM +0100, Guennadi Liakhovetski wrote:
> > 
> > different drivers, and I really hoped its new version will work 
> > out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
> > it didn't. I've spent more than a full working day trying to get it to 
> > work, but I didn't succeed so far. It works with the on-board rtc, but it 
> > doesn't work with the camera, connected over a flex cable.
> 
> What problems are you experiencing?  I've been using versions of Darius'
> driver for a while now with no problems on my system.

I couldn't read camera ID register during probing. As I mentioned, the 
camera is connected over a, hm, limited-quality self-soldered (by 
professionals) cable, and I have to reduce bus clock to 20kHz. Has anyone 
tested this driver with lower frequencies?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 1/5] I2C driver for MXC

[Sascha Hauer]

On Mon, Mar 30, 2009 at 11:26:31AM +0200, Guennadi Liakhovetski wrote:
> Hi Darius,
> 
> On Mon, 30 Mar 2009, Darius Augulis wrote:
> 
> > I tested your driver on i.MXL with OV7670 and MT9V111 cameras. I confirm it
> > works.
> 
> Thanks for testing!
> 
> > In otherside, my driver is also tested with OV7670, MT9V111, OV7720 cameras,
> > PCF8575 expander and NT7651 LCD controller. All these devices work connected
> > to i.MXL.
> > If you have problems on MX3 only with single device, it may be caused by
> > some specific conditions.
> > I don't know it is good idea to replace one driver by another, because we
> > don't know where is the problem exactly.
> > Of course would be fine to watch your I2C bus with oscilloscope. So you
> > could see what is wrong.
> > IMO we should find what is wrong in my driver, to have good working driver
> > for all MXC SoC's.
> > If we replace it by your driver, we miss lot of ML comments fixed and
> > probably we will receive errors and problems in some different situation.
> 
> Well, I have been able to get your driver to at least pass the 
> initialisation with mt9t031 (other parts are missing yet for a complete 
> test). For that I used this silly patch:
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 3296380..46e1033 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -371,6 +371,8 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>  	if (result)
>  		goto fail0;
>  
> +	msleep(2);
> +
>  	/* Start I2C transfer */
>  	i2c_imx_start(i2c_imx);
> 
> As you understand, this cannot be the final fix. We have to understand why 
> a delay is needed there and how long it actually has to be...
>  

Have you checked the value of disable_delay in Darius' driver or tried to
increase it?

BTW I just realized that the handling of disable_delay in Darius' driver
is wrong. This should be a per device variable.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/5] I2C driver for MXC

[Guennadi Liakhovetski]

On Mon, 30 Mar 2009, Sascha Hauer wrote:

> On Mon, Mar 30, 2009 at 11:26:31AM +0200, Guennadi Liakhovetski wrote:
> > 
> > Well, I have been able to get your driver to at least pass the 
> > initialisation with mt9t031 (other parts are missing yet for a complete 
> > test). For that I used this silly patch:
> > 
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index 3296380..46e1033 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -371,6 +371,8 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
> >  	if (result)
> >  		goto fail0;
> >  
> > +	msleep(2);
> > +
> >  	/* Start I2C transfer */
> >  	i2c_imx_start(i2c_imx);
> > 
> > As you understand, this cannot be the final fix. We have to understand why 
> > a delay is needed there and how long it actually has to be...
> >  
> 
> Have you checked the value of disable_delay in Darius' driver or tried to
> increase it?

Why? That variable is addreccing a problem on a completely different 
hardware:

	/*
	 * There dummy delay is calculated.
	 * It should be about one I2C clock period long.
	 * This delay is used in I2C bus disable function
	 * to fix chip hardware bug.
	 */

and is used on a different path - during controller disable.

> BTW I just realized that the handling of disable_delay in Darius' driver
> is wrong. This should be a per device variable.

Indeed.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 1/5] I2C driver for MXC

[Guennadi Liakhovetski]

Ok, I just was informed, that there is a hardware bug on the board I am 
using. So, please ignore my objections. Sorry for the wasted time.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

[PATCH] imx/i2c: Make disable_delay a per-device variable

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2427 bytes --]

On Mon, Mar 30, 2009 at 03:50:28PM +0200, Guennadi Liakhovetski wrote:

> Ok, I just was informed, that there is a hardware bug on the board I am 
> using. So, please ignore my objections. Sorry for the wasted time.

Well, not completely wasted: We found a bug :) Patch follows, Ben can
you add it?

Regards,

   Wolfram

===

'disable_delay' was static which is wrong as it is calculated using the per-device
bus speed. This patch turns 'disable_delay' into a per-device variable.

Reported-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-imx.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Index: .kernel/drivers/i2c/busses/i2c-imx.c
===================================================================
--- .kernel.orig/drivers/i2c/busses/i2c-imx.c
+++ .kernel/drivers/i2c/busses/i2c-imx.c
@@ -86,8 +86,6 @@
 /** Variables ******************************************************************
 *******************************************************************************/
 
-static unsigned int disable_delay;	/* Dummy delay */
-
 /*
  * sorted list of clock divider, register value pairs
  * taken from table 26-5, p.26-9, Freescale i.MX
@@ -121,6 +119,7 @@ struct imx_i2c_struct {
 	int			irq;
 	wait_queue_head_t	queue;
 	unsigned long		i2csr;
+	unsigned int 		disable_delay;
 };
 
 /** Functions for IMX I2C adapter driver ***************************************
@@ -212,7 +211,7 @@ static void i2c_imx_stop(struct imx_i2c_
 	 * This delay caused by an i.MXL hardware bug.
 	 * If no (or too short) delay, no "STOP" bit will be generated.
 	 */
-	udelay(disable_delay);
+	udelay(i2c_imx->disable_delay);
 	/* Disable I2C controller */
 	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
 }
@@ -243,7 +242,7 @@ static void __init i2c_imx_set_clk(struc
 	 * This delay is used in I2C bus disable function
 	 * to fix chip hardware bug.
 	 */
-	disable_delay = (500000U * i2c_clk_div[i][0]
+	i2c_imx->disable_delay = (500000U * i2c_clk_div[i][0]
 		+ (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
 
 	/* dev_dbg() can't be used, because adapter is not yet registered */

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/5] I2C driver for MXC

[Richard Zhao]

On Mon, Mar 30, 2009 at 9:50 PM, Guennadi Liakhovetski
<g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
> Ok, I just was informed, that there is a hardware bug on the board I am
> using. So, please ignore my objections. Sorry for the wasted time.
>
> Thanks
> Guennadi
> ---
It seems Guennadi's patch is based on a newer(not latest) version i2c
driver from freescale.
I suggest we create patch based on the latest freescale kernel source
to avoid fragment.

Thanks
Richard

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Ben Dooks wrote:
> On Sat, Mar 28, 2009 at 10:36:50PM +0100, Guennadi Liakhovetski wrote:
>   
>> On Wed, 25 Mar 2009, Wolfram Sang wrote:
>>
>>     
>>>> Because I hurry to submit this, because merge window is open.
>>>>         
>> Ok, I waited for this driver, I really did, and I really prefer having one 
>> driver for as many hardware (SoC) variants as possible instead of having 
>> different drivers, and I really hoped its new version will work 
>> out-of-the-box in my setup (pcm037 with a mt9t031 camera). Unfortunately, 
>> it didn't. I've spent more than a full working day trying to get it to 
>> work, but I didn't succeed so far. It works with the on-board rtc, but it 
>> doesn't work with the camera, connected over a flex cable.
>>
>> Attached to this email is a version of the i2c driver for i.mx31 that I've 
>> been using with my setup for about half a year now. I adjusted it slightly 
>> to accept the same platform data, so, it is really a drop-in replacement 
>> for i2c-imx.c. Of course, you have to adjust or extend the Makefile. I 
>> actually added a new Kconfig entry for this driver, so I could easily 
>> compare them during the tests.
>>
>> The source of "my" version does look more logical and more clean to me on 
>> quite a few occasions. So, maybe someone could give it a quick spin on 
>> other *mx* SoCs and see if we can use this one instead? You might have to 
>> replace {read,write}w with readb and writeb, so, it might be a good idea 
>> to abstract them into inlines or macros. Otherwise, I think, it might work 
>> for other CPUs straight away.
>>
>> I just would like to avoid committing a driver and having to spend hours 
>> or days fixing it afterwards when a possibly more mature version exists.
>>     
>
> I'm going to hold off pushing my current tree until this/tommorow
> evening so that people have some time to get back to me on what is
> the best way to proceed.
>  
>   

Hi Ben,

Jean already requested Linus to pull I2C updates, how about imx-i2c?
Should we have this in rc1?

Darius.

Re: [PATCH 1/5] I2C driver for MXC

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 365 bytes --]

> Jean already requested Linus to pull I2C updates, how about imx-i2c?
> Should we have this in rc1?

I think he said he will do it today evening so we had time to get the
i2c-imx problems sorted out.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Richard Zhao wrote:
> On Mon, Mar 30, 2009 at 9:50 PM, Guennadi Liakhovetski
> <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org> wrote:
>> Ok, I just was informed, that there is a hardware bug on the board I am
>> using. So, please ignore my objections. Sorry for the wasted time.
>>
>> Thanks
>> Guennadi
>> ---
> It seems Guennadi's patch is based on a newer(not latest) version i2c
> driver from freescale.
> I suggest we create patch based on the latest freescale kernel source
> to avoid fragment.

People from Freescale are welcome there to discuss and help creating drivers for Freescale processors.
I think anybody hasn't time to base their work on some non- mainline projects.

> 
> Thanks
> Richard

Re: [PATCH 1/5] I2C driver for MXC

[Holger Schurig]

Hi Darius !

I can confirm that your driver works on my i.MX21 device with two 
pca9535 chips. However, I had to add a patch to make it usable, 
which I'll send in a minute.

Greetings,
Holger




-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 1/5] I2C driver for MXC

[Holger Schurig]

[PATCH] imx21: activate i2c

Set the correct clkdev-name for the i2c clock.

It also get's rid of the ARCH_NR_GPIOS define on the rationale
that isn't an ARCH-wide setting anyway. If a device has two
pca953x devices, the reserved number will be wrong.

Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>

---

This patch applies on top Darius' patch "[PATCH 1/5] I2C driver for MXC" from
24th march.

--- linux.orig/arch/arm/plat-mxc/include/mach/mx21.h
+++ linux/arch/arm/plat-mxc/include/mach/mx21.h
@@ -54,9 +54,6 @@
 
 #define IRAM_BASE_ADDR          0xFFFFE800	/* internal ram */
 
-/* this CPU supports up to 192 GPIOs (don't forget the baseboard!) */
-#define ARCH_NR_GPIOS		(6*32 + 16)
-
 /* fixed interrupt numbers */
 #define MXC_INT_USBCTRL         58
 #define MXC_INT_USBCTRL         58
--- linux.orig/arch/arm/mach-mx2/clock_imx21.c
+++ linux/arch/arm/mach-mx2/clock_imx21.c
@@ -931,7 +931,7 @@
 	_REGISTER_CLOCK(NULL, "slcdc", slcdc_clk[0])
 	_REGISTER_CLOCK("imx-wdt.0", NULL, wdog_clk)
 	_REGISTER_CLOCK(NULL, "gpio", gpio_clk)
-	_REGISTER_CLOCK(NULL, "i2c", i2c_clk)
+	_REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
 	_REGISTER_CLOCK("mxc-keypad", NULL, kpp_clk)
 	_REGISTER_CLOCK(NULL, "owire", owire_clk)
 	_REGISTER_CLOCK(NULL, "rtc", rtc_clk)




-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 1/5] I2C driver for MXC

[Guennadi Liakhovetski]

On Mon, 30 Mar 2009, Guennadi Liakhovetski wrote:

> Ok, I just was informed, that there is a hardware bug on the board I am 
> using. So, please ignore my objections. Sorry for the wasted time.

Got a fixed board, verified - the original driver from Darius works 
without any modifications.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Guennadi Liakhovetski wrote:
> On Mon, 30 Mar 2009, Guennadi Liakhovetski wrote:
> 
>> Ok, I just was informed, that there is a hardware bug on the board I am 
>> using. So, please ignore my objections. Sorry for the wasted time.
> 
> Got a fixed board, verified - the original driver from Darius works 
> without any modifications.

Thanks for testing and feedback!

> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer

Re: [PATCH 1/5] I2C driver for MXC

[Darius Augulis]

Holger Schurig wrote:
> Hi Darius !
> 
> I can confirm that your driver works on my i.MX21 device with two 
> pca9535 chips. However, I had to add a patch to make it usable, 
> which I'll send in a minute.

Thanks for testing and feedback!

> 
> Greetings,
> Holger
> 
> 
> 
> 
> -------------------------------------------------------------------
> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
> FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
> Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php
> 

Re: [PATCH] imx/i2c: Make disable_delay a per-device variable

[Guennadi Liakhovetski]

On Mon, 30 Mar 2009, Wolfram Sang wrote:

> On Mon, Mar 30, 2009 at 03:50:28PM +0200, Guennadi Liakhovetski wrote:
> 
> > Ok, I just was informed, that there is a hardware bug on the board I am 
> > using. So, please ignore my objections. Sorry for the wasted time.
> 
> Well, not completely wasted: We found a bug :) Patch follows, Ben can
> you add it?
> 
> Regards,
> 
>    Wolfram
> 
> ===
> 
> 'disable_delay' was static which is wrong as it is calculated using the per-device
> bus speed. This patch turns 'disable_delay' into a per-device variable.
> 
> Reported-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Acked-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>

Thanks
Guennadi

> ---
>  drivers/i2c/busses/i2c-imx.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> Index: .kernel/drivers/i2c/busses/i2c-imx.c
> ===================================================================
> --- .kernel.orig/drivers/i2c/busses/i2c-imx.c
> +++ .kernel/drivers/i2c/busses/i2c-imx.c
> @@ -86,8 +86,6 @@
>  /** Variables ******************************************************************
>  *******************************************************************************/
>  
> -static unsigned int disable_delay;	/* Dummy delay */
> -
>  /*
>   * sorted list of clock divider, register value pairs
>   * taken from table 26-5, p.26-9, Freescale i.MX
> @@ -121,6 +119,7 @@ struct imx_i2c_struct {
>  	int			irq;
>  	wait_queue_head_t	queue;
>  	unsigned long		i2csr;
> +	unsigned int 		disable_delay;
>  };
>  
>  /** Functions for IMX I2C adapter driver ***************************************
> @@ -212,7 +211,7 @@ static void i2c_imx_stop(struct imx_i2c_
>  	 * This delay caused by an i.MXL hardware bug.
>  	 * If no (or too short) delay, no "STOP" bit will be generated.
>  	 */
> -	udelay(disable_delay);
> +	udelay(i2c_imx->disable_delay);
>  	/* Disable I2C controller */
>  	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
>  }
> @@ -243,7 +242,7 @@ static void __init i2c_imx_set_clk(struc
>  	 * This delay is used in I2C bus disable function
>  	 * to fix chip hardware bug.
>  	 */
> -	disable_delay = (500000U * i2c_clk_div[i][0]
> +	i2c_imx->disable_delay = (500000U * i2c_clk_div[i][0]
>  		+ (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
>  
>  	/* dev_dbg() can't be used, because adapter is not yet registered */
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH] imx/i2c: Make disable_delay a per-device variable

[Darius Augulis]

Wolfram Sang wrote:
> On Mon, Mar 30, 2009 at 03:50:28PM +0200, Guennadi Liakhovetski wrote:
> 
>> Ok, I just was informed, that there is a hardware bug on the board I am 
>> using. So, please ignore my objections. Sorry for the wasted time.
> 
> Well, not completely wasted: We found a bug :) Patch follows, Ben can
> you add it?
> 
> Regards,
> 
>    Wolfram
> 
> ===
> 
> 'disable_delay' was static which is wrong as it is calculated using the per-device
> bus speed. This patch turns 'disable_delay' into a per-device variable.
> 
> Reported-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> ---


Maybe I'm to late, but:
Acked-by: Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


>  drivers/i2c/busses/i2c-imx.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> Index: .kernel/drivers/i2c/busses/i2c-imx.c
> ===================================================================
> --- .kernel.orig/drivers/i2c/busses/i2c-imx.c
> +++ .kernel/drivers/i2c/busses/i2c-imx.c
> @@ -86,8 +86,6 @@
>  /** Variables ******************************************************************
>  *******************************************************************************/
>  
> -static unsigned int disable_delay;	/* Dummy delay */
> -
>  /*
>   * sorted list of clock divider, register value pairs
>   * taken from table 26-5, p.26-9, Freescale i.MX
> @@ -121,6 +119,7 @@ struct imx_i2c_struct {
>  	int			irq;
>  	wait_queue_head_t	queue;
>  	unsigned long		i2csr;
> +	unsigned int 		disable_delay;
>  };
>  
>  /** Functions for IMX I2C adapter driver ***************************************
> @@ -212,7 +211,7 @@ static void i2c_imx_stop(struct imx_i2c_
>  	 * This delay caused by an i.MXL hardware bug.
>  	 * If no (or too short) delay, no "STOP" bit will be generated.
>  	 */
> -	udelay(disable_delay);
> +	udelay(i2c_imx->disable_delay);
>  	/* Disable I2C controller */
>  	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
>  }
> @@ -243,7 +242,7 @@ static void __init i2c_imx_set_clk(struc
>  	 * This delay is used in I2C bus disable function
>  	 * to fix chip hardware bug.
>  	 */
> -	disable_delay = (500000U * i2c_clk_div[i][0]
> +	i2c_imx->disable_delay = (500000U * i2c_clk_div[i][0]
>  		+ (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2);
>  
>  	/* dev_dbg() can't be used, because adapter is not yet registered */
> 

Re: [PATCH 1/5] I2C driver for MXC

[Sascha Hauer]

On Wed, Apr 01, 2009 at 01:58:21PM +0200, Holger Schurig wrote:
> [PATCH] imx21: activate i2c
> 
> Set the correct clkdev-name for the i2c clock.
> 
> It also get's rid of the ARCH_NR_GPIOS define on the rationale
> that isn't an ARCH-wide setting anyway. If a device has two
> pca953x devices, the reserved number will be wrong.
> 
> Signed-off-by: Holger Schurig <hs4233-x6+DxXLjN1AJvtFkdXX2Hg4jNU5vUVPG@public.gmane.org>

Scheduled for -rc

Sascha

> 
> ---
> 
> This patch applies on top Darius' patch "[PATCH 1/5] I2C driver for MXC" from
> 24th march.
> 
> --- linux.orig/arch/arm/plat-mxc/include/mach/mx21.h
> +++ linux/arch/arm/plat-mxc/include/mach/mx21.h
> @@ -54,9 +54,6 @@
>  
>  #define IRAM_BASE_ADDR          0xFFFFE800	/* internal ram */
>  
> -/* this CPU supports up to 192 GPIOs (don't forget the baseboard!) */
> -#define ARCH_NR_GPIOS		(6*32 + 16)
> -
>  /* fixed interrupt numbers */
>  #define MXC_INT_USBCTRL         58
>  #define MXC_INT_USBCTRL         58
> --- linux.orig/arch/arm/mach-mx2/clock_imx21.c
> +++ linux/arch/arm/mach-mx2/clock_imx21.c
> @@ -931,7 +931,7 @@
>  	_REGISTER_CLOCK(NULL, "slcdc", slcdc_clk[0])
>  	_REGISTER_CLOCK("imx-wdt.0", NULL, wdog_clk)
>  	_REGISTER_CLOCK(NULL, "gpio", gpio_clk)
> -	_REGISTER_CLOCK(NULL, "i2c", i2c_clk)
> +	_REGISTER_CLOCK("imx-i2c.0", NULL, i2c_clk)
>  	_REGISTER_CLOCK("mxc-keypad", NULL, kpp_clk)
>  	_REGISTER_CLOCK(NULL, "owire", owire_clk)
>  	_REGISTER_CLOCK(NULL, "rtc", rtc_clk)
> 
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |