[PATCH] i2c: imx: check busy bit when START/STOP
From: s.hauer@pengutronix.de (Sascha Hauer)
Date: 2009-09-29 13:25:05
Also in:
linux-i2c
Hi Richard, On Tue, Sep 29, 2009 at 07:41:44PM +0800, Richard Zhao wrote:
quoted hunk ↗ jump to hunk
After START, wait busy bit to be set and after STOP, wait busy bit to be clear. Disable clock when it's possible to save power. Signed-off-by: Richard Zhao <redacted> --- drivers/i2c/busses/i2c-imx.c | 116 ++++++++++++++++++++++++++++++++---------- 1 files changed, 88 insertions(+), 28 deletions(-)diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 4afba3e..59cde70 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c@@ -120,19 +120,40 @@ struct imx_i2c_struct { wait_queue_head_t queue; unsigned long i2csr; unsigned int disable_delay; + unsigned int ifdr; /* IMX_I2C_IFDR */ }; /** Functions for IMX I2C adapter driver*************************************** *******************************************************************************/ -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx) +#ifdef CONFIG_I2C_DEBUG_BUS +#define reg_dump(i2c_imx) \ +{ \ + printk(KERN_DEBUG "fun %s:%d ", __func__, __LINE__); \ + printk(KERN_DEBUG "IADR %02x IFDR %02x I2CR %02x I2SR %02x\n", \ + readb(i2c_imx->base + IMX_I2C_IADR), \ + readb(i2c_imx->base + IMX_I2C_IFDR), \ + readb(i2c_imx->base + IMX_I2C_I2CR), \ + readb(i2c_imx->base + IMX_I2C_I2SR)); \ +} +#else +#define reg_dump(i2c_imx) +#endif +
Can you please remove this reg_dump? If we really need this it should be in an extra patch and not clutter this one.
+static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
{
unsigned long orig_jiffies = jiffies;
+ unsigned int temp;
dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
/* wait for bus not busy */This comment seems wrong now. This function waits for busy or not busy depending on for_busy.
quoted hunk ↗ jump to hunk
- while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) { + temp = readb(i2c_imx->base + IMX_I2C_I2SR); + while (1) { + if (for_busy && (temp & I2SR_IBB)) + break; + if (!for_busy && !(temp & I2SR_IBB)) + break; if (signal_pending(current)) { dev_dbg(&i2c_imx->adapter.dev, "<%s> I2C Interrupted\n", __func__);@@ -141,9 +162,11 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx) if (time_after(jiffies, orig_jiffies + HZ / 1000)) { dev_dbg(&i2c_imx->adapter.dev, "<%s> I2C bus is busy\n", __func__); + reg_dump(i2c_imx); return -EIO; } schedule(); + temp = readb(i2c_imx->base + IMX_I2C_I2SR); } return 0;@@ -158,9 +181,11 @@ static int i2c_imx_trx_complete(structimx_i2c_struct *i2c_imx) if (unlikely(result < 0)) { dev_dbg(&i2c_imx->adapter.dev, "<%s> result < 0\n", __func__); + reg_dump(i2c_imx); return result; } else if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) { dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__); + reg_dump(i2c_imx); return -ETIMEDOUT; } dev_dbg(&i2c_imx->adapter.dev, "<%s> TRX complete\n", __func__);@@ -172,6 +197,7 @@ static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx) { if (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_RXAK) { dev_dbg(&i2c_imx->adapter.dev, "<%s> No ACK\n", __func__); + reg_dump(i2c_imx); return -EIO; /* No ACK */ }@@ -179,20 +205,37 @@ static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx) return 0; } -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) { unsigned int temp = 0; + int result; dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); + clk_enable(i2c_imx->clk); + writeb(i2c_imx->ifdr, i2c_imx->base + IMX_I2C_IFDR); /* Enable I2C controller */ + writeb(0, i2c_imx->base + IMX_I2C_I2SR); writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR); + + result = i2c_imx_bus_busy(i2c_imx, 0); + if (result) { + reg_dump(i2c_imx); + return result; + } + /* Start I2C transaction */ temp = readb(i2c_imx->base + IMX_I2C_I2CR); temp |= I2CR_MSTA; writeb(temp, i2c_imx->base + IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1); + if (result) { + reg_dump(i2c_imx); + return result; + } temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; writeb(temp, i2c_imx->base + IMX_I2C_I2CR); + return result; } static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)@@ -202,18 +245,21 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) /* Stop I2C transaction */ dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); temp = readb(i2c_imx->base + IMX_I2C_I2CR); - temp &= ~I2CR_MSTA; + temp &= ~(I2CR_MSTA | I2CR_MTX);
This change seems unrelated. Is it necessary?
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(i2c_imx->disable_delay); + +
double blank line
quoted hunk ↗ jump to hunk
+ if (i2c_imx_bus_busy(i2c_imx, 0)) + reg_dump(i2c_imx); + /* Disable I2C controller */ writeb(0, i2c_imx->base + IMX_I2C_I2CR); + clk_disable(i2c_imx->clk); } static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,@@ -233,17 +279,19 @@ static void __init i2c_imx_set_clk(structimx_i2c_struct *i2c_imx, 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);
Why can't we write this value directly anymore...
- - /* - * 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. - */ - i2c_imx->disable_delay = (500000U * i2c_clk_div[i][0] - + (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2); + /* Store divider value */ + i2c_imx->ifdr = i2c_clk_div[i][1];
...but have to store it in a variable instead?
quoted hunk ↗ jump to hunk
+ + if (cpu_is_mx1()) { + /* + * 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. + */ + i2c_imx->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@@ -344,7 +392,7 @@ static int i2c_imx_read(struct imx_i2c_struct*i2c_imx, struct i2c_msg *msgs) dev_dbg(&i2c_imx->adapter.dev, "<%s> clear MSTA\n", __func__); temp = readb(i2c_imx->base + IMX_I2C_I2CR); - temp &= ~I2CR_MSTA; + temp &= ~(I2CR_MSTA | I2CR_MTX); writeb(temp, i2c_imx->base + IMX_I2C_I2CR); } else if (i == (msgs->len - 2)) { dev_dbg(&i2c_imx->adapter.dev,@@ -369,14 +417,24 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter); dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); - +#ifdef CONFIG_I2C_DEBUG_BUS + for (i = 0; i < num; i++) { + printk(KERN_DEBUG "msg%d addr %02x RD %d cnt %d d:", i, + msgs[i].addr, msgs[i].flags & I2C_M_RD, msgs[i].len); + if (!(msgs[i].flags & I2C_M_RD)) { + int j; + for (j = 0; j < msgs[i].len; j++) + printk("%02x ", msgs[i].buf[j]); + } + printk("\n"); + } +#endif /* Check if i2c bus is not busy */ - result = i2c_imx_bus_busy(i2c_imx); - if (result) - goto fail0;
When removing the code you should also remove the comment.
quoted hunk ↗ jump to hunk
/* Start I2C transfer */ - i2c_imx_start(i2c_imx); + result = i2c_imx_start(i2c_imx); + if (result) + goto fail0; /* read/write data */ for (i = 0; i < num; i++) {@@ -386,6 +444,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, temp = readb(i2c_imx->base + IMX_I2C_I2CR); temp |= I2CR_RSTA; writeb(temp, i2c_imx->base + IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1); + if (result) { + reg_dump(i2c_imx); + goto fail0; + } } dev_dbg(&i2c_imx->adapter.dev, "<%s> transfer message: %d\n", __func__, i);@@ -442,7 +505,7 @@ static int __init i2c_imx_probe(structplatform_device *pdev) int irq; int ret; - dev_dbg(&pdev->dev, "<%s>\n", __func__); + dev_info(&pdev->dev, "<%s>\n", __func__);
no
quoted hunk ↗ jump to hunk
res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) {@@ -500,7 +563,6 @@ static int __init i2c_imx_probe(structplatform_device *pdev) dev_err(&pdev->dev, "can't get I2C clock\n"); goto fail3; } - clk_enable(i2c_imx->clk);
I think it's generally a good idea to start/stop the clocks when needed as you do here. It should be an extra patch though.
quoted hunk ↗ jump to hunk
/* Request IRQ */ ret = request_irq(i2c_imx->irq, i2c_imx_isr, 0, pdev->name, i2c_imx);@@ -549,7 +611,6 @@ static int __init i2c_imx_probe(structplatform_device *pdev) fail5: free_irq(i2c_imx->irq, i2c_imx); fail4: - clk_disable(i2c_imx->clk); clk_put(i2c_imx->clk); fail3: release_mem_region(i2c_imx->res->start, resource_size(res));@@ -587,7 +648,6 @@ static int __exit i2c_imx_remove(structplatform_device *pdev) pdata->exit(&pdev->dev); /* Disable I2C clock */ - clk_disable(i2c_imx->clk); clk_put(i2c_imx->clk); release_mem_region(i2c_imx->res->start, resource_size(i2c_imx->res)); -- 1.6.0.4
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |