2012-05-16 07:31:36

by Zhang, Sonic

Subject: [PATCH 1/8] i2c: i2c-bfin-twi: Illegal i2c bus lock upon certain transfer scenarios.

From: Michael Hennerich <[email protected]>

For transfer counts > 255 bytes i2c-bfin-twi sets the data
transfer counter DCNT to 0xFF indicating unlimited transfers.
It then uses a flag iface->manual_stop to manually issue the STOP
condition, once the required amount of bytes are received.

We found that on I2C receive operation issuing the STOP condition
together with a FULL RCV FIFO (2bytes) will cause SDA and SCL be
constantly driven low.

Temporary workaround until further investigation:
Discard the RCV FIFO before issuing the STOP condition.

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Sonic Zhang <[email protected]>

git-svn-id: svn://localhost/svn/linux-kernel/trunk@9011 526b6c2d-f592-4532-a319-5dd88ccb003d
---
drivers/i2c/busses/i2c-bfin-twi.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index cdb59e5..e6dd3c9 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -131,6 +131,10 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
iface->transPtr++;
iface->readNum--;
} else if (iface->manual_stop) {
+ /* Avoid possible bus stall -
+ * Flush FIFO before issuing the STOP condition
+ */
+ read_RCV_DATA16(iface);
write_MASTER_CTL(iface,
read_MASTER_CTL(iface) | STOP);
} else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
--
1.7.0.4

2012-05-16 07:31:29

by Zhang, Sonic

Subject: [PATCH 2/8] i2c: i2c-bfin-twi: Stop receiving operation immediately in last rx interrupt.

From: Sonic Zhang <[email protected]>

Also wake up waiting process when transfer completes.

Signed-off-by: Sonic Zhang <[email protected]>

git-svn-id: svn://localhost/svn/linux-kernel/trunk@9105 526b6c2d-f592-4532-a319-5dd88ccb003d
---
drivers/i2c/busses/i2c-bfin-twi.c | 43 ++++++++++++++++++++----------------
1 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index e6dd3c9..376e331 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -130,21 +130,25 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
}
iface->transPtr++;
iface->readNum--;
- } else if (iface->manual_stop) {
- /* Avoid possible bus stall -
- * Flush FIFO before issuing the STOP condition
- */
- read_RCV_DATA16(iface);
- write_MASTER_CTL(iface,
- read_MASTER_CTL(iface) | STOP);
- } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
- iface->cur_msg + 1 < iface->msg_num) {
- if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD)
- write_MASTER_CTL(iface,
- read_MASTER_CTL(iface) | RSTART | MDIR);
- else
+ }
+
+ if (iface->readNum == 0) {
+ if (iface->manual_stop) {
+ /* Avoid possible bus stall -
+ * Flush FIFO before issuing the STOP condition
+ */
+ read_RCV_DATA16(iface);
write_MASTER_CTL(iface,
- (read_MASTER_CTL(iface) | RSTART) & ~MDIR);
+ read_MASTER_CTL(iface) | STOP);
+ } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
+ iface->cur_msg + 1 < iface->msg_num) {
+ if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD)
+ write_MASTER_CTL(iface,
+ read_MASTER_CTL(iface) | RSTART | MDIR);
+ else
+ write_MASTER_CTL(iface,
+ (read_MASTER_CTL(iface) | RSTART) & ~MDIR);
+ }
}
}
if (twi_int_status & MERR) {
@@ -245,12 +249,13 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
}
}

- if (iface->pmsg[iface->cur_msg].len <= 255)
- write_MASTER_CTL(iface,
+ if (iface->pmsg[iface->cur_msg].len <= 255) {
+ write_MASTER_CTL(iface,
(read_MASTER_CTL(iface) &
(~(0xff << 6))) |
- (iface->pmsg[iface->cur_msg].len << 6));
- else {
+ (iface->pmsg[iface->cur_msg].len << 6));
+ iface->manual_stop = 0;
+ } else {
write_MASTER_CTL(iface,
(read_MASTER_CTL(iface) |
(0xff << 6)));
@@ -264,8 +269,8 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
write_INT_MASK(iface, 0);
write_MASTER_CTL(iface, 0);
}
+ complete(&iface->complete);
}
- complete(&iface->complete);
}

/* Interrupt handler */
--
1.7.0.4

2012-05-16 07:31:38

by Zhang, Sonic

Subject: [PATCH 5/8] i2c:i2c-bfin-twi: TWI fails to restart next transfer in high system load.

From: Sonic Zhang <[email protected]>

Current driver was developed based on BF537 0.2 HRM. In high system load, BUFRDERR error
interrupt may be raised if XMTSERV interrupt of last TX byte is not served in time
(set RSTART bit), which breaks restart tranfer as expected.

"Buffer Read Error (BUFRDERR)" description in Blackfin HRM only applys to BF537
rev. < 0.3. In later rev. and later announced Blackfin chips, such as BF527 and
BF548, a new TWI master feature "Clock Stretching" is added into the TWI controller,
BUFRDERR interrupt is not triggered after TX FIFO is empty.

This patch sets RSTART bit at the beginning of the first transfer. The SCL and SDA
is hold till XMTSERV interrupt of last TX byte is served. Restart transfer is not broken
in high system load.

Signed-off-by: Sonic Zhang <[email protected]>
---
drivers/i2c/busses/i2c-bfin-twi.c | 23 +++++++++++++----------
1 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index 63fb84f..594bbb1 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -99,7 +99,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
*/
else if (iface->cur_mode == TWI_I2C_MODE_COMBINED)
write_MASTER_CTL(iface,
- read_MASTER_CTL(iface) | MDIR | RSTART);
+ read_MASTER_CTL(iface) | MDIR);
else if (iface->manual_stop)
write_MASTER_CTL(iface,
read_MASTER_CTL(iface) | STOP);
@@ -107,10 +107,10 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
iface->cur_msg + 1 < iface->msg_num) {
if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD)
write_MASTER_CTL(iface,
- read_MASTER_CTL(iface) | RSTART | MDIR);
+ read_MASTER_CTL(iface) | MDIR);
else
write_MASTER_CTL(iface,
- (read_MASTER_CTL(iface) | RSTART) & ~MDIR);
+ read_MASTER_CTL(iface) & ~MDIR);
}
}
if (twi_int_status & RCVSERV) {
@@ -144,10 +144,10 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
iface->cur_msg + 1 < iface->msg_num) {
if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD)
write_MASTER_CTL(iface,
- read_MASTER_CTL(iface) | RSTART | MDIR);
+ read_MASTER_CTL(iface) | MDIR);
else
write_MASTER_CTL(iface,
- (read_MASTER_CTL(iface) | RSTART) & ~MDIR);
+ read_MASTER_CTL(iface) & ~MDIR);
}
}
}
@@ -262,9 +262,10 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
(0xff << 6)));
iface->manual_stop = 1;
}
- /* remove restart bit and enable master receive */
- write_MASTER_CTL(iface,
- read_MASTER_CTL(iface) & ~RSTART);
+ /* remove restart bit before last message */
+ if (iface->cur_msg+1 == iface->msg_num)
+ write_MASTER_CTL(iface,
+ read_MASTER_CTL(iface) & ~RSTART);
} else {
iface->result = 1;
write_INT_MASK(iface, 0);
@@ -327,7 +328,8 @@ static int bfin_twi_do_master_xfer(struct i2c_adapter *adap,
return -EINVAL;
}

- iface->cur_mode = TWI_I2C_MODE_REPEAT;
+ if (iface->msg_num > 1)
+ iface->cur_mode = TWI_I2C_MODE_REPEAT;
iface->manual_stop = 0;
iface->transPtr = pmsg->buf;
iface->writeNum = iface->readNum = pmsg->len;
@@ -372,6 +374,7 @@ static int bfin_twi_do_master_xfer(struct i2c_adapter *adap,

/* Master enable */
write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
+ (iface->msg_num > 1 ? RSTART : 0) |
((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0));
SSYNC();
@@ -540,7 +543,7 @@ int bfin_twi_do_smbus_xfer(struct i2c_adapter *adap, u16 addr,
else
write_MASTER_CTL(iface, 0x1 << 6);
/* Master enable */
- write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
+ write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN | RSTART |
((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
break;
default:
--
1.7.0.4

2012-05-16 07:31:33

by Zhang, Sonic

Subject: [PATCH 7/8] i2c: i2c-bfin-twi: Move TWI peripheral pin request array to platform data.

From: Sonic Zhang <[email protected]>

Signed-off-by: Sonic Zhang <[email protected]>
---
drivers/i2c/busses/i2c-bfin-twi.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index d7e8fba..b2ca317 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -78,11 +78,6 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
DEFINE_TWI_REG(RCV_DATA8, 0x88)
DEFINE_TWI_REG(RCV_DATA16, 0x8C)

-static const u16 pin_req[2][3] = {
- {P_TWI0_SCL, P_TWI0_SDA, 0},
- {P_TWI1_SCL, P_TWI1_SDA, 0},
-};
-
static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
unsigned short twi_int_status)
{
@@ -719,7 +714,8 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
p_adap->timeout = 5 * HZ;
p_adap->retries = 3;

- rc = peripheral_request_list(pin_req[pdev->id], "i2c-bfin-twi");
+ rc = peripheral_request_list((unsigned short *)pdev->dev.platform_data,
+ "i2c-bfin-twi");
if (rc) {
dev_err(&pdev->dev, "Can't setup pin mux!\n");
goto out_error_pin_mux;
@@ -766,7 +762,7 @@ out_error_add_adapter:
free_irq(iface->irq, iface);
out_error_req_irq:
out_error_no_irq:
- peripheral_free_list(pin_req[pdev->id]);
+ peripheral_free_list((unsigned short *)pdev->dev.platform_data);
out_error_pin_mux:
iounmap(iface->regs_base);
out_error_ioremap:
@@ -784,7 +780,7 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)

i2c_del_adapter(&(iface->adap));
free_irq(iface->irq, iface);
- peripheral_free_list(pin_req[pdev->id]);
+ peripheral_free_list((unsigned short *)pdev->dev.platform_data);
iounmap(iface->regs_base);
kfree(iface);

--
1.7.0.4

2012-05-16 07:31:31

by Zhang, Sonic

Subject: [PATCH 4/8] i2c: i2c-bfin-twi: Tighten condition when failing I2C transfer if MEN bit is reset unexpectedly.

From: Sonic Zhang <[email protected]>

Correct I2C transfer may fail because interrupt status bits XMTSERV and
RCVSERV are not checked when MCOMP interrupt is handled.

Signed-off-by: Sonic Zhang <[email protected]>
---
drivers/i2c/busses/i2c-bfin-twi.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index 4abaf59..63fb84f 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -201,7 +201,8 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
return;
}
if (twi_int_status & MCOMP) {
- if ((read_MASTER_CTL(iface) & MEN) == 0 &&
+ if (twi_int_status & (XMTSERV|RCVSERV) &&
+ (read_MASTER_CTL(iface) & MEN) == 0 &&
(iface->cur_mode == TWI_I2C_MODE_REPEAT ||
iface->cur_mode == TWI_I2C_MODE_COMBINED)) {
iface->result = -1;
--
1.7.0.4

2012-05-16 07:33:12

by Zhang, Sonic

Subject: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access Macro to head file.

From: Sonic Zhang <[email protected]>

Signed-off-by: Sonic Zhang <[email protected]>
---
drivers/i2c/busses/i2c-bfin-twi.c | 45 -------------------------------------
1 files changed, 0 insertions(+), 45 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index b2ca317..e6a0cba 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -33,51 +33,6 @@
#define TWI_I2C_MODE_COMBINED 3
#define TWI_I2C_MODE_REPEAT 4

-struct bfin_twi_iface {
- int irq;
- spinlock_t lock;
- char read_write;
- u8 command;
- u8 *transPtr;
- int readNum;
- int writeNum;
- int cur_mode;
- int manual_stop;
- int result;
- struct i2c_adapter adap;
- struct completion complete;
- struct i2c_msg *pmsg;
- int msg_num;
- int cur_msg;
- u16 saved_clkdiv;
- u16 saved_control;
- void __iomem *regs_base;
-};
-
-
-#define DEFINE_TWI_REG(reg, off) \
-static inline u16 read_##reg(struct bfin_twi_iface *iface) \
- { return bfin_read16(iface->regs_base + (off)); } \
-static inline void write_##reg(struct bfin_twi_iface *iface, u16 v) \
- { bfin_write16(iface->regs_base + (off), v); }
-
-DEFINE_TWI_REG(CLKDIV, 0x00)
-DEFINE_TWI_REG(CONTROL, 0x04)
-DEFINE_TWI_REG(SLAVE_CTL, 0x08)
-DEFINE_TWI_REG(SLAVE_STAT, 0x0C)
-DEFINE_TWI_REG(SLAVE_ADDR, 0x10)
-DEFINE_TWI_REG(MASTER_CTL, 0x14)
-DEFINE_TWI_REG(MASTER_STAT, 0x18)
-DEFINE_TWI_REG(MASTER_ADDR, 0x1C)
-DEFINE_TWI_REG(INT_STAT, 0x20)
-DEFINE_TWI_REG(INT_MASK, 0x24)
-DEFINE_TWI_REG(FIFO_CTL, 0x28)
-DEFINE_TWI_REG(FIFO_STAT, 0x2C)
-DEFINE_TWI_REG(XMT_DATA8, 0x80)
-DEFINE_TWI_REG(XMT_DATA16, 0x84)
-DEFINE_TWI_REG(RCV_DATA8, 0x88)
-DEFINE_TWI_REG(RCV_DATA16, 0x8C)
-
static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
unsigned short twi_int_status)
{
--
1.7.0.4

2012-05-16 07:33:35

by Zhang, Sonic

Subject: [PATCH 6/8] i2c:i2c-bfin-twi: include twi head file

From: Sonic Zhang <[email protected]>

TWI bit mask macros are moved to twi head file.

Signed-off-by: Sonic Zhang <[email protected]>
Signed-off-by: Bob Liu <[email protected]>
---
drivers/i2c/busses/i2c-bfin-twi.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index 594bbb1..d7e8fba 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -25,6 +25,7 @@
#include <asm/blackfin.h>
#include <asm/portmux.h>
#include <asm/irq.h>
+#include <asm/bfin_twi.h>

/* SMBus mode*/
#define TWI_I2C_MODE_STANDARD 1
--
1.7.0.4

2012-05-16 07:31:27

by Zhang, Sonic

Subject: [PATCH 3/8] i2c: i2c-bfin-twi: Break dead waiting loop if i2c device misbehaves.

From: Sonic Zhang <[email protected]>

Some fault i2c device may hold the sda/scl line and cause i2c driver
wait in the BUS busy loop. This patch breaks this potential dead
loop.

Signed-off-by: Sonic Zhang <[email protected]>

git-svn-id: svn://localhost/svn/linux-kernel/trunk@9291 526b6c2d-f592-4532-a319-5dd88ccb003d
---
drivers/i2c/busses/i2c-bfin-twi.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index 376e331..4abaf59 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -294,6 +294,8 @@ static irqreturn_t bfin_twi_interrupt_entry(int irq, void *dev_id)
return IRQ_HANDLED;
}

+#define BFIN_TWI_BUSY_TIMEOUT 1000
+
/*
* One i2c master transfer
*/
@@ -303,12 +305,16 @@ static int bfin_twi_do_master_xfer(struct i2c_adapter *adap,
struct bfin_twi_iface *iface = adap->algo_data;
struct i2c_msg *pmsg;
int rc = 0;
+ unsigned int busy_timeout = BFIN_TWI_BUSY_TIMEOUT;

if (!(read_CONTROL(iface) & TWI_ENA))
return -ENXIO;

- while (read_MASTER_STAT(iface) & BUSBUSY)
+ while (read_MASTER_STAT(iface) & BUSBUSY) {
+ if (--busy_timeout == 0)
+ return -EBUSY;
yield();
+ }

iface->pmsg = msgs;
iface->msg_num = num;
@@ -403,12 +409,16 @@ int bfin_twi_do_smbus_xfer(struct i2c_adapter *adap, u16 addr,
{
struct bfin_twi_iface *iface = adap->algo_data;
int rc = 0;
+ unsigned int busy_timeout = BFIN_TWI_BUSY_TIMEOUT;

if (!(read_CONTROL(iface) & TWI_ENA))
return -ENXIO;

- while (read_MASTER_STAT(iface) & BUSBUSY)
+ while (read_MASTER_STAT(iface) & BUSBUSY) {
+ if (--busy_timeout == 0)
+ return -EBUSY;
yield();
+ }

iface->writeNum = 0;
iface->readNum = 0;
--
1.7.0.4

2012-05-17 05:20:29

by Shubhrajyoti Datta

Subject: Re: [PATCH 3/8] i2c: i2c-bfin-twi: Break dead waiting loop if i2c device misbehaves.

Hello,

On Wed, May 16, 2012 at 12:50 PM, <[email protected]> wrote:
> From: Sonic Zhang <[email protected]>
>
> Some fault i2c device may hold the sda/scl line and cause i2c driver
> wait in the BUS busy loop. This patch breaks this potential dead
> loop.
How about using a non-busy loop while you are at it.

>
> Signed-off-by: Sonic Zhang <[email protected]>
>
> git-svn-id: svn://localhost/svn/linux-kernel/trunk@9291 526b6c2d-f592-4532-a319-5dd88ccb003d

2012-05-17 05:23:35

by Mike Frysinger

Subject: Re: [PATCH 3/8] i2c: i2c-bfin-twi: Break dead waiting loop if i2c device misbehaves.

On Thu, May 17, 2012 at 1:20 AM, Shubhrajyoti Datta
<[email protected]> wrote:
> On Wed, May 16, 2012 at 12:50 PM,  <[email protected]> wrote:
>> From: Sonic Zhang <[email protected]>
>>
>> Some fault i2c device may hold the sda/scl line and cause i2c driver
>> wait in the BUS busy loop. This patch breaks this potential dead
>> loop.
>
> How about using a non-busy loop while you are at it.

the timings here are fairly small ...
-mike

2012-05-17 10:51:27

by Shubhrajyoti Datta

Subject: Re: [PATCH 3/8] i2c: i2c-bfin-twi: Break dead waiting loop if i2c device misbehaves.

On Thu, May 17, 2012 at 10:53 AM, Mike Frysinger <[email protected]> wrote:
> On Thu, May 17, 2012 at 1:20 AM, Shubhrajyoti Datta
> <[email protected]> wrote:
>> On Wed, May 16, 2012 at 12:50 PM, ?<[email protected]> wrote:
>>> From: Sonic Zhang <[email protected]>
>>>
>>> Some fault i2c device may hold the sda/scl line and cause i2c driver
>>> wait in the BUS busy loop. This patch breaks this potential dead
>>> loop.
>>
>> How about using a non-busy loop while you are at it.
>
> the timings here are fairly small ...
OK no issues.
Anyways was just a suggestion.:-)

> -mike

2012-05-17 11:30:43

by Shubhrajyoti Datta

Subject: Re: [PATCH 4/8] i2c: i2c-bfin-twi: Tighten condition when failing I2C transfer if MEN bit is reset unexpectedly.

On Wed, May 16, 2012 at 12:50 PM, <[email protected]> wrote:
> From: Sonic Zhang <[email protected]>
>
> Correct I2C transfer may fail because interrupt status bits XMTSERV and
> RCVSERV are not checked when MCOMP interrupt is handled.
>
Could you expand MCOMP?

2012-05-18 02:41:13

by Zhang, Sonic

Subject: RE: [PATCH 4/8] i2c: i2c-bfin-twi: Tighten condition when failing I2C transfer if MEN bit is reset unexpectedly.

>-----Original Message-----
>From: Shubhrajyoti Datta [mailto:[email protected]]
>Sent: Thursday, May 17, 2012 7:31 PM
>To: Zhang, Sonic
>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
>[email protected]
>Subject: Re: [PATCH 4/8] i2c: i2c-bfin-twi: Tighten condition when failing I2C
>transfer if MEN bit is reset unexpectedly.
>
>On Wed, May 16, 2012 at 12:50 PM, <[email protected]> wrote:
>> From: Sonic Zhang <[email protected]>
>>
>> Correct I2C transfer may fail because interrupt status bits XMTSERV and
>> RCVSERV are not checked when MCOMP interrupt is handled.
>>
>Could you expand MCOMP?

Master transfer complete (MCOMP)
[1] The initiated master transfer has completed. In the absence of a
repeat start, the bus has been released.
[0] The completion of a transfer has not been detected.


Regards,

Sonic

2012-05-18 06:22:44

by Shubhrajyoti Datta

Subject: Re: [PATCH 4/8] i2c: i2c-bfin-twi: Tighten condition when failing I2C transfer if MEN bit is reset unexpectedly.

On Fri, May 18, 2012 at 8:11 AM, Zhang, Sonic <[email protected]> wrote:
>
>
>>-----Original Message-----
>>From: Shubhrajyoti Datta [mailto:[email protected]]
>>Sent: Thursday, May 17, 2012 7:31 PM
>>To: Zhang, Sonic
>>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
>>[email protected]
>>Subject: Re: [PATCH 4/8] i2c: i2c-bfin-twi: Tighten condition when failing I2C
>>transfer if MEN bit is reset unexpectedly.
>>
>>On Wed, May 16, 2012 at 12:50 PM, ?<[email protected]> wrote:
>>> From: Sonic Zhang <[email protected]>
>>>
>>> Correct I2C transfer may fail because interrupt status bits XMTSERV and
>>> RCVSERV are not checked when MCOMP interrupt is handled.
>>>
>>Could you expand MCOMP?
>
> Master transfer complete (MCOMP)
> [1] The initiated master transfer has completed. In the absence of a
> repeat start, the bus has been released.
> [0] The completion of a transfer has not been detected.
>

A trivial suggestion may be the changelog could be enhanced.
Thanks for the explanation.

>
> Regards,
>
> Sonic
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

2012-05-24 09:17:31

by Sonic Zhang

Subject: Re: [PATCH 1/8] i2c: i2c-bfin-twi: Illegal i2c bus lock upon certain transfer scenarios.

PING


On Wed, May 16, 2012 at 3:20 PM, <[email protected]> wrote:
> From: Michael Hennerich <[email protected]>
>
> For transfer counts > 255 bytes i2c-bfin-twi sets the data
> transfer counter DCNT to 0xFF indicating unlimited transfers.
> It then uses a flag iface->manual_stop to manually issue the STOP
> condition, once the required amount of bytes are received.
>
> We found that on I2C receive operation issuing the STOP condition
> together with a FULL RCV FIFO (2bytes) will cause SDA and SCL be
> constantly driven low.
>
> Temporary workaround until further investigation:
> Discard the RCV FIFO before issuing the STOP condition.
>
> Signed-off-by: Michael Hennerich <[email protected]>
> Signed-off-by: Sonic Zhang <[email protected]>
>
> git-svn-id: svn://localhost/svn/linux-kernel/trunk@9011 526b6c2d-f592-4532-a319-5dd88ccb003d
> ---
> ?drivers/i2c/busses/i2c-bfin-twi.c | ? ?4 ++++
> ?1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index cdb59e5..e6dd3c9 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -131,6 +131,10 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> ? ? ? ? ? ? ? ? ? ? ? ?iface->transPtr++;
> ? ? ? ? ? ? ? ? ? ? ? ?iface->readNum--;
> ? ? ? ? ? ? ? ?} else if (iface->manual_stop) {
> + ? ? ? ? ? ? ? ? ? ? ? /* Avoid possible bus stall -
> + ? ? ? ? ? ? ? ? ? ? ? ?* Flush FIFO before issuing the STOP condition
> + ? ? ? ? ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? ? ? ? ? read_RCV_DATA16(iface);
> ? ? ? ? ? ? ? ? ? ? ? ?write_MASTER_CTL(iface,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?read_MASTER_CTL(iface) | STOP);
> ? ? ? ? ? ? ? ?} else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

2012-05-24 09:18:14

by Sonic Zhang

Subject: Re: [PATCH 2/8] i2c: i2c-bfin-twi: Stop receiving operation immediately in last rx interrupt.

PING

On Wed, May 16, 2012 at 3:20 PM, <[email protected]> wrote:
> From: Sonic Zhang <[email protected]>
>
> Also wake up waiting process when transfer completes.
>
> Signed-off-by: Sonic Zhang <[email protected]>
>
> git-svn-id: svn://localhost/svn/linux-kernel/trunk@9105 526b6c2d-f592-4532-a319-5dd88ccb003d
> ---
> ?drivers/i2c/busses/i2c-bfin-twi.c | ? 43 ++++++++++++++++++++----------------
> ?1 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index e6dd3c9..376e331 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -130,21 +130,25 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ? ? ? ? ?iface->transPtr++;
> ? ? ? ? ? ? ? ? ? ? ? ?iface->readNum--;
> - ? ? ? ? ? ? ? } else if (iface->manual_stop) {
> - ? ? ? ? ? ? ? ? ? ? ? /* Avoid possible bus stall -
> - ? ? ? ? ? ? ? ? ? ? ? ?* Flush FIFO before issuing the STOP condition
> - ? ? ? ? ? ? ? ? ? ? ? ?*/
> - ? ? ? ? ? ? ? ? ? ? ? read_RCV_DATA16(iface);
> - ? ? ? ? ? ? ? ? ? ? ? write_MASTER_CTL(iface,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? read_MASTER_CTL(iface) | STOP);
> - ? ? ? ? ? ? ? } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> - ? ? ? ? ? ? ? ? ? ? ? ? ?iface->cur_msg + 1 < iface->msg_num) {
> - ? ? ? ? ? ? ? ? ? ? ? if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? write_MASTER_CTL(iface,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? read_MASTER_CTL(iface) | RSTART | MDIR);
> - ? ? ? ? ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? if (iface->readNum == 0) {
> + ? ? ? ? ? ? ? ? ? ? ? if (iface->manual_stop) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? /* Avoid possible bus stall -
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* Flush FIFO before issuing the STOP condition
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? read_RCV_DATA16(iface);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?write_MASTER_CTL(iface,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (read_MASTER_CTL(iface) | RSTART) & ~MDIR);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? read_MASTER_CTL(iface) | STOP);
> + ? ? ? ? ? ? ? ? ? ? ? } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? iface->cur_msg + 1 < iface->msg_num) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? write_MASTER_CTL(iface,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? read_MASTER_CTL(iface) | RSTART | MDIR);
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? write_MASTER_CTL(iface,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (read_MASTER_CTL(iface) | RSTART) & ~MDIR);
> + ? ? ? ? ? ? ? ? ? ? ? }
> ? ? ? ? ? ? ? ?}
> ? ? ? ?}
> ? ? ? ?if (twi_int_status & MERR) {
> @@ -245,12 +249,13 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?}
> ? ? ? ? ? ? ? ? ? ? ? ?}
>
> - ? ? ? ? ? ? ? ? ? ? ? if (iface->pmsg[iface->cur_msg].len <= 255)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? write_MASTER_CTL(iface,
> + ? ? ? ? ? ? ? ? ? ? ? if (iface->pmsg[iface->cur_msg].len <= 255) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? write_MASTER_CTL(iface,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(read_MASTER_CTL(iface) &
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(~(0xff << 6))) |
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (iface->pmsg[iface->cur_msg].len << 6));
> - ? ? ? ? ? ? ? ? ? ? ? else {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (iface->pmsg[iface->cur_msg].len << 6));
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? iface->manual_stop = 0;
> + ? ? ? ? ? ? ? ? ? ? ? } else {
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?write_MASTER_CTL(iface,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(read_MASTER_CTL(iface) |
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(0xff << 6)));
> @@ -264,8 +269,8 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> ? ? ? ? ? ? ? ? ? ? ? ?write_INT_MASK(iface, 0);
> ? ? ? ? ? ? ? ? ? ? ? ?write_MASTER_CTL(iface, 0);
> ? ? ? ? ? ? ? ?}
> + ? ? ? ? ? ? ? complete(&iface->complete);
> ? ? ? ?}
> - ? ? ? complete(&iface->complete);
> ?}
>
> ?/* Interrupt handler */
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

2012-05-24 09:18:36

by Sonic Zhang

Subject: Re: [PATCH 6/8] i2c:i2c-bfin-twi: include twi head file

PING

On Wed, May 16, 2012 at 3:20 PM, <[email protected]> wrote:
> From: Sonic Zhang <[email protected]>
>
> TWI bit mask macros are moved to twi head file.
>
> Signed-off-by: Sonic Zhang <[email protected]>
> Signed-off-by: Bob Liu <[email protected]>
> ---
> ?drivers/i2c/busses/i2c-bfin-twi.c | ? ?1 +
> ?1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 594bbb1..d7e8fba 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -25,6 +25,7 @@
> ?#include <asm/blackfin.h>
> ?#include <asm/portmux.h>
> ?#include <asm/irq.h>
> +#include <asm/bfin_twi.h>
>
> ?/* SMBus mode*/
> ?#define TWI_I2C_MODE_STANDARD ? ? ? ? ?1
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

2012-05-24 09:19:12

by Sonic Zhang

Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access Macro to head file.

PING

On Wed, May 16, 2012 at 3:20 PM, <[email protected]> wrote:
> From: Sonic Zhang <[email protected]>
>
> Signed-off-by: Sonic Zhang <[email protected]>
> ---
> ?drivers/i2c/busses/i2c-bfin-twi.c | ? 45 -------------------------------------
> ?1 files changed, 0 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index b2ca317..e6a0cba 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -33,51 +33,6 @@
> ?#define TWI_I2C_MODE_COMBINED ? ? ? ? ?3
> ?#define TWI_I2C_MODE_REPEAT ? ? ? ? ? ?4
>
> -struct bfin_twi_iface {
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? irq;
> - ? ? ? spinlock_t ? ? ? ? ? ? ?lock;
> - ? ? ? char ? ? ? ? ? ? ? ? ? ?read_write;
> - ? ? ? u8 ? ? ? ? ? ? ? ? ? ? ?command;
> - ? ? ? u8 ? ? ? ? ? ? ? ? ? ? ?*transPtr;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? readNum;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? writeNum;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? cur_mode;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? manual_stop;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? result;
> - ? ? ? struct i2c_adapter ? ? ?adap;
> - ? ? ? struct completion ? ? ? complete;
> - ? ? ? struct i2c_msg ? ? ? ? ?*pmsg;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? msg_num;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? cur_msg;
> - ? ? ? u16 ? ? ? ? ? ? ? ? ? ? saved_clkdiv;
> - ? ? ? u16 ? ? ? ? ? ? ? ? ? ? saved_control;
> - ? ? ? void __iomem ? ? ? ? ? ?*regs_base;
> -};
> -
> -
> -#define DEFINE_TWI_REG(reg, off) \
> -static inline u16 read_##reg(struct bfin_twi_iface *iface) \
> - ? ? ? { return bfin_read16(iface->regs_base + (off)); } \
> -static inline void write_##reg(struct bfin_twi_iface *iface, u16 v) \
> - ? ? ? { bfin_write16(iface->regs_base + (off), v); }
> -
> -DEFINE_TWI_REG(CLKDIV, 0x00)
> -DEFINE_TWI_REG(CONTROL, 0x04)
> -DEFINE_TWI_REG(SLAVE_CTL, 0x08)
> -DEFINE_TWI_REG(SLAVE_STAT, 0x0C)
> -DEFINE_TWI_REG(SLAVE_ADDR, 0x10)
> -DEFINE_TWI_REG(MASTER_CTL, 0x14)
> -DEFINE_TWI_REG(MASTER_STAT, 0x18)
> -DEFINE_TWI_REG(MASTER_ADDR, 0x1C)
> -DEFINE_TWI_REG(INT_STAT, 0x20)
> -DEFINE_TWI_REG(INT_MASK, 0x24)
> -DEFINE_TWI_REG(FIFO_CTL, 0x28)
> -DEFINE_TWI_REG(FIFO_STAT, 0x2C)
> -DEFINE_TWI_REG(XMT_DATA8, 0x80)
> -DEFINE_TWI_REG(XMT_DATA16, 0x84)
> -DEFINE_TWI_REG(RCV_DATA8, 0x88)
> -DEFINE_TWI_REG(RCV_DATA16, 0x8C)
> -
> ?static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned short twi_int_status)
> ?{
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

2012-05-25 09:48:22

by Shubhrajyoti Datta

Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access Macro to head file.

Hi ,
Some minor comments/ doubts.
On Wed, May 16, 2012 at 12:50 PM, <[email protected]> wrote:
> From: Sonic Zhang <[email protected]>
>

A small description may be helpful.
Also the $SUBJECT says move the patch looks more like the remove.
Am I missing something.

> Signed-off-by: Sonic Zhang <[email protected]>
> ---
> ?drivers/i2c/busses/i2c-bfin-twi.c | ? 45 -------------------------------------
> ?1 files changed, 0 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index b2ca317..e6a0cba 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -33,51 +33,6 @@
> ?#define TWI_I2C_MODE_COMBINED ? ? ? ? ?3
> ?#define TWI_I2C_MODE_REPEAT ? ? ? ? ? ?4
>
> -struct bfin_twi_iface {
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? irq;
> - ? ? ? spinlock_t ? ? ? ? ? ? ?lock;
> - ? ? ? char ? ? ? ? ? ? ? ? ? ?read_write;
> - ? ? ? u8 ? ? ? ? ? ? ? ? ? ? ?command;
> - ? ? ? u8 ? ? ? ? ? ? ? ? ? ? ?*transPtr;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? readNum;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? writeNum;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? cur_mode;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? manual_stop;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? result;
> - ? ? ? struct i2c_adapter ? ? ?adap;
> - ? ? ? struct completion ? ? ? complete;
> - ? ? ? struct i2c_msg ? ? ? ? ?*pmsg;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? msg_num;
> - ? ? ? int ? ? ? ? ? ? ? ? ? ? cur_msg;
> - ? ? ? u16 ? ? ? ? ? ? ? ? ? ? saved_clkdiv;
> - ? ? ? u16 ? ? ? ? ? ? ? ? ? ? saved_control;
> - ? ? ? void __iomem ? ? ? ? ? ?*regs_base;
> -};
> -
> -
> -#define DEFINE_TWI_REG(reg, off) \
> -static inline u16 read_##reg(struct bfin_twi_iface *iface) \
> - ? ? ? { return bfin_read16(iface->regs_base + (off)); } \
> -static inline void write_##reg(struct bfin_twi_iface *iface, u16 v) \
> - ? ? ? { bfin_write16(iface->regs_base + (off), v); }
> -
> -DEFINE_TWI_REG(CLKDIV, 0x00)
> -DEFINE_TWI_REG(CONTROL, 0x04)
> -DEFINE_TWI_REG(SLAVE_CTL, 0x08)
> -DEFINE_TWI_REG(SLAVE_STAT, 0x0C)
> -DEFINE_TWI_REG(SLAVE_ADDR, 0x10)
> -DEFINE_TWI_REG(MASTER_CTL, 0x14)
> -DEFINE_TWI_REG(MASTER_STAT, 0x18)
> -DEFINE_TWI_REG(MASTER_ADDR, 0x1C)
> -DEFINE_TWI_REG(INT_STAT, 0x20)
> -DEFINE_TWI_REG(INT_MASK, 0x24)
> -DEFINE_TWI_REG(FIFO_CTL, 0x28)
> -DEFINE_TWI_REG(FIFO_STAT, 0x2C)
> -DEFINE_TWI_REG(XMT_DATA8, 0x80)
> -DEFINE_TWI_REG(XMT_DATA16, 0x84)
> -DEFINE_TWI_REG(RCV_DATA8, 0x88)
> -DEFINE_TWI_REG(RCV_DATA16, 0x8C)
> -
> ?static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned short twi_int_status)
> ?{
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/

2012-05-25 09:54:08

by Zhang, Sonic

Subject: RE: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access Macro to head file.

>-----Original Message-----
>From: Shubhrajyoti Datta [mailto:[email protected]]
>Sent: Friday, May 25, 2012 5:48 PM
>To: Zhang, Sonic
>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
>[email protected]
>Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access
>Macro to head file.
>
>Hi ,
>Some minor comments/ doubts.
>On Wed, May 16, 2012 at 12:50 PM, <[email protected]> wrote:
>> From: Sonic Zhang <[email protected]>
>>
>
>A small description may be helpful.
>Also the $SUBJECT says move the patch looks more like the remove.
>Am I missing something.

The subject says these definitions are moved to a head file by the other patch for Blackfin architecture.

Regards,

Sonic

>
>> Signed-off-by: Sonic Zhang <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-bfin-twi.c | 45 -------------------------------------
>> 1 files changed, 0 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
>> index b2ca317..e6a0cba 100644
>> --- a/drivers/i2c/busses/i2c-bfin-twi.c
>> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
>> @@ -33,51 +33,6 @@
>> #define TWI_I2C_MODE_COMBINED 3
>> #define TWI_I2C_MODE_REPEAT 4
>>
>> -struct bfin_twi_iface {
>> - int irq;
>> - spinlock_t lock;
>> - char read_write;
>> - u8 command;
>> - u8 *transPtr;
>> - int readNum;
>> - int writeNum;
>> - int cur_mode;
>> - int manual_stop;
>> - int result;
>> - struct i2c_adapter adap;
>> - struct completion complete;
>> - struct i2c_msg *pmsg;
>> - int msg_num;
>> - int cur_msg;
>> - u16 saved_clkdiv;
>> - u16 saved_control;
>> - void __iomem *regs_base;
>> -};
>> -
>> -
>> -#define DEFINE_TWI_REG(reg, off) \
>> -static inline u16 read_##reg(struct bfin_twi_iface *iface) \
>> - { return bfin_read16(iface->regs_base + (off)); } \
>> -static inline void write_##reg(struct bfin_twi_iface *iface, u16 v) \
>> - { bfin_write16(iface->regs_base + (off), v); }
>> -
>> -DEFINE_TWI_REG(CLKDIV, 0x00)
>> -DEFINE_TWI_REG(CONTROL, 0x04)
>> -DEFINE_TWI_REG(SLAVE_CTL, 0x08)
>> -DEFINE_TWI_REG(SLAVE_STAT, 0x0C)
>> -DEFINE_TWI_REG(SLAVE_ADDR, 0x10)
>> -DEFINE_TWI_REG(MASTER_CTL, 0x14)
>> -DEFINE_TWI_REG(MASTER_STAT, 0x18)
>> -DEFINE_TWI_REG(MASTER_ADDR, 0x1C)
>> -DEFINE_TWI_REG(INT_STAT, 0x20)
>> -DEFINE_TWI_REG(INT_MASK, 0x24)
>> -DEFINE_TWI_REG(FIFO_CTL, 0x28)
>> -DEFINE_TWI_REG(FIFO_STAT, 0x2C)
>> -DEFINE_TWI_REG(XMT_DATA8, 0x80)
>> -DEFINE_TWI_REG(XMT_DATA16, 0x84)
>> -DEFINE_TWI_REG(RCV_DATA8, 0x88)
>> -DEFINE_TWI_REG(RCV_DATA16, 0x8C)
>> -
>> static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
>> unsigned short twi_int_status)
>> {
>> --
>> 1.7.0.4
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/

2012-05-25 10:10:36

by Shubhrajyoti Datta

Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access Macro to head file.

Hi ,
On Fri, May 25, 2012 at 3:24 PM, Zhang, Sonic <[email protected]> wrote:
>
>
>>-----Original Message-----
>>From: Shubhrajyoti Datta [mailto:[email protected]]
>>Sent: Friday, May 25, 2012 5:48 PM
>>To: Zhang, Sonic
>>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
>>[email protected]
>>Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access
>>Macro to head file.
>>
>>Hi ,
>>Some minor comments/ doubts.
>>On Wed, May 16, 2012 at 12:50 PM, ?<[email protected]> wrote:
>>> From: Sonic Zhang <[email protected]>
>>>
>>
>>A small description may be helpful.
>>Also the $SUBJECT says move the patch looks more like the remove.
>>Am I missing something.
>
> The subject says these definitions are moved to a head file by the other patch for Blackfin architecture.

Got it thanks.

You may want to add a dependency otherwise it might lead to a compilation break?


>
> Regards,
>
> Sonic

2012-05-25 10:12:00

by Zhang, Sonic

Subject: RE: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access Macro to head file.

>-----Original Message-----
>From: Shubhrajyoti Datta [mailto:[email protected]]
>Sent: Friday, May 25, 2012 6:10 PM
>To: Zhang, Sonic
>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
>[email protected]
>Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access
>Macro to head file.
>
>Hi ,
>On Fri, May 25, 2012 at 3:24 PM, Zhang, Sonic <[email protected]>
>wrote:
>>
>>
>>>-----Original Message-----
>>>From: Shubhrajyoti Datta [mailto:[email protected]]
>>>Sent: Friday, May 25, 2012 5:48 PM
>>>To: Zhang, Sonic
>>>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
>>>[email protected]
>>>Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access
>>>Macro to head file.
>>>
>>>Hi ,
>>>Some minor comments/ doubts.
>>>On Wed, May 16, 2012 at 12:50 PM, <[email protected]> wrote:
>>>> From: Sonic Zhang <[email protected]>
>>>>
>>>
>>>A small description may be helpful.
>>>Also the $SUBJECT says move the patch looks more like the remove.
>>>Am I missing something.
>>
>> The subject says these definitions are moved to a head file by the other patch
>for Blackfin architecture.
>
>Got it thanks.
>
>You may want to add a dependency otherwise it might lead to a compilation break?
>

That patch for Blackfin architecture has already been merged into mainline.

Regards,

Sonic

>
>>
>> Regards,
>>
>> Sonic

2012-05-25 10:16:09

by Shubhrajyoti Datta

Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access Macro to head file.

On Fri, May 25, 2012 at 3:42 PM, Zhang, Sonic <[email protected]> wrote:
>
>
>>-----Original Message-----
>>From: Shubhrajyoti Datta [mailto:[email protected]]
>>Sent: Friday, May 25, 2012 6:10 PM
>>To: Zhang, Sonic
>>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
>>[email protected]
>>Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access
>>Macro to head file.
>>
>>Hi ,
>>On Fri, May 25, 2012 at 3:24 PM, Zhang, Sonic <[email protected]>
>>wrote:
>>>
>>>
>>>>-----Original Message-----
>>>>From: Shubhrajyoti Datta [mailto:[email protected]]
>>>>Sent: Friday, May 25, 2012 5:48 PM
>>>>To: Zhang, Sonic
>>>>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
>>>>[email protected]
>>>>Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access
>>>>Macro to head file.
>>>>
>>>>Hi ,
>>>>Some minor comments/ doubts.
>>>>On Wed, May 16, 2012 at 12:50 PM, ?<[email protected]> wrote:
>>>>> From: Sonic Zhang <[email protected]>
>>>>>
>>>>
>>>>A small description may be helpful.
>>>>Also the $SUBJECT says move the patch looks more like the remove.
>>>>Am I missing something.
>>>
>>> The subject says these definitions are moved to a head file by the other patch
>>for Blackfin architecture.
>>
>>Got it thanks.
>>
>>You may want to add a dependency otherwise it might lead to a compilation break?
>>
>
> That patch for Blackfin architecture has already been merged into mainline.
Great,

Apologies for the noise.

2012-06-11 16:24:57

by Wolfram Sang

Subject: Re: [PATCH 1/8] i2c: i2c-bfin-twi: Illegal i2c bus lock upon certain transfer scenarios.

On Wed, May 16, 2012 at 03:20:19PM +0800, [email protected] wrote:
> From: Michael Hennerich <[email protected]>
>
> For transfer counts > 255 bytes i2c-bfin-twi sets the data
> transfer counter DCNT to 0xFF indicating unlimited transfers.
> It then uses a flag iface->manual_stop to manually issue the STOP
> condition, once the required amount of bytes are received.
>
> We found that on I2C receive operation issuing the STOP condition
> together with a FULL RCV FIFO (2bytes) will cause SDA and SCL be
> constantly driven low.
>
> Temporary workaround until further investigation:
> Discard the RCV FIFO before issuing the STOP condition.
>
> Signed-off-by: Michael Hennerich <[email protected]>
> Signed-off-by: Sonic Zhang <[email protected]>
>
> git-svn-id: svn://localhost/svn/linux-kernel/trunk@9011 526b6c2d-f592-4532-a319-5dd88ccb003d

Please remove this line.

> ---
> drivers/i2c/busses/i2c-bfin-twi.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index cdb59e5..e6dd3c9 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -131,6 +131,10 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> iface->transPtr++;
> iface->readNum--;
> } else if (iface->manual_stop) {
> + /* Avoid possible bus stall -
> + * Flush FIFO before issuing the STOP condition
> + */
> + read_RCV_DATA16(iface);

Please mention that this is currently only a workaround, so people know
there could be still a proper solution.

> write_MASTER_CTL(iface,
> read_MASTER_CTL(iface) | STOP);
> } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> --
> 1.7.0.4
>
>

--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Attachments:

2012-06-11 16:28:14

by Wolfram Sang

Subject: Re: [PATCH 2/8] i2c: i2c-bfin-twi: Stop receiving operation immediately in last rx interrupt.

On Wed, May 16, 2012 at 03:20:20PM +0800, [email protected] wrote:
> From: Sonic Zhang <[email protected]>
>
> Also wake up waiting process when transfer completes.

The description says what you do, but not why it is needed. What bug was
experienced and why is this change proper? Patch 5/8 does that quite
good.

>
> Signed-off-by: Sonic Zhang <[email protected]>
>
> git-svn-id: svn://localhost/svn/linux-kernel/trunk@9105 526b6c2d-f592-4532-a319-5dd88ccb003d

Please remove this line.

> ---
> drivers/i2c/busses/i2c-bfin-twi.c | 43 ++++++++++++++++++++----------------
> 1 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index e6dd3c9..376e331 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -130,21 +130,25 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> }
> iface->transPtr++;
> iface->readNum--;
> - } else if (iface->manual_stop) {
> - /* Avoid possible bus stall -
> - * Flush FIFO before issuing the STOP condition
> - */
> - read_RCV_DATA16(iface);
> - write_MASTER_CTL(iface,
> - read_MASTER_CTL(iface) | STOP);
> - } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> - iface->cur_msg + 1 < iface->msg_num) {
> - if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD)
> - write_MASTER_CTL(iface,
> - read_MASTER_CTL(iface) | RSTART | MDIR);
> - else
> + }
> +
> + if (iface->readNum == 0) {
> + if (iface->manual_stop) {
> + /* Avoid possible bus stall -
> + * Flush FIFO before issuing the STOP condition
> + */
> + read_RCV_DATA16(iface);
> write_MASTER_CTL(iface,
> - (read_MASTER_CTL(iface) | RSTART) & ~MDIR);
> + read_MASTER_CTL(iface) | STOP);
> + } else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
> + iface->cur_msg + 1 < iface->msg_num) {
> + if (iface->pmsg[iface->cur_msg + 1].flags & I2C_M_RD)
> + write_MASTER_CTL(iface,
> + read_MASTER_CTL(iface) | RSTART | MDIR);
> + else
> + write_MASTER_CTL(iface,
> + (read_MASTER_CTL(iface) | RSTART) & ~MDIR);
> + }
> }
> }
> if (twi_int_status & MERR) {
> @@ -245,12 +249,13 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> }
> }
>
> - if (iface->pmsg[iface->cur_msg].len <= 255)
> - write_MASTER_CTL(iface,
> + if (iface->pmsg[iface->cur_msg].len <= 255) {
> + write_MASTER_CTL(iface,
> (read_MASTER_CTL(iface) &
> (~(0xff << 6))) |
> - (iface->pmsg[iface->cur_msg].len << 6));
> - else {
> + (iface->pmsg[iface->cur_msg].len << 6));
> + iface->manual_stop = 0;
> + } else {
> write_MASTER_CTL(iface,
> (read_MASTER_CTL(iface) |
> (0xff << 6)));
> @@ -264,8 +269,8 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> write_INT_MASK(iface, 0);
> write_MASTER_CTL(iface, 0);
> }
> + complete(&iface->complete);
> }
> - complete(&iface->complete);
> }
>
> /* Interrupt handler */
> --
> 1.7.0.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

--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Attachments:

2012-06-11 16:30:51

by Wolfram Sang

Subject: Re: [PATCH 3/8] i2c: i2c-bfin-twi: Break dead waiting loop if i2c device misbehaves.

On Wed, May 16, 2012 at 03:20:21PM +0800, [email protected] wrote:
> From: Sonic Zhang <[email protected]>
>
> Some fault i2c device may hold the sda/scl line and cause i2c driver
> wait in the BUS busy loop. This patch breaks this potential dead
> loop.
>
> Signed-off-by: Sonic Zhang <[email protected]>
>
> git-svn-id: svn://localhost/svn/linux-kernel/trunk@9291 526b6c2d-f592-4532-a319-5dd88ccb003d

^

> ---
> drivers/i2c/busses/i2c-bfin-twi.c | 14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 376e331..4abaf59 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -294,6 +294,8 @@ static irqreturn_t bfin_twi_interrupt_entry(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +#define BFIN_TWI_BUSY_TIMEOUT 1000
> +

Please use a unit like microseconds for the timeout.

> /*
> * One i2c master transfer
> */
> @@ -303,12 +305,16 @@ static int bfin_twi_do_master_xfer(struct i2c_adapter *adap,
> struct bfin_twi_iface *iface = adap->algo_data;
> struct i2c_msg *pmsg;
> int rc = 0;
> + unsigned int busy_timeout = BFIN_TWI_BUSY_TIMEOUT;
>
> if (!(read_CONTROL(iface) & TWI_ENA))
> return -ENXIO;
>
> - while (read_MASTER_STAT(iface) & BUSBUSY)
> + while (read_MASTER_STAT(iface) & BUSBUSY) {
> + if (--busy_timeout == 0)
> + return -EBUSY;
> yield();
> + }
>
> iface->pmsg = msgs;
> iface->msg_num = num;
> @@ -403,12 +409,16 @@ int bfin_twi_do_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> {
> struct bfin_twi_iface *iface = adap->algo_data;
> int rc = 0;
> + unsigned int busy_timeout = BFIN_TWI_BUSY_TIMEOUT;
>
> if (!(read_CONTROL(iface) & TWI_ENA))
> return -ENXIO;
>
> - while (read_MASTER_STAT(iface) & BUSBUSY)
> + while (read_MASTER_STAT(iface) & BUSBUSY) {
> + if (--busy_timeout == 0)
> + return -EBUSY;
> yield();
> + }
>
> iface->writeNum = 0;
> iface->readNum = 0;
> --
> 1.7.0.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

--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Attachments:

2012-06-11 16:34:04

by Wolfram Sang

Subject: Re: [PATCH 4/8] i2c: i2c-bfin-twi: Tighten condition when failing I2C transfer if MEN bit is reset unexpectedly.

On Thu, May 17, 2012 at 05:00:40PM +0530, Shubhrajyoti Datta wrote:
> On Wed, May 16, 2012 at 12:50 PM, <[email protected]> wrote:
> > From: Sonic Zhang <[email protected]>
> >
> > Correct I2C transfer may fail because interrupt status bits XMTSERV and
> > RCVSERV are not checked when MCOMP interrupt is handled.
> >
> Could you expand MCOMP?

I agree. It is nice to understand patch descriptions without needing to
know all the datasheet terminology, so please rework the description a
little.

--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Attachments:

2012-06-11 16:34:50

by Wolfram Sang

Subject: Re: [PATCH 6/8] i2c:i2c-bfin-twi: include twi head file

On Wed, May 16, 2012 at 03:20:24PM +0800, [email protected] wrote:
> From: Sonic Zhang <[email protected]>
>
> TWI bit mask macros are moved to twi head file.
>
> Signed-off-by: Sonic Zhang <[email protected]>
> Signed-off-by: Bob Liu <[email protected]>

Please state the dependency if any, or the commit if already merged.

> ---
> drivers/i2c/busses/i2c-bfin-twi.c | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 594bbb1..d7e8fba 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -25,6 +25,7 @@
> #include <asm/blackfin.h>
> #include <asm/portmux.h>
> #include <asm/irq.h>
> +#include <asm/bfin_twi.h>
>
> /* SMBus mode*/
> #define TWI_I2C_MODE_STANDARD 1
> --
> 1.7.0.4
>
>

--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Attachments:

2012-06-11 16:35:22

by Wolfram Sang

Subject: Re: [PATCH 7/8] i2c: i2c-bfin-twi: Move TWI peripheral pin request array to platform data.

On Wed, May 16, 2012 at 03:20:25PM +0800, [email protected] wrote:
> From: Sonic Zhang <[email protected]>
>
> Signed-off-by: Sonic Zhang <[email protected]>

Again, please state the dependency.

> ---
> drivers/i2c/busses/i2c-bfin-twi.c | 12 ++++--------
> 1 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index d7e8fba..b2ca317 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -78,11 +78,6 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
> DEFINE_TWI_REG(RCV_DATA8, 0x88)
> DEFINE_TWI_REG(RCV_DATA16, 0x8C)
>
> -static const u16 pin_req[2][3] = {
> - {P_TWI0_SCL, P_TWI0_SDA, 0},
> - {P_TWI1_SCL, P_TWI1_SDA, 0},
> -};
> -
> static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface,
> unsigned short twi_int_status)
> {
> @@ -719,7 +714,8 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
> p_adap->timeout = 5 * HZ;
> p_adap->retries = 3;
>
> - rc = peripheral_request_list(pin_req[pdev->id], "i2c-bfin-twi");
> + rc = peripheral_request_list((unsigned short *)pdev->dev.platform_data,
> + "i2c-bfin-twi");
> if (rc) {
> dev_err(&pdev->dev, "Can't setup pin mux!\n");
> goto out_error_pin_mux;
> @@ -766,7 +762,7 @@ out_error_add_adapter:
> free_irq(iface->irq, iface);
> out_error_req_irq:
> out_error_no_irq:
> - peripheral_free_list(pin_req[pdev->id]);
> + peripheral_free_list((unsigned short *)pdev->dev.platform_data);
> out_error_pin_mux:
> iounmap(iface->regs_base);
> out_error_ioremap:
> @@ -784,7 +780,7 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)
>
> i2c_del_adapter(&(iface->adap));
> free_irq(iface->irq, iface);
> - peripheral_free_list(pin_req[pdev->id]);
> + peripheral_free_list((unsigned short *)pdev->dev.platform_data);
> iounmap(iface->regs_base);
> kfree(iface);
>
> --
> 1.7.0.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

--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Attachments:

2012-06-11 16:36:50

by Wolfram Sang

Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access Macro to head file.

On Fri, May 25, 2012 at 03:46:06PM +0530, Shubhrajyoti Datta wrote:
> On Fri, May 25, 2012 at 3:42 PM, Zhang, Sonic <[email protected]> wrote:
> >
> >
> >>-----Original Message-----
> >>From: Shubhrajyoti Datta [mailto:[email protected]]
> >>Sent: Friday, May 25, 2012 6:10 PM
> >>To: Zhang, Sonic
> >>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
> >>[email protected]
> >>Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access
> >>Macro to head file.
> >>
> >>Hi ,
> >>On Fri, May 25, 2012 at 3:24 PM, Zhang, Sonic <[email protected]>
> >>wrote:
> >>>
> >>>
> >>>>-----Original Message-----
> >>>>From: Shubhrajyoti Datta [mailto:[email protected]]
> >>>>Sent: Friday, May 25, 2012 5:48 PM
> >>>>To: Zhang, Sonic
> >>>>Cc: Ben Dooks; Wolfram Sang; [email protected]; LKML; uclinux-dist-
> >>>>[email protected]
> >>>>Subject: Re: [PATCH 8/8] i2c: i2c-bfin-twi: Move blackfin TWI register access
> >>>>Macro to head file.
> >>>>
> >>>>Hi ,
> >>>>Some minor comments/ doubts.
> >>>>On Wed, May 16, 2012 at 12:50 PM, ?<[email protected]> wrote:
> >>>>> From: Sonic Zhang <[email protected]>
> >>>>>
> >>>>
> >>>>A small description may be helpful.
> >>>>Also the $SUBJECT says move the patch looks more like the remove.
> >>>>Am I missing something.
> >>>
> >>> The subject says these definitions are moved to a head file by the other patch
> >>for Blackfin architecture.
> >>
> >>Got it thanks.
> >>
> >>You may want to add a dependency otherwise it might lead to a compilation break?
> >>
> >
> > That patch for Blackfin architecture has already been merged into mainline.
> Great,
>
> Apologies for the noise.

I don't think this is noise, I agree with you. I'd like to see the
commit which added that and the reasoning. Usually, encapsulating the
accesses in the driver is good, because no one else should bother.

Thanks,

Wolfram

--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Attachments:

2012-06-13 07:47:54

by Zhang, Sonic

Subject: RE: [PATCH 3/8] i2c: i2c-bfin-twi: Break dead waiting loop if i2c device misbehaves.

>-----Original Message-----
>From: Wolfram Sang [mailto:[email protected]]
>Sent: Tuesday, June 12, 2012 12:31 AM
>To: Zhang, Sonic
>Cc: Ben Dooks; [email protected]; LKML; uclinux-dist-
>[email protected]
>Subject: Re: [PATCH 3/8] i2c: i2c-bfin-twi: Break dead waiting loop if i2c device
>misbehaves.
>
>On Wed, May 16, 2012 at 03:20:21PM +0800, [email protected] wrote:
>> From: Sonic Zhang <[email protected]>
>>
>> Some fault i2c device may hold the sda/scl line and cause i2c driver
>> wait in the BUS busy loop. This patch breaks this potential dead loop.
>>
>> Signed-off-by: Sonic Zhang <[email protected]>
>>
>> git-svn-id: svn://localhost/svn/linux-kernel/trunk@9291
>> 526b6c2d-f592-4532-a319-5dd88ccb003d
>
>^
>
>> ---
>> drivers/i2c/busses/i2c-bfin-twi.c | 14 ++++++++++++--
>> 1 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c
>> b/drivers/i2c/busses/i2c-bfin-twi.c
>> index 376e331..4abaf59 100644
>> --- a/drivers/i2c/busses/i2c-bfin-twi.c
>> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
>> @@ -294,6 +294,8 @@ static irqreturn_t bfin_twi_interrupt_entry(int irq, void
>*dev_id)
>> return IRQ_HANDLED;
>> }
>>
>> +#define BFIN_TWI_BUSY_TIMEOUT 1000
>> +
>
>Please use a unit like microseconds for the timeout.
>

Take a look at the I2C framework code again. I find this loop to poll BUSBUSY bit is not necessary. Just return -EAGAIN, the framework do retry before timeout already.

Regards,

Sonic


>> /*
>> * One i2c master transfer
>> */
>> @@ -303,12 +305,16 @@ static int bfin_twi_do_master_xfer(struct i2c_adapter
>*adap,
>> struct bfin_twi_iface *iface = adap->algo_data;
>> struct i2c_msg *pmsg;
>> int rc = 0;
>> + unsigned int busy_timeout = BFIN_TWI_BUSY_TIMEOUT;
>>
>> if (!(read_CONTROL(iface) & TWI_ENA))
>> return -ENXIO;
>>
>> - while (read_MASTER_STAT(iface) & BUSBUSY)
>> + while (read_MASTER_STAT(iface) & BUSBUSY) {
>> + if (--busy_timeout == 0)
>> + return -EBUSY;
>> yield();
>> + }
>>
>> iface->pmsg = msgs;
>> iface->msg_num = num;
>> @@ -403,12 +409,16 @@ int bfin_twi_do_smbus_xfer(struct i2c_adapter
>> *adap, u16 addr, {
>> struct bfin_twi_iface *iface = adap->algo_data;
>> int rc = 0;
>> + unsigned int busy_timeout = BFIN_TWI_BUSY_TIMEOUT;
>>
>> if (!(read_CONTROL(iface) & TWI_ENA))
>> return -ENXIO;
>>
>> - while (read_MASTER_STAT(iface) & BUSBUSY)
>> + while (read_MASTER_STAT(iface) & BUSBUSY) {
>> + if (--busy_timeout == 0)
>> + return -EBUSY;
>> yield();
>> + }
>>
>> iface->writeNum = 0;
>> iface->readNum = 0;
>> --
>> 1.7.0.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
>
>--
>Pengutronix e.K. | Wolfram Sang |
>Industrial Linux Solutions | http://www.pengutronix.de/ |