* [PATCH V1 2/3] i2c: imx: only imx1 needs disable delay
[not found] ` <1255772784-11387-1-git-send-email-linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2009-10-17 9:46 ` Richard Zhao
[not found] ` <1255772784-11387-2-git-send-email-linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2009-10-20 10:03 ` [PATCH V1 1/3] i2c: imx: check busy bit when START/STOP Richard Zhao
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Richard Zhao @ 2009-10-17 9:46 UTC (permalink / raw)
To: kernel-bIcnvbaLZ9MEGnE8C9+IrQ
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, Richard Zhao
check cpu_is_mx1() when disable delay.
Signed-off-by: Richard Zhao <linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 6055e92..671d37c 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -225,11 +225,13 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
i2c_imx->stopped = 1;
}
- /*
- * 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);
+ if (cpu_is_mx1()) {
+ /*
+ * 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);
+ }
if (!i2c_imx->stopped)
i2c_imx_bus_busy(i2c_imx, 0);
--
1.6.0.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH V1 1/3] i2c: imx: check busy bit when START/STOP
[not found] ` <1255772784-11387-1-git-send-email-linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2009-10-17 9:46 ` [PATCH V1 2/3] i2c: imx: only imx1 needs disable delay Richard Zhao
@ 2009-10-20 10:03 ` Richard Zhao
2009-10-21 14:11 ` Richard Zhao
2009-10-30 0:40 ` Ben Dooks
3 siblings, 0 replies; 8+ messages in thread
From: Richard Zhao @ 2009-10-20 10:03 UTC (permalink / raw)
To: kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Sascha Hauer
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, Richard Zhao
On Sat, Oct 17, 2009 at 5:46 PM, Richard Zhao <linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> The controller can't do anything else before it actually generates START/STOP.
> So we check busy bit to make sure START/STOP is successfully finished.
>
> If we don't check busy bit, START/STOP may fail on some fast CPUs.
>
> Signed-off-by: Richard Zhao <linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 4afba3e..6055e92 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -120,19 +120,25 @@ struct imx_i2c_struct {
> wait_queue_head_t queue;
> unsigned long i2csr;
> unsigned int disable_delay;
> + int stopped;
> };
>
> /** Functions for IMX I2C adapter driver ***************************************
> *******************************************************************************/
>
> -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
> +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 */
> - while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
> + while (1) {
> + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
> + 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__);
> @@ -179,39 +185,55 @@ 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__);
>
> /* Enable I2C controller */
> + writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> +
> + /* Wait controller to be stable */
> + udelay(50);
> +
> /* 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)
> + return result;
> + i2c_imx->stopped = 0;
> +
> 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)
> {
> 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);
> + if (!i2c_imx->stopped) {
> + /* Stop I2C transaction */
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> + temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> + temp &= ~(I2CR_MSTA | I2CR_MTX);
> + writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> + i2c_imx->stopped = 1;
> + }
> /*
> * 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);
> +
> + if (!i2c_imx->stopped)
> + i2c_imx_bus_busy(i2c_imx, 0);
> +
> /* Disable I2C controller */
> writeb(0, i2c_imx->base + IMX_I2C_I2CR);
> }
> @@ -341,11 +363,15 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> if (result)
> return result;
> if (i == (msgs->len - 1)) {
> + /* It must generate STOP before read I2DR to prevent
> + controller from generating another clock cycle */
> 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);
> + i2c_imx_bus_busy(i2c_imx, 0);
> + i2c_imx->stopped = 1;
> } else if (i == (msgs->len - 2)) {
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> set TXAK\n", __func__);
> @@ -370,14 +396,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>
> - /* Check if i2c bus is not busy */
> - result = i2c_imx_bus_busy(i2c_imx);
> + /* Start I2C transfer */
> + result = i2c_imx_start(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) {
> @@ -386,6 +409,9 @@ 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)
> + goto fail0;
> }
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> transfer message: %d\n", __func__, i);
> --
> 1.6.0.4
>
>
Hello Sascha,
Could you please review this series of patch?
Thanks
Richard
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH V1 1/3] i2c: imx: check busy bit when START/STOP
[not found] ` <1255772784-11387-1-git-send-email-linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2009-10-17 9:46 ` [PATCH V1 2/3] i2c: imx: only imx1 needs disable delay Richard Zhao
2009-10-20 10:03 ` [PATCH V1 1/3] i2c: imx: check busy bit when START/STOP Richard Zhao
@ 2009-10-21 14:11 ` Richard Zhao
2009-10-30 0:40 ` Ben Dooks
3 siblings, 0 replies; 8+ messages in thread
From: Richard Zhao @ 2009-10-21 14:11 UTC (permalink / raw)
To: kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Ben Dooks (embedded platforms)
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, Richard Zhao
On Sat, Oct 17, 2009 at 5:46 PM, Richard Zhao <linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> The controller can't do anything else before it actually generates START/STOP.
> So we check busy bit to make sure START/STOP is successfully finished.
>
> If we don't check busy bit, START/STOP may fail on some fast CPUs.
>
> Signed-off-by: Richard Zhao <linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 4afba3e..6055e92 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -120,19 +120,25 @@ struct imx_i2c_struct {
> wait_queue_head_t queue;
> unsigned long i2csr;
> unsigned int disable_delay;
> + int stopped;
> };
>
> /** Functions for IMX I2C adapter driver ***************************************
> *******************************************************************************/
>
> -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx)
> +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 */
> - while (readb(i2c_imx->base + IMX_I2C_I2SR) & I2SR_IBB) {
> + while (1) {
> + temp = readb(i2c_imx->base + IMX_I2C_I2SR);
> + 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__);
> @@ -179,39 +185,55 @@ 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__);
>
> /* Enable I2C controller */
> + writeb(0, i2c_imx->base + IMX_I2C_I2SR);
> writeb(I2CR_IEN, i2c_imx->base + IMX_I2C_I2CR);
> +
> + /* Wait controller to be stable */
> + udelay(50);
> +
> /* 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)
> + return result;
> + i2c_imx->stopped = 0;
> +
> 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)
> {
> 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);
> + if (!i2c_imx->stopped) {
> + /* Stop I2C transaction */
> + dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> + temp = readb(i2c_imx->base + IMX_I2C_I2CR);
> + temp &= ~(I2CR_MSTA | I2CR_MTX);
> + writeb(temp, i2c_imx->base + IMX_I2C_I2CR);
> + i2c_imx->stopped = 1;
> + }
> /*
> * 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);
> +
> + if (!i2c_imx->stopped)
> + i2c_imx_bus_busy(i2c_imx, 0);
> +
> /* Disable I2C controller */
> writeb(0, i2c_imx->base + IMX_I2C_I2CR);
> }
> @@ -341,11 +363,15 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> if (result)
> return result;
> if (i == (msgs->len - 1)) {
> + /* It must generate STOP before read I2DR to prevent
> + controller from generating another clock cycle */
> 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);
> + i2c_imx_bus_busy(i2c_imx, 0);
> + i2c_imx->stopped = 1;
> } else if (i == (msgs->len - 2)) {
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> set TXAK\n", __func__);
> @@ -370,14 +396,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
>
> dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
>
> - /* Check if i2c bus is not busy */
> - result = i2c_imx_bus_busy(i2c_imx);
> + /* Start I2C transfer */
> + result = i2c_imx_start(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) {
> @@ -386,6 +409,9 @@ 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)
> + goto fail0;
> }
> dev_dbg(&i2c_imx->adapter.dev,
> "<%s> transfer message: %d\n", __func__, i);
> --
> 1.6.0.4
>
>
Adding Ben ...
Thanks
Richard
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH V1 1/3] i2c: imx: check busy bit when START/STOP
[not found] ` <1255772784-11387-1-git-send-email-linuxzsc-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
` (2 preceding siblings ...)
2009-10-21 14:11 ` Richard Zhao
@ 2009-10-30 0:40 ` Ben Dooks
[not found] ` <20091030004025.GG13398-elnMNo+KYs3pIgCt6eIbzw@public.gmane.org>
3 siblings, 1 reply; 8+ messages in thread
From: Ben Dooks @ 2009-10-30 0:40 UTC (permalink / raw)
To: Richard Zhao
Cc: kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
Does anyone have any comments on these, or shall I look at merging
them as fixes?
--
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
'a smiley only costs 4 bytes'
^ permalink raw reply [flat|nested] 8+ messages in thread