[PATCH][resend] iMX/MXC support for I2C

[PATCH][resend] iMX/MXC support for I2C

[Darius]

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



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

From: Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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>

Index: linux-2.6.28-rc6/drivers/i2c/busses/i2c-imx.c
===================================================================
--- /dev/null
+++ linux-2.6.28-rc6/drivers/i2c/busses/i2c-imx.c
@@ -0,0 +1,656 @@
+/*
+ *	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
+ *
+ *	module parameters:
+ *		- clkfreq:
+ *			Sets the desired clock rate
+ *			The default value is 100000
+ *			Max value is 400000
+ *
+ *	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 values of module parameters */
+#define IMX_I2C_BIT_RATE	100000	/* 100kHz */
+
+/* Timeouts */
+#define I2C_IMX_TIME_BUSY	2000	/* loop count */
+#define I2C_IMX_TIME_ACK	2000	/* loop count */
+#define I2C_IMX_TIME_TRX	5	/* seconds */
+
+/* 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 clkfreq = IMX_I2C_BIT_RATE;
+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)) {
+			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, I2C_IMX_TIME_TRX * HZ);
+
+	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)
+{
+	unsigned long orig_jiffies = jiffies;
+
+	/* Wait for ack */
+	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
+		if (signal_pending(current)) {
+			dev_dbg(&i2c_imx->adapter.dev,
+				"<%s> I2C Interrupted\n", __func__);
+			return -EINTR;
+		}
+		if (time_after(jiffies, orig_jiffies + HZ)) {
+			dev_dbg(&i2c_imx->adapter.dev,
+				"<%s> No ACK\n", __func__);
+			return -EIO;
+		}
+		schedule();
+	}
+
+	dev_dbg(&i2c_imx->adapter.dev, "<%s> ACK received\n", __func__);
+	return 0;   /* No ACK */
+}
+
+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;
+}
+
+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);
+
+	/* write slave address */
+	writeb(msgs->addr, 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 | 0x01);
+
+	/* write slave address */
+	writeb(msgs->addr | 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;
+	int irq;
+	int res_size;
+	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;
+	}
+
+	res_size = res->end - res->start + 1;
+	if (!request_mem_region(res->start, res_size, res->name)) {
+		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
+			res_size, res->start);
+		return -ENOMEM;
+	}
+
+	if (pdata->init) {
+		ret = pdata->init(&pdev->dev);
+		if (ret)
+			goto fail0;
+	}
+
+	base = ioremap(res->start, res_size);
+	if (!base) {
+		dev_err(&pdev->dev, "ioremap failed\n");
+		ret = -EIO;
+		goto fail1;
+	}
+
+	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 fail2;
+	}
+
+	/* 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(NULL, "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 fail3;
+	}
+	clk_enable(i2c_imx->clk);
+
+	/* Request IRQ */
+	ret = request_irq(i2c_imx->irq, i2c_imx_isr,
+		IRQF_DISABLED, pdev->name, i2c_imx);
+	if (ret) {
+		dev_err(&pdev->dev, "can't claim irq %d\n", i2c_imx->irq);
+		goto fail4;
+	}
+
+	/* Init queue */
+	init_waitqueue_head(&i2c_imx->queue);
+
+	/* Set up adapter data */
+	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
+
+	/* Set up clock divider */
+	i2c_imx_set_clk(i2c_imx, clkfreq);
+
+	/* 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 fail5;
+	}
+
+	/* 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 */
+
+fail5:
+	free_irq(i2c_imx->irq, i2c_imx);
+fail4:
+	clk_disable(i2c_imx->clk);
+	clk_put(i2c_imx->clk);
+fail3:
+	kfree(i2c_imx);
+fail2:
+	iounmap(base);
+fail1:
+	if (pdata->exit)
+		pdata->exit(&pdev->dev);
+fail0:
+	release_mem_region(res->start, res_size);
+	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_param(clkfreq, uint, S_IRUGO);
+MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");
+
+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.28-rc6/drivers/i2c/busses/Kconfig
===================================================================
--- linux-2.6.28-rc6.orig/drivers/i2c/busses/Kconfig
+++ linux-2.6.28-rc6/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.28-rc6/drivers/i2c/busses/Makefile
===================================================================
--- linux-2.6.28-rc6.orig/drivers/i2c/busses/Makefile
+++ linux-2.6.28-rc6/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.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
===================================================================
--- /dev/null
+++ linux-2.6.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
@@ -0,0 +1,17 @@
+/*
+ * i2c.h - i.MX I2C driver header file
+ *
+ * Copyright (c) 2008, Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __ASM_ARCH_I2C_H_
+#define __ASM_ARCH_I2C_H_
+
+struct imxi2c_platform_data {
+	int (*init)(struct device *dev);
+	int (*exit)(struct device *dev);
+};
+
+#endif /* __ASM_ARCH_I2C_H_ */

Re: [PATCH][resend] iMX/MXC support for I2C

[Darius]

Please review and add this for 2.6.29 if it's ok.

Re: [PATCH][resend] iMX/MXC support for I2C

[Jean Delvare]

On Tue, 02 Dec 2008 16:04:55 +0200, Darius wrote:
> Please review and add this for 2.6.29 if it's ok.

Ben, can you please take care of this?

Thanks,
-- 
Jean Delvare

Re: [PATCH][resend] iMX/MXC support for I2C

[Guennadi Liakhovetski]

The good news is - I was able to get this driver running on i.MX31 - 
somewhat... Now the comments:

On Tue, 2 Dec 2008, Darius wrote:

Commenting would have been easier, if you had inlined your patch...

> From augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org Tue Dec  2 15:15:21 2008
> Date: Tue, 02 Dec 2008 16:00:20 +0200
> From: Darius <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW@public.gmane.org
> Subject: [PATCH][resend] iMX/MXC support for I2C
> 
> 
>     [ Empty or malformed message. Use "H" to see raw text. ]
> 
> 
>     [ Part 2: "Attached Text" ]
> 
> From: Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> 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>
> 
> Index: linux-2.6.28-rc6/drivers/i2c/busses/i2c-imx.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.28-rc6/drivers/i2c/busses/i2c-imx.c
> @@ -0,0 +1,656 @@
> +/*
> + *	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
> + *
> + *	module parameters:
> + *		- clkfreq:
> + *			Sets the desired clock rate
> + *			The default value is 100000
> + *			Max value is 400000
> + *
> + *	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 values of module parameters */
> +#define IMX_I2C_BIT_RATE	100000	/* 100kHz */
> +
> +/* Timeouts */
> +#define I2C_IMX_TIME_BUSY	2000	/* loop count */
> +#define I2C_IMX_TIME_ACK	2000	/* loop count */
> +#define I2C_IMX_TIME_TRX	5	/* seconds */
> +
> +/* 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 clkfreq = IMX_I2C_BIT_RATE;
> +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) {

The i2c-mxc driver I ported over from Freescale sources uses 16-bit 
access, and the datasheet defines registers to be 16-bit wide, although 
only low 8 bits are used. Whereas an i.MX datasheet (MC9328MX1) I have 
here defines i2c registers to be 32-bit wide.

> +		if (signal_pending(current)) {
> +			dev_dbg(&i2c_imx->adapter.dev,
> +				"<%s> I2C Interrupted\n", __func__);
> +			return -EINTR;
> +		}
> +		if (time_after(jiffies, orig_jiffies + HZ)) {
> +			dev_dbg(&i2c_imx->adapter.dev,
> +				"<%s> I2C bus is busy\n", __func__);
> +			return -EIO;
> +		}
> +		schedule();

For comparison: the MXC driver only waits for bus busy to clear for 600us 
at most.

> +	}
> +
> +	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, I2C_IMX_TIME_TRX * HZ);

5s is much too long!

> +
> +	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)
> +{
> +	unsigned long orig_jiffies = jiffies;
> +
> +	/* Wait for ack */
> +	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
> +		if (signal_pending(current)) {
> +			dev_dbg(&i2c_imx->adapter.dev,
> +				"<%s> I2C Interrupted\n", __func__);
> +			return -EINTR;
> +		}
> +		if (time_after(jiffies, orig_jiffies + HZ)) {
> +			dev_dbg(&i2c_imx->adapter.dev,
> +				"<%s> No ACK\n", __func__);
> +			return -EIO;
> +		}
> +		schedule();
> +	}

1s is way too long here! i2cdetect takes ages to complete. The mxc driver 
doesn't wait here at all - as soon as a tx is complete, it checks the ack 
status. Could you verify if this would work on your MX1 too?

> +
> +	dev_dbg(&i2c_imx->adapter.dev, "<%s> ACK received\n", __func__);
> +	return 0;   /* No ACK */
> +}
> +
> +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;
> +}
> +
> +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);
> +
> +	/* write slave address */
> +	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);

This is wrong! You have to double the address before writing to the 
register.

> +	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 | 0x01);
> +
> +	/* write slave address */
> +	writeb(msgs->addr | 0x01, i2c_imx->base + IMX_I2C_I2DR);

Same here - (msgs->addr << 1) | 0x01.

> +	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;
> +	int irq;
> +	int res_size;
> +	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;
> +	}
> +
> +	res_size = res->end - res->start + 1;

You can use resource_size()

> +	if (!request_mem_region(res->start, res_size, res->name)) {
> +		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
> +			res_size, res->start);
> +		return -ENOMEM;
> +	}

I was once told, one doesn't need request_mem_region() on regions from 
platform data resources - this is already done implicitly.

> +
> +	if (pdata->init) {
> +		ret = pdata->init(&pdev->dev);
> +		if (ret)
> +			goto fail0;
> +	}
> +
> +	base = ioremap(res->start, res_size);
> +	if (!base) {
> +		dev_err(&pdev->dev, "ioremap failed\n");
> +		ret = -EIO;
> +		goto fail1;
> +	}
> +
> +	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 fail2;
> +	}
> +
> +	/* 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(NULL, "i2c_clk");

There can be several i2c busses on the system, so you want:

+	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 fail3;
> +	}
> +	clk_enable(i2c_imx->clk);
> +
> +	/* Request IRQ */
> +	ret = request_irq(i2c_imx->irq, i2c_imx_isr,
> +		IRQF_DISABLED, pdev->name, i2c_imx);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't claim irq %d\n", i2c_imx->irq);
> +		goto fail4;
> +	}
> +
> +	/* Init queue */
> +	init_waitqueue_head(&i2c_imx->queue);
> +
> +	/* Set up adapter data */
> +	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
> +
> +	/* Set up clock divider */
> +	i2c_imx_set_clk(i2c_imx, clkfreq);
> +
> +	/* 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 fail5;
> +	}
> +
> +	/* 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 */
> +
> +fail5:
> +	free_irq(i2c_imx->irq, i2c_imx);
> +fail4:
> +	clk_disable(i2c_imx->clk);
> +	clk_put(i2c_imx->clk);
> +fail3:
> +	kfree(i2c_imx);
> +fail2:
> +	iounmap(base);
> +fail1:
> +	if (pdata->exit)
> +		pdata->exit(&pdev->dev);
> +fail0:
> +	release_mem_region(res->start, res_size);
> +	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_param(clkfreq, uint, S_IRUGO);
> +MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");

Making clkfreq a module parameter you force the same frequency on all i2c 
busses. On my i.MX31 system 2 busses are internal and one goes to a 
connector, to which a camera is connected. This third bus can only handle 
a lower frequency, which, however, doesn't mean we also have to throttle 
the other two busses. Can we put this into platform data?

> +
> +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.28-rc6/drivers/i2c/busses/Kconfig
> ===================================================================
> --- linux-2.6.28-rc6.orig/drivers/i2c/busses/Kconfig
> +++ linux-2.6.28-rc6/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.28-rc6/drivers/i2c/busses/Makefile
> ===================================================================
> --- linux-2.6.28-rc6.orig/drivers/i2c/busses/Makefile
> +++ linux-2.6.28-rc6/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.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
> @@ -0,0 +1,17 @@
> +/*
> + * i2c.h - i.MX I2C driver header file
> + *
> + * Copyright (c) 2008, Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __ASM_ARCH_I2C_H_
> +#define __ASM_ARCH_I2C_H_
> +
> +struct imxi2c_platform_data {
> +	int (*init)(struct device *dev);
> +	int (*exit)(struct device *dev);

What are you going to use .exit() for? Is it really needed? Even if it is, 
it can easily return void I guess?

> +};
> +
> +#endif /* __ASM_ARCH_I2C_H_ */

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Darius]

Guennadi Liakhovetski wrote:
> The good news is - I was able to get this driver running on i.MX31 - 
> somewhat... Now the comments:

Thank you for review and test on MX3!

> 
> On Tue, 2 Dec 2008, Darius wrote:
> 
> Commenting would have been easier, if you had inlined your patch...
> 
>> From augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org Tue Dec  2 15:15:21 2008
>> Date: Tue, 02 Dec 2008 16:00:20 +0200
>> From: Darius <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Cc: linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW@public.gmane.org
>> Subject: [PATCH][resend] iMX/MXC support for I2C
>>
>>
>>     [ Empty or malformed message. Use "H" to see raw text. ]
>>
>>
>>     [ Part 2: "Attached Text" ]
>>
>> From: Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> 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>
>>
>> Index: linux-2.6.28-rc6/drivers/i2c/busses/i2c-imx.c
>> ===================================================================
>> --- /dev/null
>> +++ linux-2.6.28-rc6/drivers/i2c/busses/i2c-imx.c
>> @@ -0,0 +1,656 @@
>> +/*
>> + *	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
>> + *
>> + *	module parameters:
>> + *		- clkfreq:
>> + *			Sets the desired clock rate
>> + *			The default value is 100000
>> + *			Max value is 400000
>> + *
>> + *	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 values of module parameters */
>> +#define IMX_I2C_BIT_RATE	100000	/* 100kHz */
>> +
>> +/* Timeouts */
>> +#define I2C_IMX_TIME_BUSY	2000	/* loop count */
>> +#define I2C_IMX_TIME_ACK	2000	/* loop count */
>> +#define I2C_IMX_TIME_TRX	5	/* seconds */
>> +
>> +/* 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 clkfreq = IMX_I2C_BIT_RATE;
>> +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) {
> 
> The i2c-mxc driver I ported over from Freescale sources uses 16-bit 
> access, and the datasheet defines registers to be 16-bit wide, although 
> only low 8 bits are used. Whereas an i.MX datasheet (MC9328MX1) I have 
> here defines i2c registers to be 32-bit wide.

yes, it's no difference how to access registers - in 8bit or 16bit wide, because only 8 lower bits are used.

> 
>> +		if (signal_pending(current)) {
>> +			dev_dbg(&i2c_imx->adapter.dev,
>> +				"<%s> I2C Interrupted\n", __func__);
>> +			return -EINTR;
>> +		}
>> +		if (time_after(jiffies, orig_jiffies + HZ)) {
>> +			dev_dbg(&i2c_imx->adapter.dev,
>> +				"<%s> I2C bus is busy\n", __func__);
>> +			return -EIO;
>> +		}
>> +		schedule();
> 
> For comparison: the MXC driver only waits for bus busy to clear for 600us 
> at most.

but there is shedule() used, no impact to performace. but I will decrease timeouts.

> 
>> +	}
>> +
>> +	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, I2C_IMX_TIME_TRX * HZ);
> 
> 5s is much too long!

how much? 600us?

> 
>> +
>> +	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)
>> +{
>> +	unsigned long orig_jiffies = jiffies;
>> +
>> +	/* Wait for ack */
>> +	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
>> +		if (signal_pending(current)) {
>> +			dev_dbg(&i2c_imx->adapter.dev,
>> +				"<%s> I2C Interrupted\n", __func__);
>> +			return -EINTR;
>> +		}
>> +		if (time_after(jiffies, orig_jiffies + HZ)) {
>> +			dev_dbg(&i2c_imx->adapter.dev,
>> +				"<%s> No ACK\n", __func__);
>> +			return -EIO;
>> +		}
>> +		schedule();
>> +	}
> 
> 1s is way too long here! i2cdetect takes ages to complete. The mxc driver 
> doesn't wait here at all - as soon as a tx is complete, it checks the ack 
> status. Could you verify if this would work on your MX1 too?

I will try.

> 
>> +
>> +	dev_dbg(&i2c_imx->adapter.dev, "<%s> ACK received\n", __func__);
>> +	return 0;   /* No ACK */
>> +}
>> +
>> +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;
>> +}
>> +
>> +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);
>> +
>> +	/* write slave address */
>> +	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);
> 
> This is wrong! You have to double the address before writing to the 
> register.

strange! there are I2c board data in my MXLADS code:

struct i2c_board_info __initdata mx1ads_i2c_devices[] = {
	{
		I2C_BOARD_INFO("ov7xxx", 0x42),
		.platform_data = &iclink[0],
	}, {
		I2C_BOARD_INFO("mt9v111", 0x90),
		.platform_data = &iclink[0],
	}
}

slave addresses are exactly 0x42 and 0x90 (from datasheets).
my driver works with these devices with address not doubled.
I saw this in other I2C drivers, but If I double address in my driver, it works wrong.
I tested this with oscilloscope - now it works ok, with all devices I have tryed.

> 
>> +	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 | 0x01);
>> +
>> +	/* write slave address */
>> +	writeb(msgs->addr | 0x01, i2c_imx->base + IMX_I2C_I2DR);
> 
> Same here - (msgs->addr << 1) | 0x01.
> 
>> +	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;
>> +	int irq;
>> +	int res_size;
>> +	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;
>> +	}
>> +
>> +	res_size = res->end - res->start + 1;
> 
> You can use resource_size()
> 
>> +	if (!request_mem_region(res->start, res_size, res->name)) {
>> +		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
>> +			res_size, res->start);
>> +		return -ENOMEM;
>> +	}
> 
> I was once told, one doesn't need request_mem_region() on regions from 
> platform data resources - this is already done implicitly.

ok, i will fix it.

> 
>> +
>> +	if (pdata->init) {
>> +		ret = pdata->init(&pdev->dev);
>> +		if (ret)
>> +			goto fail0;
>> +	}
>> +
>> +	base = ioremap(res->start, res_size);
>> +	if (!base) {
>> +		dev_err(&pdev->dev, "ioremap failed\n");
>> +		ret = -EIO;
>> +		goto fail1;
>> +	}
>> +
>> +	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 fail2;
>> +	}
>> +
>> +	/* 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(NULL, "i2c_clk");
> 
> There can be several i2c busses on the system, so you want:
> 
> +	i2c_imx->clk = clk_get(&pdev->dev, "i2c_clk");

ok, fixed.

> 
>> +	if (IS_ERR(i2c_imx->clk)) {
>> +		ret = PTR_ERR(i2c_imx->clk);
>> +		dev_err(&pdev->dev, "can't get I2C clock\n");
>> +		goto fail3;
>> +	}
>> +	clk_enable(i2c_imx->clk);
>> +
>> +	/* Request IRQ */
>> +	ret = request_irq(i2c_imx->irq, i2c_imx_isr,
>> +		IRQF_DISABLED, pdev->name, i2c_imx);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "can't claim irq %d\n", i2c_imx->irq);
>> +		goto fail4;
>> +	}
>> +
>> +	/* Init queue */
>> +	init_waitqueue_head(&i2c_imx->queue);
>> +
>> +	/* Set up adapter data */
>> +	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
>> +
>> +	/* Set up clock divider */
>> +	i2c_imx_set_clk(i2c_imx, clkfreq);
>> +
>> +	/* 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 fail5;
>> +	}
>> +
>> +	/* 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 */
>> +
>> +fail5:
>> +	free_irq(i2c_imx->irq, i2c_imx);
>> +fail4:
>> +	clk_disable(i2c_imx->clk);
>> +	clk_put(i2c_imx->clk);
>> +fail3:
>> +	kfree(i2c_imx);
>> +fail2:
>> +	iounmap(base);
>> +fail1:
>> +	if (pdata->exit)
>> +		pdata->exit(&pdev->dev);
>> +fail0:
>> +	release_mem_region(res->start, res_size);
>> +	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_param(clkfreq, uint, S_IRUGO);
>> +MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");
> 
> Making clkfreq a module parameter you force the same frequency on all i2c 
> busses. On my i.MX31 system 2 busses are internal and one goes to a 
> connector, to which a camera is connected. This third bus can only handle 
> a lower frequency, which, however, doesn't mean we also have to throttle 
> the other two busses. Can we put this into platform data?

We can do that, but now there is possibility to change bitrate when re-loading module.
What is better?

> 
>> +
>> +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.28-rc6/drivers/i2c/busses/Kconfig
>> ===================================================================
>> --- linux-2.6.28-rc6.orig/drivers/i2c/busses/Kconfig
>> +++ linux-2.6.28-rc6/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.28-rc6/drivers/i2c/busses/Makefile
>> ===================================================================
>> --- linux-2.6.28-rc6.orig/drivers/i2c/busses/Makefile
>> +++ linux-2.6.28-rc6/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.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
>> ===================================================================
>> --- /dev/null
>> +++ linux-2.6.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
>> @@ -0,0 +1,17 @@
>> +/*
>> + * i2c.h - i.MX I2C driver header file
>> + *
>> + * Copyright (c) 2008, Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + *
>> + * This file is released under the GPLv2
>> + */
>> +
>> +#ifndef __ASM_ARCH_I2C_H_
>> +#define __ASM_ARCH_I2C_H_
>> +
>> +struct imxi2c_platform_data {
>> +	int (*init)(struct device *dev);
>> +	int (*exit)(struct device *dev);
> 
> What are you going to use .exit() for? Is it really needed? Even if it is, 
> it can easily return void I guess?

.init is used to request and setup gpio pins, .exit used to free gpio.
yes, .exit can return void - I will fix it.

> 
>> +};
>> +
>> +#endif /* __ASM_ARCH_I2C_H_ */
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer

Re: [PATCH][resend] iMX/MXC support for I2C

[Guennadi Liakhovetski]

One more thing: you also want to do something like this

	i2c_imx->adapter.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;

in i2c_imx_probe().

If you want, I can send you my current version of your driver, so you can 
compare.

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Darius]

Guennadi Liakhovetski wrote:
> One more thing: you also want to do something like this
> 
> 	i2c_imx->adapter.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;

some time ago, it was I2C_CLASS_ALL, but Jean told, that it will be removed at all :)

> 
> in i2c_imx_probe().
> 
> If you want, I can send you my current version of your driver, so you can 
> compare.

yes, please send it.

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Wolfram Sang]

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


On Wed, Dec 03, 2008 at 03:07:08PM +0200, Darius wrote:
> Guennadi Liakhovetski wrote:
>> One more thing: you also want to do something like this
>>
>> 	i2c_imx->adapter.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;
>
> some time ago, it was I2C_CLASS_ALL, but Jean told, that it will be removed at all :)

I also think it is better to not ask for probing. Those flags were
recently removed for some platforms, see for example commit
0a346dacee18ff69f6162d9860d723a058f47321 or commit
618b26d52843c0f85b8eb143cf2695d7f6fd072d. It usually causes more
problems on embedded boards.

Kind regards,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Mark Brown]

On Wed, Dec 03, 2008 at 02:52:56PM +0200, Darius wrote:
> Guennadi Liakhovetski wrote:

> >>+	/* write slave address */
> >>+	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);

> >This is wrong! You have to double the address before writing to the 
> >register.

> strange! there are I2c board data in my MXLADS code:

...

> slave addresses are exactly 0x42 and 0x90 (from datasheets).
> my driver works with these devices with address not doubled.
> I saw this in other I2C drivers, but If I double address in my driver, it 
> works wrong.

Many datasheets quote the I2C address in wire format (ie, after the
doubling, including the read/write bit as LSB) that the hardware is
looking for here.  The Linux I2C framework doesn't do this, it doesn't
include the read/write bit in the address.

> >Making clkfreq a module parameter you force the same frequency on all i2c 
> >busses. On my i.MX31 system 2 busses are internal and one goes to a 
> >connector, to which a camera is connected. This third bus can only handle 
> >a lower frequency, which, however, doesn't mean we also have to throttle 
> >the other two busses. Can we put this into platform data?

> We can do that, but now there is possibility to change bitrate when 
> re-loading module.
> What is better?

Platform data is better but you could also include a default for use
when no platform data is specified and then have a module parameter to
override that.

Re: [PATCH][resend] iMX/MXC support for I2C

[Guennadi Liakhovetski]

Hi Darius,

it would be better, if you didn't drop my email from cc, thanks.

On Wed, 3 Dec 2008, Darius wrote:

> Guennadi Liakhovetski wrote:
> > One more thing: you also want to do something like this
> > 
> > 	i2c_imx->adapter.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;
> 
> some time ago, it was I2C_CLASS_ALL, but Jean told, that it will be removed at
> all :)
> 
> > 
> > in i2c_imx_probe().
> > 
> > If you want, I can send you my current version of your driver, so you can
> > compare.
> 
> yes, please send it.

Below.

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

>From 1d592e3a94b3e13537a680e3e0198c345248692d Mon Sep 17 00:00:00 2001
From: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
Date: Wed, 3 Dec 2008 17:02:37 +0100
Subject: [PATCH] i2x-imx: fix client addresses, remove needless delays, support multiple busses

With above bugs fixed now also tested to work on i.MX31. bus frequency is now
configured over platform data, which allows different frequencies on different
busses. Add an adapter.class assignment.

Signed-off-by: Guennadi Liakhovetski <lg-ynQEQJNshbs@public.gmane.org>
---
 arch/arm/plat-mxc/include/mach/i2c.h |    8 ++
 drivers/i2c/busses/i2c-imx.c         |  143 ++++++++++++++++------------------
 2 files changed, 76 insertions(+), 75 deletions(-)

diff --git a/arch/arm/plat-mxc/include/mach/i2c.h b/arch/arm/plat-mxc/include/mach/i2c.h
index 9e6a9d8..37d068b 100644
--- a/arch/arm/plat-mxc/include/mach/i2c.h
+++ b/arch/arm/plat-mxc/include/mach/i2c.h
@@ -10,8 +10,16 @@
 #define __ASM_ARCH_I2C_H_
 
 struct imxi2c_platform_data {
+	u32 i2c_clk;
 	int (*init)(struct device *dev);
 	int (*exit)(struct device *dev);
 };
 
+struct imxi2c_board_data {
+	struct imxi2c_platform_data	data;
+	unsigned int			id;
+};
+
+extern int mxc_i2c_register_adapters(struct imxi2c_board_data *data, int n);
+
 #endif /* __ASM_ARCH_I2C_H_ */
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index fd2d16d..eeea7d7 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -23,12 +23,6 @@
  *	Implementation of I2C Adapter/Algorithm Driver
  *	for I2C Bus integrated in Freescale i.MX/MXC processors
  *
- *	module parameters:
- *		- clkfreq:
- *			Sets the desired clock rate
- *			The default value is 100000
- *			Max value is 400000
- *
  *	Derived from Motorola GSG China I2C example driver
  *
  *	Copyright (C) 2005 Torsten Koschorrek <koschorrek at synertronixx.de
@@ -64,14 +58,6 @@
 /* This will be the driver name the kernel reports */
 #define DRIVER_NAME "imx-i2c"
 
-/* Default values of module parameters */
-#define IMX_I2C_BIT_RATE	100000	/* 100kHz */
-
-/* Timeouts */
-#define I2C_IMX_TIME_BUSY	2000	/* loop count */
-#define I2C_IMX_TIME_ACK	2000	/* loop count */
-#define I2C_IMX_TIME_TRX	5	/* seconds */
-
 /* IMX I2C registers */
 #define IMX_I2C_IADR	0x00	/* i2c slave address */
 #define IMX_I2C_IFDR	0x04	/* i2c frequency divider */
@@ -97,7 +83,6 @@
 /** Variables ******************************************************************
 *******************************************************************************/
 
-static unsigned int clkfreq = IMX_I2C_BIT_RATE;
 static unsigned int disable_delay;	/* Dummy delay */
 
 /*
@@ -135,6 +120,21 @@ struct imx_i2c_struct {
 	unsigned long		i2csr;
 };
 
+/*
+ * i.MX has 32-bit registers, i.MX31 16-bit, but only 8 bits are used. So far
+ * 8-bit access seems to work on both.
+ */
+static inline unsigned int reg_read(const void __iomem *p)
+{
+	return __raw_readb(p);
+}
+
+static inline void reg_write(unsigned int value, void __iomem *p)
+{
+	__raw_writeb(value, p);
+}
+
+
 /** Functions for IMX I2C adapter driver ***************************************
 *******************************************************************************/
 
@@ -145,7 +145,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
 	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
 
 	/* wait for bus not busy */
-	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
+	while (reg_read(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
 		if (signal_pending(current)) {
 			dev_dbg(&i2c_imx->adapter.dev,
 				"<%s> I2C Interrupted\n", __func__);
@@ -167,7 +167,7 @@ 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, I2C_IMX_TIME_TRX * HZ);
+		i2c_imx->i2csr & I2SR_IIF, i2c_imx->adapter.timeout);
 
 	if (unlikely(result < 0)) {
 		dev_dbg(&i2c_imx->adapter.dev, "<%s> result < 0\n", __func__);
@@ -183,62 +183,50 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
 
 static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
 {
-	unsigned long orig_jiffies = jiffies;
-
-	/* Wait for ack */
-	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
-		if (signal_pending(current)) {
-			dev_dbg(&i2c_imx->adapter.dev,
-				"<%s> I2C Interrupted\n", __func__);
-			return -EINTR;
-		}
-		if (time_after(jiffies, orig_jiffies + HZ)) {
-			dev_dbg(&i2c_imx->adapter.dev,
-				"<%s> No ACK\n", __func__);
-			return -EIO;
-		}
-		schedule();
+	if (reg_read(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;   /* No ACK */
+	return 0;
 }
 
 static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
 {
-	unsigned int temp = 0;
+	unsigned int temp;
 
 	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
 
 	/* Enable I2C controller */
-	writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
+	reg_write(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
 	/* Start I2C transaction */
-	temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+	temp = reg_read(i2c_imx->base + IMX_I2C_I2CR);
 	temp |= I2CR_MSTA;
-	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+	reg_write(temp, i2c_imx->base + IMX_I2C_I2CR);
 	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
-	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+	reg_write(temp, i2c_imx->base + IMX_I2C_I2CR);
 }
 
 static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
 {
-	unsigned int temp = 0;
+	unsigned int temp;
 
 	/* Stop I2C transaction */
 	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
-	temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+	temp = reg_read(i2c_imx->base + IMX_I2C_I2CR);
 	temp &= ~I2CR_MSTA;
-	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+	reg_write(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);
+	reg_write(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
+	reg_write(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);
+	reg_write(0, i2c_imx->base + IMX_I2C_I2CR);
 }
 
 static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
@@ -259,7 +247,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
 		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);
+	reg_write(i2c_clk_div[i][1], i2c_imx->base + IMX_I2C_IFDR);
 
 	/*
 	 * There dummy delay is calculated.
@@ -284,12 +272,12 @@ 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);
+	temp = reg_read(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);
+		reg_write(temp, i2c_imx->base + IMX_I2C_I2SR);
 		wake_up_interruptible(&i2c_imx->queue);
 	}
 
@@ -299,12 +287,15 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
 static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 {
 	int i, result;
+	unsigned int addr_trans;
 
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
 		__func__, msgs->addr);
 
+	addr_trans = msgs->addr << 1;
+
 	/* write slave address */
-	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);
+	reg_write(addr_trans, i2c_imx->base + IMX_I2C_I2DR);
 	result = i2c_imx_trx_complete(i2c_imx);
 	if (result)
 		return result;
@@ -318,7 +309,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 		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);
+		reg_write(msgs->buf[i], i2c_imx->base + IMX_I2C_I2DR);
 		result = i2c_imx_trx_complete(i2c_imx);
 		if (result)
 			return result;
@@ -333,13 +324,16 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 {
 	int i, result;
 	unsigned int temp;
+	unsigned int addr_trans;
 
 	dev_dbg(&i2c_imx->adapter.dev,
 		"<%s> write slave address: addr=0x%x\n",
 		__func__, msgs->addr | 0x01);
 
+	addr_trans = (msgs->addr << 1) | 0x01;
+
 	/* write slave address */
-	writeb(msgs->addr | 0x01, i2c_imx->base + IMX_I2C_I2DR);
+	reg_write(addr_trans, i2c_imx->base + IMX_I2C_I2DR);
 	result = i2c_imx_trx_complete(i2c_imx);
 	if (result)
 		return result;
@@ -350,12 +344,12 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 	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 = reg_read(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 */
+	reg_write(temp, i2c_imx->base + IMX_I2C_I2CR);
+	reg_read(i2c_imx->base + IMX_I2C_I2DR); /* dummy read */
 
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__);
 
@@ -367,17 +361,17 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 		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 = reg_read(i2c_imx->base + IMX_I2C_I2CR);
 			temp &= ~I2CR_MSTA;
-			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+			reg_write(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 = reg_read(i2c_imx->base + IMX_I2C_I2CR);
 			temp |= I2CR_TXAK;
-			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+			reg_write(temp, i2c_imx->base + IMX_I2C_I2CR);
 		}
-		msgs->buf[i] = readb(i2c_imx->base + IMX_I2C_I2DR);
+		msgs->buf[i] = reg_read(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]);
@@ -407,21 +401,21 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 		if (i) {
 			dev_dbg(&i2c_imx->adapter.dev,
 				"<%s> repeated start\n", __func__);
-			temp = readb(i2c_imx->base + IMX_I2C_I2CR);
+			temp = reg_read(i2c_imx->base + IMX_I2C_I2CR);
 			temp |= I2CR_RSTA;
-			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
+			reg_write(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);
+		temp = reg_read(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);
+		temp = reg_read(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__,
@@ -442,8 +436,8 @@ fail0:
 
 	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;
+			result < 0 ? result : num);
+	return result < 0 ? result : num;
 }
 
 static u32 i2c_imx_func(struct i2c_adapter *adapter)
@@ -518,12 +512,14 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
 	i2c_imx->adapter.algo		= &i2c_imx_algo;
 	i2c_imx->adapter.dev.parent	= &pdev->dev;
 	i2c_imx->adapter.nr 		= pdev->id;
+	i2c_imx->adapter.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;
+	i2c_imx->adapter.timeout	= 1;
 	i2c_imx->irq			= irq;
 	i2c_imx->base			= base;
 	i2c_imx->res			= res;
 
 	/* Get I2C clock */
-	i2c_imx->clk = clk_get(NULL, "i2c_clk");
+	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");
@@ -546,11 +542,11 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
 	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
 
 	/* Set up clock divider */
-	i2c_imx_set_clk(i2c_imx, clkfreq);
+	i2c_imx_set_clk(i2c_imx, pdata->i2c_clk);
 
 	/* Set up chip registers to defaults */
-	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
-	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
+	reg_write(0, i2c_imx->base + IMX_I2C_I2CR);
+	reg_write(0, i2c_imx->base + IMX_I2C_I2SR);
 
 	/* Add I2C adapter */
 	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
@@ -604,10 +600,10 @@ static int __exit i2c_imx_remove(struct platform_device *pdev)
 	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);
+	reg_write(0, i2c_imx->base + IMX_I2C_IADR);
+	reg_write(0, i2c_imx->base + IMX_I2C_IFDR);
+	reg_write(0, i2c_imx->base + IMX_I2C_I2CR);
+	reg_write(0, i2c_imx->base + IMX_I2C_I2SR);
 
 	/* Shut down hardware */
 	if (pdata->exit)
@@ -647,9 +643,6 @@ static void __exit i2c_adap_imx_exit(void)
 module_init(i2c_adap_imx_init);
 module_exit(i2c_adap_imx_exit);
 
-module_param(clkfreq, uint, S_IRUGO);
-MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");
-
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Darius Augulis");
 MODULE_DESCRIPTION("I2C adapter driver for IMX I2C bus");
-- 
1.5.4

Re: [PATCH][resend] iMX/MXC support for I2C

[Guennadi Liakhovetski]

On Wed, 3 Dec 2008, Darius wrote:

> Guennadi Liakhovetski wrote:
> > > +
> > > +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, I2C_IMX_TIME_TRX * HZ);
> > 
> > 5s is much too long!
> 
> how much? 600us?

mxc uses 1 jiffie.

> > > +	/* write slave address */
> > > +	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);
> > 
> > This is wrong! You have to double the address before writing to the
> > register.
> 
> strange! there are I2c board data in my MXLADS code:
> 
> struct i2c_board_info __initdata mx1ads_i2c_devices[] = {
> 	{
> 		I2C_BOARD_INFO("ov7xxx", 0x42),
> 		.platform_data = &iclink[0],
> 	}, {
> 		I2C_BOARD_INFO("mt9v111", 0x90),
> 		.platform_data = &iclink[0],
> 	}
> }
> 
> slave addresses are exactly 0x42 and 0x90 (from datasheets).
> my driver works with these devices with address not doubled.
> I saw this in other I2C drivers, but If I double address in my driver, it
> works wrong.
> I tested this with oscilloscope - now it works ok, with all devices I have
> tryed.

As Mark explained - Linux uses i2c addresses without the read/write bit, 
i.e., shifted one bit right.

> > > +module_param(clkfreq, uint, S_IRUGO);
> > > +MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");
> > 
> > Making clkfreq a module parameter you force the same frequency on all i2c
> > busses. On my i.MX31 system 2 busses are internal and one goes to a
> > connector, to which a camera is connected. This third bus can only handle a
> > lower frequency, which, however, doesn't mean we also have to throttle the
> > other two busses. Can we put this into platform data?
> 
> We can do that, but now there is possibility to change bitrate when re-loading
> module.
> What is better?

But is it really necessary to be able to override this at load-time? At 
least not as one single parameter. If you absolutely need this 
possibility, maybe an array of frequencies? But then you don't know how 
many busses you are going to have. Having an array of 8 ints will probably 
be enough for a while:-)

> > > +struct imxi2c_platform_data {
> > > +	int (*init)(struct device *dev);
> > > +	int (*exit)(struct device *dev);
> > 
> > What are you going to use .exit() for? Is it really needed? Even if it is,
> > it can easily return void I guess?
> 
> .init is used to request and setup gpio pins, .exit used to free gpio.
> yes, .exit can return void - I will fix it.

You mean in your .init() you not only configure iomux pins for i2c, you 
also gpio_request(pin_to_gpio(), "i2c")? Now that I think about this, 
maybe this is indeed correct, and then you gpio_free() in .exit()... Is 
this what you mean?

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Ben Dooks]

On Wed, Dec 03, 2008 at 12:17:58PM +0100, Guennadi Liakhovetski wrote:
> The good news is - I was able to get this driver running on i.MX31 - 
> somewhat... Now the comments:
> 
> On Tue, 2 Dec 2008, Darius wrote:
> 
> Commenting would have been easier, if you had inlined your patch...
> 
> > From augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org Tue Dec  2 15:15:21 2008
> > Date: Tue, 02 Dec 2008 16:00:20 +0200
> > From: Darius <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW@public.gmane.org
> > Subject: [PATCH][resend] iMX/MXC support for I2C
> > 
> > 
> >     [ Empty or malformed message. Use "H" to see raw text. ]
> > 
> > 
> >     [ Part 2: "Attached Text" ]
> > 
> > From: Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > 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>
> > 
> > Index: linux-2.6.28-rc6/drivers/i2c/busses/i2c-imx.c
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6.28-rc6/drivers/i2c/busses/i2c-imx.c
> > @@ -0,0 +1,656 @@
> > +/*
> > + *	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
> > + *
> > + *	module parameters:
> > + *		- clkfreq:
> > + *			Sets the desired clock rate
> > + *			The default value is 100000
> > + *			Max value is 400000
> > + *
> > + *	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 values of module parameters */
> > +#define IMX_I2C_BIT_RATE	100000	/* 100kHz */
> > +
> > +/* Timeouts */
> > +#define I2C_IMX_TIME_BUSY	2000	/* loop count */
> > +#define I2C_IMX_TIME_ACK	2000	/* loop count */
> > +#define I2C_IMX_TIME_TRX	5	/* seconds */
> > +
> > +/* 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 clkfreq = IMX_I2C_BIT_RATE;
> > +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) {
> 
> The i2c-mxc driver I ported over from Freescale sources uses 16-bit 
> access, and the datasheet defines registers to be 16-bit wide, although 
> only low 8 bits are used. Whereas an i.MX datasheet (MC9328MX1) I have 
> here defines i2c registers to be 32-bit wide.
> 
> > +		if (signal_pending(current)) {
> > +			dev_dbg(&i2c_imx->adapter.dev,
> > +				"<%s> I2C Interrupted\n", __func__);
> > +			return -EINTR;
> > +		}
> > +		if (time_after(jiffies, orig_jiffies + HZ)) {
> > +			dev_dbg(&i2c_imx->adapter.dev,
> > +				"<%s> I2C bus is busy\n", __func__);
> > +			return -EIO;
> > +		}
> > +		schedule();

is there a function that would do the equiv of
wait_event_interruptible?
 
> For comparison: the MXC driver only waits for bus busy to clear for 600us 
> at most.
> 
> > +	}
> > +
> > +	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, I2C_IMX_TIME_TRX * HZ);
> 
> 5s is much too long!

Hmm, I would tend to agree, a half-second would probably be better
unless you know you are really going to pushing a lot of data over
a low speed i2c bus.
 
> > +
> > +	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)
> > +{
> > +	unsigned long orig_jiffies = jiffies;
> > +
> > +	/* Wait for ack */
> > +	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
> > +		if (signal_pending(current)) {
> > +			dev_dbg(&i2c_imx->adapter.dev,
> > +				"<%s> I2C Interrupted\n", __func__);
> > +			return -EINTR;
> > +		}
> > +		if (time_after(jiffies, orig_jiffies + HZ)) {
> > +			dev_dbg(&i2c_imx->adapter.dev,
> > +				"<%s> No ACK\n", __func__);
> > +			return -EIO;
> > +		}
> > +		schedule();
> > +	}
> 
> 1s is way too long here! i2cdetect takes ages to complete. The mxc driver 
> doesn't wait here at all - as soon as a tx is complete, it checks the ack 
> status. Could you verify if this would work on your MX1 too?

That does seem a problem, if we can detect a NAK immediately then all
the better.
 
> > +
> > +	dev_dbg(&i2c_imx->adapter.dev, "<%s> ACK received\n", __func__);
> > +	return 0;   /* No ACK */
> > +}
> > +
> > +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

not using dev_xxx() here?

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

it would be nice if we returned IRQ_NONE if we didn't have anything
to handle.

> > +
> > +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);
> > +
> > +	/* write slave address */
> > +	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);
> 
> This is wrong! You have to double the address before writing to the 
> register.

do you have a reference to the datasheet?
 
> > +	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 | 0x01);
> > +
> > +	/* write slave address */
> > +	writeb(msgs->addr | 0x01, i2c_imx->base + IMX_I2C_I2DR);
> 
> Same here - (msgs->addr << 1) | 0x01.

good catch, missed that myself.
 
> > +	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;
> > +	int irq;
> > +	int res_size;
> > +	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;
> > +	}
> > +
> > +	res_size = res->end - res->start + 1;
> 
> You can use resource_size()
> 
> > +	if (!request_mem_region(res->start, res_size, res->name)) {
> > +		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
> > +			res_size, res->start);
> > +		return -ENOMEM;
> > +	}
> 
> I was once told, one doesn't need request_mem_region() on regions from 
> platform data resources - this is already done implicitly.

I belive that request_mem_region() is the correct thing to do, IIRC,
the platform_device_register() calls insert_resource() on all the
resource regions of the device so that it is guarnateed to appear in
the /proc/ioport or /proc/iomem map. However I belive this does not
actually make a reservation of that resource.
 
> > +
> > +	if (pdata->init) {
> > +		ret = pdata->init(&pdev->dev);
> > +		if (ret)
> > +			goto fail0;
> > +	}
> > +
> > +	base = ioremap(res->start, res_size);
> > +	if (!base) {
> > +		dev_err(&pdev->dev, "ioremap failed\n");
> > +		ret = -EIO;
> > +		goto fail1;
> > +	}
> > +
> > +	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 fail2;
> > +	}
> > +
> > +	/* 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(NULL, "i2c_clk");
> 
> There can be several i2c busses on the system, so you want:
> 
> +	i2c_imx->clk = clk_get(&pdev->dev, "i2c_clk");

tbh, the bus clock should be defined as NULL, any extra clocks for
a device get names. However this may need fixing in the platform
as well. I belive RMK is already making these changes for AMBA and
I will probably follow suit with the s3c architectures as soon as
the rest of the development work is out of the way.
 
> > +	if (IS_ERR(i2c_imx->clk)) {
> > +		ret = PTR_ERR(i2c_imx->clk);
> > +		dev_err(&pdev->dev, "can't get I2C clock\n");
> > +		goto fail3;
> > +	}
> > +	clk_enable(i2c_imx->clk);
> > +
> > +	/* Request IRQ */
> > +	ret = request_irq(i2c_imx->irq, i2c_imx_isr,
> > +		IRQF_DISABLED, pdev->name, i2c_imx);

do you really need to run your i2c isr with interrupts disabled?

> > +	if (ret) {
> > +		dev_err(&pdev->dev, "can't claim irq %d\n", i2c_imx->irq);
> > +		goto fail4;
> > +	}
> > +
> > +	/* Init queue */
> > +	init_waitqueue_head(&i2c_imx->queue);
> > +
> > +	/* Set up adapter data */
> > +	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
> > +
> > +	/* Set up clock divider */
> > +	i2c_imx_set_clk(i2c_imx, clkfreq);
> > +
> > +	/* 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 fail5;
> > +	}
> > +
> > +	/* 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 */
> > +
> > +fail5:
> > +	free_irq(i2c_imx->irq, i2c_imx);
> > +fail4:
> > +	clk_disable(i2c_imx->clk);
> > +	clk_put(i2c_imx->clk);
> > +fail3:
> > +	kfree(i2c_imx);
> > +fail2:
> > +	iounmap(base);
> > +fail1:
> > +	if (pdata->exit)
> > +		pdata->exit(&pdev->dev);
> > +fail0:
> > +	release_mem_region(res->start, res_size);
> > +	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_param(clkfreq, uint, S_IRUGO);
> > +MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");
> 
> Making clkfreq a module parameter you force the same frequency on all i2c 
> busses. On my i.MX31 system 2 busses are internal and one goes to a 
> connector, to which a camera is connected. This third bus can only handle 
> a lower frequency, which, however, doesn't mean we also have to throttle 
> the other two busses. Can we put this into platform data?
> 
> > +
> > +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.28-rc6/drivers/i2c/busses/Kconfig
> > ===================================================================
> > --- linux-2.6.28-rc6.orig/drivers/i2c/busses/Kconfig
> > +++ linux-2.6.28-rc6/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.28-rc6/drivers/i2c/busses/Makefile
> > ===================================================================
> > --- linux-2.6.28-rc6.orig/drivers/i2c/busses/Makefile
> > +++ linux-2.6.28-rc6/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.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
> > @@ -0,0 +1,17 @@
> > +/*
> > + * i2c.h - i.MX I2C driver header file
> > + *
> > + * Copyright (c) 2008, Darius Augulis <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __ASM_ARCH_I2C_H_
> > +#define __ASM_ARCH_I2C_H_
> > +

kernel doc comments would be nice for this to actually say what is
expected of the implementation and what these calls are there for.

> > +struct imxi2c_platform_data {
> > +	int (*init)(struct device *dev);
> > +	int (*exit)(struct device *dev);
> 
> What are you going to use .exit() for? Is it really needed? Even if it is, 
> it can easily return void I guess?
> 
> > +};
> > +
> > +#endif /* __ASM_ARCH_I2C_H_ */
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

  'a smiley only costs 4 bytes'

Re: [PATCH][resend] iMX/MXC support for I2C

[Ben Dooks]

On Wed, Dec 03, 2008 at 10:19:32PM +0100, Guennadi Liakhovetski wrote:
> On Wed, 3 Dec 2008, Darius wrote:
> 
> > Guennadi Liakhovetski wrote:
> > > > +
> > > > +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, I2C_IMX_TIME_TRX * HZ);
> > > 
> > > 5s is much too long!
> > 
> > how much? 600us?
> 
> mxc uses 1 jiffie.
> 
> > > > +	/* write slave address */
> > > > +	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);
> > > 
> > > This is wrong! You have to double the address before writing to the
> > > register.
> > 
> > strange! there are I2c board data in my MXLADS code:
> > 
> > struct i2c_board_info __initdata mx1ads_i2c_devices[] = {
> > 	{
> > 		I2C_BOARD_INFO("ov7xxx", 0x42),
> > 		.platform_data = &iclink[0],
> > 	}, {
> > 		I2C_BOARD_INFO("mt9v111", 0x90),
> > 		.platform_data = &iclink[0],
> > 	}
> > }
> > 
> > slave addresses are exactly 0x42 and 0x90 (from datasheets).
> > my driver works with these devices with address not doubled.
> > I saw this in other I2C drivers, but If I double address in my driver, it
> > works wrong.
> > I tested this with oscilloscope - now it works ok, with all devices I have
> > tryed.
> 
> As Mark explained - Linux uses i2c addresses without the read/write bit, 
> i.e., shifted one bit right.
> 
> > > > +module_param(clkfreq, uint, S_IRUGO);
> > > > +MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");
> > > 
> > > Making clkfreq a module parameter you force the same frequency on all i2c
> > > busses. On my i.MX31 system 2 busses are internal and one goes to a
> > > connector, to which a camera is connected. This third bus can only handle a
> > > lower frequency, which, however, doesn't mean we also have to throttle the
> > > other two busses. Can we put this into platform data?
> > 
> > We can do that, but now there is possibility to change bitrate when re-loading
> > module.
> > What is better?
> 
> But is it really necessary to be able to override this at load-time? At 
> least not as one single parameter. If you absolutely need this 
> possibility, maybe an array of frequencies? But then you don't know how 
> many busses you are going to have. Having an array of 8 ints will probably 
> be enough for a while:-)

Platform data seems a good way of getting a board to declare a bus
rate. Otherwise a standard i2c command line option for all busses to
use may be the best thing to do.
 
> > > > +struct imxi2c_platform_data {
> > > > +	int (*init)(struct device *dev);
> > > > +	int (*exit)(struct device *dev);
> > > 
> > > What are you going to use .exit() for? Is it really needed? Even if it is,
> > > it can easily return void I guess?
> > 
> > .init is used to request and setup gpio pins, .exit used to free gpio.
> > yes, .exit can return void - I will fix it.
> 
> You mean in your .init() you not only configure iomux pins for i2c, you 
> also gpio_request(pin_to_gpio(), "i2c")? Now that I think about this, 
> maybe this is indeed correct, and then you gpio_free() in .exit()... Is 
> this what you mean?

What does David Brownell think of that?

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

  'a smiley only costs 4 bytes'

Re: [PATCH][resend] iMX/MXC support for I2C

[Ben Dooks]

On Wed, Dec 03, 2008 at 09:58:10PM +0100, Guennadi Liakhovetski wrote:
> Hi Darius,
> 
> it would be better, if you didn't drop my email from cc, thanks.
> 
> On Wed, 3 Dec 2008, Darius wrote:
> 
> > Guennadi Liakhovetski wrote:
> > > One more thing: you also want to do something like this
> > > 
> > > 	i2c_imx->adapter.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;
> > 
> > some time ago, it was I2C_CLASS_ALL, but Jean told, that it will be removed at
> > all :)
> > 
> > > 
> > > in i2c_imx_probe().
> > > 
> > > If you want, I can send you my current version of your driver, so you can
> > > compare.
> > 
> > yes, please send it.
> 
> Below.
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> 
> >From 1d592e3a94b3e13537a680e3e0198c345248692d Mon Sep 17 00:00:00 2001
> From: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> Date: Wed, 3 Dec 2008 17:02:37 +0100
> Subject: [PATCH] i2x-imx: fix client addresses, remove needless delays, support multiple busses
> 
> With above bugs fixed now also tested to work on i.MX31. bus frequency is now
> configured over platform data, which allows different frequencies on different
> busses. Add an adapter.class assignment.
> 
> Signed-off-by: Guennadi Liakhovetski <lg-ynQEQJNshbs@public.gmane.org>
> ---
>  arch/arm/plat-mxc/include/mach/i2c.h |    8 ++
>  drivers/i2c/busses/i2c-imx.c         |  143 ++++++++++++++++------------------
>  2 files changed, 76 insertions(+), 75 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/include/mach/i2c.h b/arch/arm/plat-mxc/include/mach/i2c.h
> index 9e6a9d8..37d068b 100644
> --- a/arch/arm/plat-mxc/include/mach/i2c.h
> +++ b/arch/arm/plat-mxc/include/mach/i2c.h
> @@ -10,8 +10,16 @@
>  #define __ASM_ARCH_I2C_H_
>  
>  struct imxi2c_platform_data {
> +	u32 i2c_clk;
>  	int (*init)(struct device *dev);
>  	int (*exit)(struct device *dev);
>  };
>  
> +struct imxi2c_board_data {
> +	struct imxi2c_platform_data	data;
> +	unsigned int			id;
> +};
> +
> +extern int mxc_i2c_register_adapters(struct imxi2c_board_data *data, int n);

if there are only 1 or two busses, why not do

mx2_i2c_register_board(int i2c_channel_nr, struct
		       imx_i2c_platform_data *pd);

>  #endif /* __ASM_ARCH_I2C_H_ */
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index fd2d16d..eeea7d7 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -23,12 +23,6 @@
>   *	Implementation of I2C Adapter/Algorithm Driver
>   *	for I2C Bus integrated in Freescale i.MX/MXC processors
>   *
> - *	module parameters:
> - *		- clkfreq:
> - *			Sets the desired clock rate
> - *			The default value is 100000
> - *			Max value is 400000
> - *
>   *	Derived from Motorola GSG China I2C example driver
>   *
>   *	Copyright (C) 2005 Torsten Koschorrek <koschorrek at synertronixx.de
> @@ -64,14 +58,6 @@
>  /* This will be the driver name the kernel reports */
>  #define DRIVER_NAME "imx-i2c"
>  
> -/* Default values of module parameters */
> -#define IMX_I2C_BIT_RATE	100000	/* 100kHz */
> -
> -/* Timeouts */
> -#define I2C_IMX_TIME_BUSY	2000	/* loop count */
> -#define I2C_IMX_TIME_ACK	2000	/* loop count */
> -#define I2C_IMX_TIME_TRX	5	/* seconds */
> -
>  /* IMX I2C registers */
>  #define IMX_I2C_IADR	0x00	/* i2c slave address */
>  #define IMX_I2C_IFDR	0x04	/* i2c frequency divider */
> @@ -97,7 +83,6 @@
>  /** Variables ******************************************************************
>  *******************************************************************************/
>  
> -static unsigned int clkfreq = IMX_I2C_BIT_RATE;
>  static unsigned int disable_delay;	/* Dummy delay */
>  
>  /*
> @@ -135,6 +120,21 @@ struct imx_i2c_struct {
>  	unsigned long		i2csr;
>  };
>  
> +/*
> + * i.MX has 32-bit registers, i.MX31 16-bit, but only 8 bits are used. So far
> + * 8-bit access seems to work on both.
> + */
> +static inline unsigned int reg_read(const void __iomem *p)
> +{
> +	return __raw_readb(p);
> +}
> +
> +static inline void reg_write(unsigned int value, void __iomem *p)
> +{
> +	__raw_writeb(value, p);
> +}

readb and writeb() are the right verions to use here.

> +
> +
>  /** Functions for IMX I2C adapter driver ***************************************
>  *******************************************************************************/
>  
> @@ -145,7 +145,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>  
>  	/* wait for bus not busy */
> -	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
> +	while (reg_read(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
>  		if (signal_pending(current)) {
>  			dev_dbg(&i2c_imx->adapter.dev,
>  				"<%s> I2C Interrupted\n", __func__);
> @@ -167,7 +167,7 @@ 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, I2C_IMX_TIME_TRX * HZ);
> +		i2c_imx->i2csr & I2SR_IIF, i2c_imx->adapter.timeout);
>  
>  	if (unlikely(result < 0)) {
>  		dev_dbg(&i2c_imx->adapter.dev, "<%s> result < 0\n", __func__);
> @@ -183,62 +183,50 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
>  
>  static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
>  {
> -	unsigned long orig_jiffies = jiffies;
> -
> -	/* Wait for ack */
> -	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
> -		if (signal_pending(current)) {
> -			dev_dbg(&i2c_imx->adapter.dev,
> -				"<%s> I2C Interrupted\n", __func__);
> -			return -EINTR;
> -		}
> -		if (time_after(jiffies, orig_jiffies + HZ)) {
> -			dev_dbg(&i2c_imx->adapter.dev,
> -				"<%s> No ACK\n", __func__);
> -			return -EIO;
> -		}
> -		schedule();
> +	if (reg_read(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;   /* No ACK */
> +	return 0;
>  }
>  
>  static void i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>  {
> -	unsigned int temp = 0;
> +	unsigned int temp;
>  
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>  
>  	/* Enable I2C controller */
> -	writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> +	reg_write(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
>  	/* Start I2C transaction */
> -	temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> +	temp = reg_read(i2c_imx->base + IMX_I2C_I2CR);
>  	temp |= I2CR_MSTA;
> -	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> +	reg_write(temp, i2c_imx->base + IMX_I2C_I2CR);
>  	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> -	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> +	reg_write(temp, i2c_imx->base + IMX_I2C_I2CR);
>  }
>  
>  static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
>  {
> -	unsigned int temp = 0;
> +	unsigned int temp;
>  
>  	/* Stop I2C transaction */
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> -	temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> +	temp = reg_read(i2c_imx->base + IMX_I2C_I2CR);
>  	temp &= ~I2CR_MSTA;
> -	writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> +	reg_write(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);
> +	reg_write(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> +	reg_write(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);
> +	reg_write(0, i2c_imx->base + IMX_I2C_I2CR);
>  }
>  
>  static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
> @@ -259,7 +247,7 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
>  		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);
> +	reg_write(i2c_clk_div[i][1], i2c_imx->base + IMX_I2C_IFDR);
>  
>  	/*
>  	 * There dummy delay is calculated.
> @@ -284,12 +272,12 @@ 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);
> +	temp = reg_read(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);
> +		reg_write(temp, i2c_imx->base + IMX_I2C_I2SR);
>  		wake_up_interruptible(&i2c_imx->queue);
>  	}
>  
> @@ -299,12 +287,15 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>  static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
>  {
>  	int i, result;
> +	unsigned int addr_trans;
>  
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
>  		__func__, msgs->addr);
>  
> +	addr_trans = msgs->addr << 1;
> +
>  	/* write slave address */
> -	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);
> +	reg_write(addr_trans, i2c_imx->base + IMX_I2C_I2DR);
>  	result = i2c_imx_trx_complete(i2c_imx);
>  	if (result)
>  		return result;
> @@ -318,7 +309,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
>  		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);
> +		reg_write(msgs->buf[i], i2c_imx->base + IMX_I2C_I2DR);
>  		result = i2c_imx_trx_complete(i2c_imx);
>  		if (result)
>  			return result;
> @@ -333,13 +324,16 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
>  {
>  	int i, result;
>  	unsigned int temp;
> +	unsigned int addr_trans;
>  
>  	dev_dbg(&i2c_imx->adapter.dev,
>  		"<%s> write slave address: addr=0x%x\n",
>  		__func__, msgs->addr | 0x01);
>  
> +	addr_trans = (msgs->addr << 1) | 0x01;
> +
>  	/* write slave address */
> -	writeb(msgs->addr | 0x01, i2c_imx->base + IMX_I2C_I2DR);
> +	reg_write(addr_trans, i2c_imx->base + IMX_I2C_I2DR);
>  	result = i2c_imx_trx_complete(i2c_imx);
>  	if (result)
>  		return result;
> @@ -350,12 +344,12 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
>  	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 = reg_read(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 */
> +	reg_write(temp, i2c_imx->base + IMX_I2C_I2CR);
> +	reg_read(i2c_imx->base + IMX_I2C_I2DR); /* dummy read */
>  
>  	dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__);
>  
> @@ -367,17 +361,17 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
>  		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 = reg_read(i2c_imx->base + IMX_I2C_I2CR);
>  			temp &= ~I2CR_MSTA;
> -			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> +			reg_write(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 = reg_read(i2c_imx->base + IMX_I2C_I2CR);
>  			temp |= I2CR_TXAK;
> -			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> +			reg_write(temp, i2c_imx->base + IMX_I2C_I2CR);
>  		}
> -		msgs->buf[i] = readb(i2c_imx->base + IMX_I2C_I2DR);
> +		msgs->buf[i] = reg_read(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]);
> @@ -407,21 +401,21 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>  		if (i) {
>  			dev_dbg(&i2c_imx->adapter.dev,
>  				"<%s> repeated start\n", __func__);
> -			temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> +			temp = reg_read(i2c_imx->base + IMX_I2C_I2CR);
>  			temp |= I2CR_RSTA;
> -			writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> +			reg_write(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);
> +		temp = reg_read(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);
> +		temp = reg_read(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__,
> @@ -442,8 +436,8 @@ fail0:
>  
>  	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;
> +			result < 0 ? result : num);
> +	return result < 0 ? result : num;
>  }
>  
>  static u32 i2c_imx_func(struct i2c_adapter *adapter)
> @@ -518,12 +512,14 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
>  	i2c_imx->adapter.algo		= &i2c_imx_algo;
>  	i2c_imx->adapter.dev.parent	= &pdev->dev;
>  	i2c_imx->adapter.nr 		= pdev->id;
> +	i2c_imx->adapter.class		= I2C_CLASS_HWMON | I2C_CLASS_SPD;
> +	i2c_imx->adapter.timeout	= 1;
>  	i2c_imx->irq			= irq;
>  	i2c_imx->base			= base;
>  	i2c_imx->res			= res;
>  
>  	/* Get I2C clock */
> -	i2c_imx->clk = clk_get(NULL, "i2c_clk");
> +	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");
> @@ -546,11 +542,11 @@ static int __init i2c_imx_probe(struct platform_device *pdev)
>  	i2c_set_adapdata(&i2c_imx->adapter, i2c_imx);
>  
>  	/* Set up clock divider */
> -	i2c_imx_set_clk(i2c_imx, clkfreq);
> +	i2c_imx_set_clk(i2c_imx, pdata->i2c_clk);
>  
>  	/* Set up chip registers to defaults */
> -	writeb(0, i2c_imx->base + IMX_I2C_I2CR);
> -	writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> +	reg_write(0, i2c_imx->base + IMX_I2C_I2CR);
> +	reg_write(0, i2c_imx->base + IMX_I2C_I2SR);
>  
>  	/* Add I2C adapter */
>  	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
> @@ -604,10 +600,10 @@ static int __exit i2c_imx_remove(struct platform_device *pdev)
>  	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);
> +	reg_write(0, i2c_imx->base + IMX_I2C_IADR);
> +	reg_write(0, i2c_imx->base + IMX_I2C_IFDR);
> +	reg_write(0, i2c_imx->base + IMX_I2C_I2CR);
> +	reg_write(0, i2c_imx->base + IMX_I2C_I2SR);
>  
>  	/* Shut down hardware */
>  	if (pdata->exit)
> @@ -647,9 +643,6 @@ static void __exit i2c_adap_imx_exit(void)
>  module_init(i2c_adap_imx_init);
>  module_exit(i2c_adap_imx_exit);
>  
> -module_param(clkfreq, uint, S_IRUGO);
> -MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");
> -
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Darius Augulis");
>  MODULE_DESCRIPTION("I2C adapter driver for IMX I2C bus");
> -- 
> 1.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

  'a smiley only costs 4 bytes'

Re: [PATCH][resend] iMX/MXC support for I2C

[Guennadi Liakhovetski]

On Wed, 3 Dec 2008, Ben Dooks wrote:

> On Wed, Dec 03, 2008 at 09:58:10PM +0100, Guennadi Liakhovetski wrote:
> >  
> >  struct imxi2c_platform_data {
> > +	u32 i2c_clk;
> >  	int (*init)(struct device *dev);
> >  	int (*exit)(struct device *dev);
> >  };
> >  
> > +struct imxi2c_board_data {
> > +	struct imxi2c_platform_data	data;
> > +	unsigned int			id;
> > +};
> > +
> > +extern int mxc_i2c_register_adapters(struct imxi2c_board_data *data, int n);
> 
> if there are only 1 or two busses, why not do
> 
> mx2_i2c_register_board(int i2c_channel_nr, struct
> 		       imx_i2c_platform_data *pd);

on i.MX31 there are 3.

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Darius]

Guennadi Liakhovetski wrote:
> On Wed, 3 Dec 2008, Darius wrote:
>
>   
>> Guennadi Liakhovetski wrote:
>>     
>>>> +
>>>> +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, I2C_IMX_TIME_TRX * HZ);
>>>>         
>>> 5s is much too long!
>>>       
>> how much? 600us?
>>     
>
> mxc uses 1 jiffie.
>
>   
>>>> +	/* write slave address */
>>>> +	writeb(msgs->addr, i2c_imx->base + IMX_I2C_I2DR);
>>>>         
>>> This is wrong! You have to double the address before writing to the
>>> register.
>>>       
>> strange! there are I2c board data in my MXLADS code:
>>
>> struct i2c_board_info __initdata mx1ads_i2c_devices[] = {
>> 	{
>> 		I2C_BOARD_INFO("ov7xxx", 0x42),
>> 		.platform_data = &iclink[0],
>> 	}, {
>> 		I2C_BOARD_INFO("mt9v111", 0x90),
>> 		.platform_data = &iclink[0],
>> 	}
>> }
>>
>> slave addresses are exactly 0x42 and 0x90 (from datasheets).
>> my driver works with these devices with address not doubled.
>> I saw this in other I2C drivers, but If I double address in my driver, it
>> works wrong.
>> I tested this with oscilloscope - now it works ok, with all devices I have
>> tryed.
>>     
>
> As Mark explained - Linux uses i2c addresses without the read/write bit, 
> i.e., shifted one bit right.
>   

This means I should use shifted address in platform data?
For example: OV7670 image sensor has slave address 0x42 for write, and 
0x43 for read operations.
This is hardware slave address, to which device responds.
But I should use 0x21 in platform data, because it must be doubled in 
adapter driver?
If I use original 0x42 address, after doubling it will be 0x108, to 
which device does not respond...

>   
>>>> +module_param(clkfreq, uint, S_IRUGO);
>>>> +MODULE_PARM_DESC(clkfreq, "desired IMX I2C Clock Rate in Hz");
>>>>         
>>> Making clkfreq a module parameter you force the same frequency on all i2c
>>> busses. On my i.MX31 system 2 busses are internal and one goes to a
>>> connector, to which a camera is connected. This third bus can only handle a
>>> lower frequency, which, however, doesn't mean we also have to throttle the
>>> other two busses. Can we put this into platform data?
>>>       
>> We can do that, but now there is possibility to change bitrate when re-loading
>> module.
>> What is better?
>>     
>
> But is it really necessary to be able to override this at load-time? At 
> least not as one single parameter. If you absolutely need this 
> possibility, maybe an array of frequencies? But then you don't know how 
> many busses you are going to have. Having an array of 8 ints will probably 
> be enough for a while:-)
>   

No it's not necessary. I will leave this setting only in platform data 
and default value if platform data is not provided.

>   
>>>> +struct imxi2c_platform_data {
>>>> +	int (*init)(struct device *dev);
>>>> +	int (*exit)(struct device *dev);
>>>>         
>>> What are you going to use .exit() for? Is it really needed? Even if it is,
>>> it can easily return void I guess?
>>>       
>> .init is used to request and setup gpio pins, .exit used to free gpio.
>> yes, .exit can return void - I will fix it.
>>     
>
> You mean in your .init() you not only configure iomux pins for i2c, you 
> also gpio_request(pin_to_gpio(), "i2c")? Now that I think about this, 
> maybe this is indeed correct, and then you gpio_free() in .exit()... Is 
> this what you mean?
>   

yes, you're right.

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Darius]

>>> +static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx)
>>> +{
>>> +	unsigned long orig_jiffies = jiffies;
>>> +
>>> +	/* Wait for ack */
>>> +	while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) {
>>> +		if (signal_pending(current)) {
>>> +			dev_dbg(&i2c_imx->adapter.dev,
>>> +				"<%s> I2C Interrupted\n", __func__);
>>> +			return -EINTR;
>>> +		}
>>> +		if (time_after(jiffies, orig_jiffies + HZ)) {
>>> +			dev_dbg(&i2c_imx->adapter.dev,
>>> +				"<%s> No ACK\n", __func__);
>>> +			return -EIO;
>>> +		}
>>> +		schedule();
>>> +	}
>> 1s is way too long here! i2cdetect takes ages to complete. The mxc driver 
>> doesn't wait here at all - as soon as a tx is complete, it checks the ack 
>> status. Could you verify if this would work on your MX1 too?
> 
> That does seem a problem, if we can detect a NAK immediately then all
> the better.

I tested on MX1. There delay is not needed at all.

>  
>>> +
>>> +	dev_dbg(&i2c_imx->adapter.dev, "<%s> ACK received\n", __func__);
>>> +	return 0;   /* No ACK */
>>> +}
>>> +
>>> +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
> 
> not using dev_xxx() here?

because adapter is not yet registered.

> 
>>> +}
>>> +
>>> +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;
>>> +}
> 
> it would be nice if we returned IRQ_NONE if we didn't have anything
> to handle.

I will add this.

>>> +	if (!request_mem_region(res->start, res_size, res->name)) {
>>> +		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
>>> +			res_size, res->start);
>>> +		return -ENOMEM;
>>> +	}
>> I was once told, one doesn't need request_mem_region() on regions from 
>> platform data resources - this is already done implicitly.
> 
> I belive that request_mem_region() is the correct thing to do, IIRC,
> the platform_device_register() calls insert_resource() on all the
> resource regions of the device so that it is guarnateed to appear in
> the /proc/ioport or /proc/iomem map. However I belive this does not
> actually make a reservation of that resource.
>  

confused... Guennadi, can you please explain more your mention?

>>> +
>>> +	if (pdata->init) {
>>> +		ret = pdata->init(&pdev->dev);
>>> +		if (ret)
>>> +			goto fail0;
>>> +	}
>>> +
>>> +	base = ioremap(res->start, res_size);
>>> +	if (!base) {
>>> +		dev_err(&pdev->dev, "ioremap failed\n");
>>> +		ret = -EIO;
>>> +		goto fail1;
>>> +	}
>>> +
>>> +	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 fail2;
>>> +	}
>>> +
>>> +	/* 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(NULL, "i2c_clk");
>> There can be several i2c busses on the system, so you want:
>>
>> +	i2c_imx->clk = clk_get(&pdev->dev, "i2c_clk");
> 
> tbh, the bus clock should be defined as NULL, any extra clocks for
> a device get names. However this may need fixing in the platform
> as well. I belive RMK is already making these changes for AMBA and
> I will probably follow suit with the s3c architectures as soon as
> the rest of the development work is out of the way.
>  
>>> +	if (IS_ERR(i2c_imx->clk)) {
>>> +		ret = PTR_ERR(i2c_imx->clk);
>>> +		dev_err(&pdev->dev, "can't get I2C clock\n");
>>> +		goto fail3;
>>> +	}
>>> +	clk_enable(i2c_imx->clk);
>>> +
>>> +	/* Request IRQ */
>>> +	ret = request_irq(i2c_imx->irq, i2c_imx_isr,
>>> +		IRQF_DISABLED, pdev->name, i2c_imx);
> 
> do you really need to run your i2c isr with interrupts disabled?

no, interrupts can be enabled. will fix.


> 
> kernel doc comments would be nice for this to actually say what is
> expected of the implementation and what these calls are there for.
> 
>>> +struct imxi2c_platform_data {
>>> +	int (*init)(struct device *dev);
>>> +	int (*exit)(struct device *dev);
>> What are you going to use .exit() for? Is it really needed? Even if it is, 
>> it can easily return void I guess?
>>

Where to put commnets? Right here in driver code or is better in /Documentation?

Re: [PATCH][resend] iMX/MXC support for I2C

[Guennadi Liakhovetski]

On Thu, 4 Dec 2008, Darius wrote:

> > > > +	if (!request_mem_region(res->start, res_size, res->name)) {
> > > > +		dev_err(&pdev->dev, "can't allocate %d bytes at %d address\n",
> > > > +			res_size, res->start);
> > > > +		return -ENOMEM;
> > > > +	}
> > > I was once told, one doesn't need request_mem_region() on regions from
> > > platform data resources - this is already done implicitly.
> > 
> > I belive that request_mem_region() is the correct thing to do, IIRC,
> > the platform_device_register() calls insert_resource() on all the
> > resource regions of the device so that it is guarnateed to appear in
> > the /proc/ioport or /proc/iomem map. However I belive this does not
> > actually make a reservation of that resource.
> >  
> 
> confused... Guennadi, can you please explain more your mention?

Look at this:

http://marc.info/?l=linux-sh&m=121642007016402&w=2

> > > > +	/* Get I2C clock */
> > > > +	i2c_imx->clk = clk_get(NULL, "i2c_clk");
> > > There can be several i2c busses on the system, so you want:
> > > 
> > > +	i2c_imx->clk = clk_get(&pdev->dev, "i2c_clk");
> > 
> > tbh, the bus clock should be defined as NULL, any extra clocks for
> > a device get names. However this may need fixing in the platform
> > as well. I belive RMK is already making these changes for AMBA and
> > I will probably follow suit with the s3c architectures as soon as
> > the rest of the development work is out of the way.

Ben, what do you mean here? I thought Russell was against 
platform-specific clock names and for function names. But i2c_clk is a 
function name. I don't understand the difference between "bus clock" and 
"extra clocks" - on i.MX31 we have 3 i2c busses with separate clock 
sources, so, we have to use the platform device in clk_get to request a 
specific i2c clock matching the platform-device index. That's all what I 
meant.

> > > > +struct imxi2c_platform_data {
> > > > +	int (*init)(struct device *dev);
> > > > +	int (*exit)(struct device *dev);
> > > What are you going to use .exit() for? Is it really needed? Even if it is,
> > > it can easily return void I guess?
> > > 
> 
> Where to put commnets? Right here in driver code or is better in
> /Documentation?

Something like this:

+/**
+ * @init:	initialise the interface
+ * @exit:	free interface resources
+ */
+struct imxi2c_platform_data {
+	int (*init)(struct device *dev);
+	int (*exit)(struct device *dev);

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Mark Brown]

On Thu, Dec 04, 2008 at 09:47:38AM +0200, Darius wrote:

> This means I should use shifted address in platform data?
> For example: OV7670 image sensor has slave address 0x42 for write, and 
> 0x43 for read operations.
> This is hardware slave address, to which device responds.
> But I should use 0x21 in platform data, because it must be doubled in 
> adapter driver?

Yes, exactly.  Linux does not treat the read/write bit as part of the
address.

Re: [PATCH][resend] iMX/MXC support for I2C

[Guennadi Liakhovetski]

On Thu, 4 Dec 2008, Darius wrote:

> I tested on MX1. There delay is not needed at all.

Darius, how exactly you tested on MX1? I just found out, that also 
i2c_smbus_{read,write}_word_data() doesn't work with imx driver on i.MX31, 
whereas the mxc driver does.

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

Re: [PATCH][resend] iMX/MXC support for I2C

[Darius]

Guennadi Liakhovetski wrote:
> On Thu, 4 Dec 2008, Darius wrote:
> 
>> I tested on MX1. There delay is not needed at all.
> 
> Darius, how exactly you tested on MX1? I just found out, that also 
> i2c_smbus_{read,write}_word_data() doesn't work with imx driver on i.MX31, 
> whereas the mxc driver does.

Hi Guennadi,

I tested with OV7720, OV7670 and MT9V111 drivers. Last one uses smbus data transfers - all works 100% ok.
Today I will post new patch version with most of your and Ben comments fixed.

Darius.

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