From mboxrd@z Thu Jan 1 00:00:00 1970 From: Darius Subject: Re: [PATCH][resend] iMX/MXC support for I2C Date: Wed, 03 Dec 2008 14:52:56 +0200 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW@public.gmane.org List-Id: linux-i2c@vger.kernel.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 >> 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 >> >> 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 >> >> 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 > + * Copyright (C) 2005 Matthias Blaschke > + * Copyright (C) 2007 RightHand Technologies, Inc. >> + * Copyright (C) 2008 Darius Augulis >> + * >> + */ >> + >> +/** Includes ******************************************************************* >> +*******************************************************************************/ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> + >> +/** 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 >> + * >> + * 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