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 [EMAIL PROTECTED] Tue Dec  2 15:15:21 2008
Date: Tue, 02 Dec 2008 16:00:20 +0200
From: Darius <[EMAIL PROTECTED]>
To: [email protected]
Cc: [EMAIL PROTECTED]
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 <[EMAIL PROTECTED]>

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 <[EMAIL PROTECTED]>

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 <[EMAIL PROTECTED]>
+ *
+ * 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

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to