On Tue, Sep 29, 2009 at 10:06 PM, Sascha Hauer [off-list ref] wrote:

On Tue, Sep 29, 2009 at 09:54:05PM +0800, Richard Zhao wrote:

quoted

Thanks, Sacha. See comments inline.

On Tue, Sep 29, 2009 at 9:25 PM, Sascha Hauer [off-list ref] wrote:

quoted

Hi Richard,


On Tue, Sep 29, 2009 at 07:41:44PM +0800, Richard Zhao wrote:

quoted

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.

I think it's needed. It helps much to debug and don't effect
performance by default.

quoted

quoted

+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.

Right. Thanks

quoted

quoted

- ? ? 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(struct

imx_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?

Yes. It's about STOP. It's better clear MTX. We must generate STOP
before read the last byte of Data register. If not, there will be an
extra 9bit clock.

quoted

quoted

? ? ? 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

Right.

quoted

quoted

+ ? ? 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(struct

imx_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...

clock will be enable/disable in start/stop

quoted

quoted

-
- ? ? /*
- ? ? ?* 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?

To save power, because i2c is not aways be accessed, and it's very low
speed bus.

quoted

quoted

+
+ ? ? 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.

Right

quoted

quoted

? ? ? /* 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(struct

platform_device *pdev)
? ? ? int irq;
? ? ? int ret;

- ? ? dev_dbg(&pdev->dev, "<%s>\n", __func__);
+ ? ? dev_info(&pdev->dev, "<%s>\n", __func__);

no

Why? I even don't know how many i2c controllers we have, if I didn't
get sysfs working. And it only print once.

quoted

quoted

? ? ? res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
? ? ? if (!res) {

@@ -500,7 +563,6 @@ static int __init i2c_imx_probe(struct

platform_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.

You think I should split the patch to three: IBB, reg_dump, clk dis/en ?
The idea is good, more easy to revert. but sometimes, I think it make
small things too complex.
Anyway, I'll follow your suggestion.

In case of regressions everybody will thank you for doing it.

Sascha


--
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 |

Hi Sascha,

Do you agree with my other comments?

Thanks
Richard