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

[Darius <augulis.darius-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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