iMX/MXC support for I2C
Subsystems (5)
PATCH RESEND 1/1 iMX/MXC support for I2C
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.
drivers/i2c/busses/i2c-imx.c new file +656 -0
@@ -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 );
drivers/i2c/busses/Kconfig +10 -0
@@ -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
drivers/i2c/busses/Makefile +1 -0
@@ -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
arch/arm/plat-mxc/include/mach/i2c.h new file +17 -0
@@ -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); +}; + +#endif /* __ASM_ARCH_I2C_H_ */
Please review and add this for 2.6.29 if it's ok.
Quoted 1 line · Darius
Please review and add this for 2.6.29 if it's ok.
Ben, can you please take care of this?
Thanks,
Signature · 1 line
Jean Delvare
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...
Quoted 164 lines
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: linux-arm-kernel-xIg/[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
===================================================================drivers/i2c/busses/i2c-imx.c new file +138
-0@@ -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 ***************************************
drivers/i2c/busses/i2c-imx.c line 148
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+*******************************************************************************/ + +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.
drivers/i2c/busses/i2c-imx.c line 159
Quoted 11 lines
drivers/i2c/busses/i2c-imx.c new file +11
-0+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.
drivers/i2c/busses/i2c-imx.c line 170
Quoted 11 lines
drivers/i2c/busses/i2c-imx.c new file +11
-0+ } + +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!
Quoted 21 lines
drivers/i2c/busses/i2c-imx.c new file +21
-0+ +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,
drivers/i2c/busses/i2c-imx.c line 201
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+ "<%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?
Quoted 96 lines
drivers/i2c/busses/i2c-imx.c new file +96
-0+ +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 ; +}
drivers/i2c/busses/i2c-imx.c line 307
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+ +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.
Quoted 25 lines
drivers/i2c/busses/i2c-imx.c new file +25
-0+ 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)
drivers/i2c/busses/i2c-imx.c line 342
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+{ +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.
Quoted 136 lines
drivers/i2c/busses/i2c-imx.c new file +136
-0+ 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" );
drivers/i2c/busses/i2c-imx.c line 488
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+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()
drivers/i2c/busses/i2c-imx.c line 493
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+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.
Quoted 23 lines
drivers/i2c/busses/i2c-imx.c new file +23
-0+ +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);
drivers/i2c/busses/i2c-imx.c line 526
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+ 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");
Quoted 115 lines
drivers/i2c/busses/i2c-imx.c new file +115
-0+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); +} +
drivers/i2c/busses/i2c-imx.c line 651
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+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?
Quoted 47 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+ +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
===================================================================drivers/i2c/busses/Kconfig +10
-0@@ -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_IOP13XXIndex: linux-2.6.28-rc6/drivers/i2c/busses/Makefile
===================================================================drivers/i2c/busses/Makefile +1
-0@@ -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.oIndex:
linux-2.6.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
===================================================================arch/arm/plat-mxc/include/mach/i2c.h new file +4
-0@@ -0,0 +1,17 @@ +/* +* i2c.h - i.MX I2C driver header file +* +* Copyright (c) 2008, Darius Augulis <[email protected]>
arch/arm/plat-mxc/include/mach/i2c.h line 14
Quoted 10 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +10
-0+* +* 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?
arch/arm/plat-mxc/include/mach/i2c.h line 17
Quoted 3 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0+}; + +#endif /* __ASM_ARCH_I2C_H_ */
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
Quoted 1 line · Guennadi Liakhovetski
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!
Quoted 173 lines · Darius
Commenting would have been easier, if you had inlined your patch...
Quoted 168 lines
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: linux-arm-kernel-xIg/[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
===================================================================drivers/i2c/busses/i2c-imx.c new file +142
-0@@ -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) +{
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 148
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c new file +6
-0+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.
Quoted 14 lines
drivers/i2c/busses/i2c-imx.c line 159
Quoted 12 lines
drivers/i2c/busses/i2c-imx.c new file +11
-0+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.
Quoted 14 lines
drivers/i2c/busses/i2c-imx.c line 170
Quoted 12 lines
drivers/i2c/busses/i2c-imx.c new file +11
-0+ } + +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?
Quoted 26 lines
Quoted 26 lines
drivers/i2c/busses/i2c-imx.c new file +25
-0+ +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 )) {
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 201
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c new file +6
-0+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.
Quoted 101 lines
Quoted 101 lines
drivers/i2c/busses/i2c-imx.c new file +100
-0+ +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;
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 307
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c new file +6
-0+ +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.
Quoted 188 lines
Quoted 26 lines
drivers/i2c/busses/i2c-imx.c new file +25
-0+ 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)drivers/i2c/busses/i2c-imx.c line 342
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+{ +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.Quoted 136 lines
drivers/i2c/busses/i2c-imx.c new file +136
-0+ 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" );drivers/i2c/busses/i2c-imx.c line 488
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+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()
Quoted 7 lines
drivers/i2c/busses/i2c-imx.c line 493
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+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.
Quoted 37 lines
Quoted 24 lines
drivers/i2c/busses/i2c-imx.c new file +23
-0+ +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);drivers/i2c/busses/i2c-imx.c line 526
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+ 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:
Review
Quoted 1 line
(quoted diff) +1
-0+ i2c_imx->clk = clk_get(&pdev->dev, "i2c_clk");
ok, fixed.
Quoted 120 lines
Quoted 120 lines
drivers/i2c/busses/i2c-imx.c new file +119
-0+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); +}
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 651
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c new file +6
-0+ +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?
Quoted 52 lines
Quoted 52 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+ +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
===================================================================drivers/i2c/busses/Kconfig +10
-0@@ -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_IOP13XXIndex: linux-2.6.28-rc6/drivers/i2c/busses/Makefile
===================================================================drivers/i2c/busses/Makefile +1
-0@@ -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.oIndex:
linux-2.6.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
===================================================================arch/arm/plat-mxc/include/mach/i2c.h new file +8
-0@@ -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 +*/ +
Quoted 8 lines
arch/arm/plat-mxc/include/mach/i2c.h line 14
Quoted 6 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +6
-0+#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.
Quoted 10 lines
arch/arm/plat-mxc/include/mach/i2c.h line 17
Quoted 4 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0+}; + +#endif /* __ASM_ARCH_I2C_H_ */ Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
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
Quoted 3 lines · Guennadi Liakhovetski
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 :)
Quoted 4 lines
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.
Quoted 6 lines
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
Quoted 6 lines · Darius
Quoted 3 lines · Guennadi Liakhovetski
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
Signature · 2 lines
Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de Pengutronix - Linux Solutions for Science and Industry
Quoted 9 lines · Darius
Quoted 5 lines · Guennadi Liakhovetski
drivers/i2c/busses/i2c-imx.c line 307
Quoted 2 lines
drivers/i2c/busses/i2c-imx.c new file +2
-0+/* 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:
...
Quoted 4 lines
slave addresses are exactly
0x42and0x90(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.
Quoted 9 lines
Quoted 5 lines
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.
Hi Darius,
it would be better, if you didn't drop my email from cc, thanks.
Quoted 9 lines · Darius
Quoted 3 lines · Guennadi Liakhovetski
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 :)Quoted 1 line
Quoted 6 lines
Quoted 4 lines
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
Quoted 1 line
From
1d592e3a94b3e13537a680e3e0198c345248692dMon Sep 17 00:00:00 2001
From: Guennadi Liakhovetski <[email protected]>
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.
--- arch/arm/plat-mxc/include/mach/i2c.h | 8 ++ drivers/i2c/busses/i2c-imx.c | 143 ++++++++++++++++------------------ 2 files changed, 76 insertions(+), 75 deletions(-)
arch/arm/plat-mxc/include/mach/i2c.h +8 -0
@@ -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_ */
drivers/i2c/busses/i2c-imx.c +68 -75
334 lines not on this page · raw message
Signature · 1 line
1.5.4
Quoted 11 lines · Darius
Quoted 9 lines · Guennadi Liakhovetski
drivers/i2c/busses/i2c-imx.c line 170
Quoted 7 lines
drivers/i2c/busses/i2c-imx.c new file +7
-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!
Quoted 1 line
how much? 600us?
mxc uses 1 jiffie.
Quoted 18 lines
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c line 307
Quoted 2 lines
drivers/i2c/busses/i2c-imx.c new file +2
-0+/* 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], } }
Quoted 6 lines
slave addresses are exactly
0x42and0x90(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.
Quoted 9 lines
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 651
Quoted 2 lines
drivers/i2c/busses/i2c-imx.c new file +2
-0+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?
Quoted 3 lines
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:-)
Quoted 9 lines
Quoted 6 lines
arch/arm/plat-mxc/include/mach/i2c.h line 14
Quoted 3 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0+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
Quoted 187 lines · 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...
Quoted 164 lines
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: linux-arm-kernel-xIg/[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
===================================================================drivers/i2c/busses/i2c-imx.c new file +138
-0@@ -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 *************************************** drivers/i2c/busses/i2c-imx.c line 148
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+*******************************************************************************/ + +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.
Quoted 11 lines
drivers/i2c/busses/i2c-imx.c line 159
Quoted 11 lines
drivers/i2c/busses/i2c-imx.c new file +11
-0+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 ofwait_event_interruptible?
Quoted 16 lines
For comparison: the MXC driver only waits for bus busy to clear for 600us at most.
drivers/i2c/busses/i2c-imx.c line 170
Quoted 11 lines
drivers/i2c/busses/i2c-imx.c new file +11
-0+ } + +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.
Quoted 27 lines
Quoted 27 lines
drivers/i2c/busses/i2c-imx.c new file +27
-0+ +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__);
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 160
Quoted 4 lines
drivers/i2c/busses/i2c-imx.c new file +4
-0+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.
Quoted 70 lines
Quoted 70 lines
drivers/i2c/busses/i2c-imx.c new file +70
-0+ +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 );
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 279
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c new file +8
-0+ +/* 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?
Quoted 10 lines
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+} + +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 */
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 297
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c new file +8
-0+ 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.
Quoted 5 lines
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+ +static int i2c_imx_write (struct imx_i2c_struct *i2c_imx,struct i2c_msg *msgs) +{ +int i, result; +
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 307
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+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?
Quoted 29 lines
Quoted 29 lines
drivers/i2c/busses/i2c-imx.c new file +29
-0+ 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; +
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 342
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c new file +6
-0+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.
Quoted 149 lines
Quoted 136 lines
drivers/i2c/busses/i2c-imx.c new file +136
-0+ 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" );drivers/i2c/busses/i2c-imx.c line 488
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+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()
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 493
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+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.
Quoted 36 lines
Quoted 23 lines
drivers/i2c/busses/i2c-imx.c new file +23
-0+ +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);drivers/i2c/busses/i2c-imx.c line 526
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+ 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:
Review
Quoted 1 line
(quoted diff) +1
-0+ 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.
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c line 536
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+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?
Quoted 168 lines
Quoted 105 lines
drivers/i2c/busses/i2c-imx.c new file +105
-0+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); +} +drivers/i2c/busses/i2c-imx.c line 651
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+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?
Quoted 46 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+ +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
===================================================================drivers/i2c/busses/Kconfig +10
-0@@ -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_IOP13XXIndex: linux-2.6.28-rc6/drivers/i2c/busses/Makefile
===================================================================drivers/i2c/busses/Makefile +1
-0@@ -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.oIndex:
linux-2.6.28-rc6/arch/arm/plat-mxc/include/mach/i2c.h
===================================================================arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0@@ -0,0 +1,17 @@ +/* +* i2c.h - i.MX I2C driver header file +*
Quoted 8 lines
arch/arm/plat-mxc/include/mach/i2c.h line 11
Quoted 8 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +8
-0+* Copyright (c) 2008, Darius Augulis <[email protected]> +* +* 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.
Quoted 20 lines
arch/arm/plat-mxc/include/mach/i2c.h line 14
Quoted 3 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0+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?
arch/arm/plat-mxc/include/mach/i2c.h line 17
Quoted 3 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0+}; + +#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
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
Quoted 58 lines · Guennadi Liakhovetski
Quoted 11 lines · Darius
Quoted 9 lines · Guennadi Liakhovetski
drivers/i2c/busses/i2c-imx.c line 170
Quoted 7 lines
drivers/i2c/busses/i2c-imx.c new file +7
-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!
Quoted 1 line
how much? 600us?
mxc uses 1 jiffie.
Quoted 18 lines
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c line 307
Quoted 2 lines
drivers/i2c/busses/i2c-imx.c new file +2
-0+/* 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], } }Quoted 6 lines
slave addresses are exactly
0x42and0x90(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.
Quoted 9 lines
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 651
Quoted 2 lines
drivers/i2c/busses/i2c-imx.c new file +2
-0+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?
Quoted 3 lines
We can do that, but now there is possibility to change bitrate when re-loading module.
What is better?
Quoted 5 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
arch/arm/plat-mxc/include/mach/i2c.h line 14
Quoted 3 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0+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?
Quoted 7 lines
Quoted 2 lines
.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 yougpio_free()in .exit()... Is this what you mean?
What does David Brownell think of that?
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
Quoted 54 lines · Guennadi Liakhovetski
Hi Darius,
it would be better, if you didn't drop my email from cc, thanks.
Quoted 9 lines · Darius
Quoted 3 lines · Guennadi Liakhovetski
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 :)Quoted 1 line
Quoted 6 lines
Quoted 4 lines
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 DeveloperQuoted 1 line
From
1d592e3a94b3e13537a680e3e0198c345248692dMon Sep 17 00:00:00 2001From: Guennadi Liakhovetski <[email protected]>
Date: Wed, 3 Dec 2008 17:02:37 +0100
Subject: [PATCH] i2x-imx: fix client addresses, remove needless delays, support multiple bussesWith 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 <[email protected]>--- arch/arm/plat-mxc/include/mach/i2c.h | 8 ++ drivers/i2c/busses/i2c-imx.c | 143 ++++++++++++++++------------------ 2 files changed, 76 insertions(+), 75 deletions(-)arch/arm/plat-mxc/include/mach/i2c.h +1
-0@@ -10,8 +10,16 @@#define __ASM_ARCH_I2C_H_struct imxi2c_platform_data { +u32 i2c_clk;
arch/arm/plat-mxc/include/mach/i2c.h line 23
Quoted 10 lines
arch/arm/plat-mxc/include/mach/i2c.h +6
-0int (*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);
Quoted 48 lines
arch/arm/plat-mxc/include/mach/i2c.h +0
-0#endif /* __ASM_ARCH_I2C_H_ */ drivers/i2c/busses/i2c-imx.c +3
-15@@ -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.
drivers/i2c/busses/i2c-imx.c struct imx_i2c_struct { · line 135
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c +10
-0+*/ +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.
Quoted 308 lines
drivers/i2c/busses/i2c-imx.c +55
-60+ +/** 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" );Signature · 6 lines
1.5.4 -- 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
Signature · 3 lines
Ben ([email protected], http://www.fluff.org/) 'a smiley only costs 4 bytes'
Quoted 17 lines · Ben Dooks
arch/arm/plat-mxc/include/mach/i2c.h line 23
Quoted 13 lines · Guennadi Liakhovetski
struct
imxi2c_platform_data{arch/arm/plat-mxc/include/mach/i2c.h +7
-0+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
Quoted 2 lines
mx2_i2c_register_board(int
i2c_channel_nr, structimx_i2c_platform_data *pd);
on i.MX31 there are 3.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
Quoted 47 lines · Guennadi Liakhovetski
Quoted 12 lines · Darius
Quoted 10 lines · Guennadi Liakhovetski
drivers/i2c/busses/i2c-imx.c line 170
Quoted 8 lines
drivers/i2c/busses/i2c-imx.c new file +7
-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!
Quoted 2 lines
how much? 600us?
mxc uses 1 jiffie.
Quoted 18 lines
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c line 307
Quoted 3 lines
drivers/i2c/busses/i2c-imx.c new file +2
-0+/* 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], } }Quoted 7 lines
slave addresses are exactly
0x42and0x90(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.
Quoted 2 lines
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...
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c line 651
Quoted 4 lines
drivers/i2c/busses/i2c-imx.c new file +2
-0+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?
Quoted 7 lines
Quoted 4 lines
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.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
arch/arm/plat-mxc/include/mach/i2c.h line 14
Quoted 5 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0+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?
Quoted 6 lines
Quoted 3 lines
.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 yougpio_free()in .exit()... Is this what you mean?
yes, you're right.
Quoted 7 lines
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
drivers/i2c/busses/i2c-imx.c new file +11
-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 ; + }Quoted 8 lines
drivers/i2c/busses/i2c-imx.c line 201
Quoted 7 lines
drivers/i2c/busses/i2c-imx.c new file +7
-0+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?
Quoted 2 lines
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.
Quoted 73 lines
Quoted 73 lines
Quoted 73 lines
drivers/i2c/busses/i2c-imx.c new file +72
-0+ +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 */
Quoted 8 lines
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c line 279
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c new file +6
-0+#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.
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
drivers/i2c/busses/i2c-imx.c new file +13
-0+} + +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 );
Quoted 8 lines
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c line 297
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+wake_up_interruptible (&i2c_imx->queue); + } + +return IRQ_HANDLED ; +}it would be nice if we returned
IRQ_NONEif we didn't have anything to handle.
I will add this.
Quoted 7 lines
Quoted 6 lines
drivers/i2c/busses/i2c-imx.c line 493
Quoted 5 lines
drivers/i2c/busses/i2c-imx.c new file +5
-0+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.
Quoted 6 lines
I belive that
request_mem_region()is the correct thing to do, IIRC, theplatform_device_register()callsinsert_resource()on all the resource regions of the device so that it is guarnateed to appear in the/proc/ioportor/proc/iomemmap. However I belive this does not actually make a reservation of that resource.
confused... Guennadi, can you please explain more your mention?
Quoted 43 lines
Quoted 35 lines
Quoted 23 lines
drivers/i2c/busses/i2c-imx.c new file +23
-0+ +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);drivers/i2c/busses/i2c-imx.c line 526
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+ 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:
Review
Quoted 1 line
(quoted diff) +1
-0+ 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.
Quoted 12 lines
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c line 536
Quoted 10 lines
drivers/i2c/busses/i2c-imx.c new file +10
-0+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.
Quoted 9 lines
kernel doc comments would be nice for this to actually say what is expected of the implementation and what these calls are there for.
Quoted 5 lines
arch/arm/plat-mxc/include/mach/i2c.h line 14
Quoted 3 lines
arch/arm/plat-mxc/include/mach/i2c.h new file +3
-0+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?
Quoted 8 lines · Darius
Quoted 8 lines
Quoted 7 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ 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.
Quoted 8 lines
Quoted 6 lines
I belive that
request_mem_region()is the correct thing to do, IIRC, theplatform_device_register()callsinsert_resource()on all the resource regions of the device so that it is guarnateed to appear in the/proc/ioportor/proc/iomemmap. 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
Quoted 11 lines
Quoted 11 lines
Review
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ /* Get I2C clock */ + i2c_imx->clk = clk_get(NULL, "i2c_clk");There can be several i2c busses on the system, so you want:
(quoted diff) +1
-0+ 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.
Quoted 9 lines
Quoted 6 lines
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+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
Quoted 6 lines · Darius
This means I should use shifted address in platform data? For example: OV7670 image sensor has slave address
0x42for write, and0x43for read operations.
This is hardware slave address, to which device responds. But I should use0x21in 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.
Quoted 1 line · Darius
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
Quoted 5 lines · Guennadi Liakhovetski
Quoted 1 line · Darius
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.
Quoted 6 lines
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer