Skip to contentSkip to conversation

Blackfin I2C/TWI driver: Add repeat start feature to avoid break of a bundle of i2c master xfer operation.

PATCH In review Bryan Wu 3 participants 5 messages Mar 12, 2008 – Mar 26, 2008 4 messages not in the archive I2C subsystem host drivers

lore
Share & tools
lore

Bryan Wu

author
#0 lore raw

PATCH 1/6 Blackfin I2C/TWI driver: Add repeat start feature to avoid break of a bundle of i2c master xfer operation.

From: Sonic Zhang <[email protected]>

- Create a new mode TWI_I2C_MODE_REPEAT.
- No change to smbus operation.

Signed-off-by Sonic Zhang <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
Signed-off-by Bryan Wu <[email protected]>
---
 drivers/i2c/busses/i2c-bfin-twi.c |  183 +++++++++++++++++++++++--------------
 1 files changed, 113 insertions(+), 70 deletions(-)
drivers/i2c/busses/i2c-bfin-twi.c +111 -68
index 7dbdaeb..ea15125 100644
@@ -42,6 +42,7 @@
 #define TWI_I2C_MODE_STANDARD		0x01
 #define TWI_I2C_MODE_STANDARDSUB	0x02
 #define TWI_I2C_MODE_COMBINED		0x04
+#define TWI_I2C_MODE_REPEAT		0x08
 
 struct bfin_twi_iface {
 	int			irq;

@@ -58,6 +59,9 @@ struct bfin_twi_iface {
 	struct timer_list	timeout_timer;
 	struct i2c_adapter	adap;
 	struct completion	complete;
+	struct i2c_msg 		*pmsg;
+	int			msg_num;
+	int			cur_msg;
 };
 
 static struct bfin_twi_iface twi_iface;

@@ -76,12 +80,16 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 		/* start receive immediately after complete sending in
 		 * combine mode.
 		 */
-		else if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
+		else if (iface->cur_mode == TWI_I2C_MODE_COMBINED)
 			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
 				| MDIR | RSTART);
-		} else if (iface->manual_stop)
+		else if (iface->manual_stop)
 			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
 				| STOP);
+		else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
+				iface->cur_msg+1 < iface->msg_num)
+			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
+				| RSTART);
 		SSYNC();
 		/* Clear status */
 		bfin_write_TWI_INT_STAT(XMTSERV);

@@ -108,6 +116,11 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
 				| STOP);
 			SSYNC();
+		} else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
+				iface->cur_msg+1 < iface->msg_num) {
+			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
+				| RSTART);
+			SSYNC();
 		}
 		/* Clear interrupt source */
 		bfin_write_TWI_INT_STAT(RCVSERV);

@@ -119,7 +132,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 		bfin_write_TWI_MASTER_STAT(0x3e);
 		bfin_write_TWI_MASTER_CTL(0);
 		SSYNC();
-		iface->result = -1;
+		iface->result = -EIO;
 		/* if both err and complete int stats are set, return proper
 		 * results.
 		 */

@@ -170,6 +183,42 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() |
 				MEN | MDIR);
 			SSYNC();
+		} else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
+				iface->cur_msg+1 < iface->msg_num) {
+			iface->cur_msg++;
+			iface->transPtr = iface->pmsg[iface->cur_msg].buf;
+			iface->writeNum = iface->readNum =
+				iface->pmsg[iface->cur_msg].len;
+			/* Set Transmit device address */
+			bfin_write_TWI_MASTER_ADDR(
+				iface->pmsg[iface->cur_msg].addr);
+			if (iface->pmsg[iface->cur_msg].flags & I2C_M_RD)
+				iface->read_write = I2C_SMBUS_READ;
+			else {
+				iface->read_write = I2C_SMBUS_WRITE;
+				/* Transmit first data */
+				if (iface->writeNum > 0) {
+					bfin_write_TWI_XMT_DATA8(
+						*(iface->transPtr++));
+					iface->writeNum--;
+					SSYNC();
+				}
+			}
+
+			if (iface->pmsg[iface->cur_msg].len <= 255)
+				bfin_write_TWI_MASTER_CTL(
+				iface->pmsg[iface->cur_msg].len << 6);
+			else if (iface->pmsg[iface->cur_msg].len > 255) {
+				bfin_write_TWI_MASTER_CTL(0xff << 6);
+				iface->manual_stop = 1;
+			}
+			/* remove restart bit and enable master receive */
+			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() &
+				~RSTART);
+			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() |
+				MEN | ((iface->read_write == I2C_SMBUS_READ) ?
+				MDIR : 0));
+			SSYNC();
 		} else {
 			iface->result = 1;
 			bfin_write_TWI_INT_MASK(0);

@@ -221,7 +270,6 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
 {
 	struct bfin_twi_iface *iface = adap->algo_data;
 	struct i2c_msg *pmsg;
-	int i, ret;
 	int rc = 0;
 
 	if (!(bfin_read_TWI_CONTROL() & TWI_ENA))

@@ -231,81 +279,76 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
 		yield();
 	}
 
-	ret = 0;
-	for (i = 0; rc >= 0 && i < num; i++) {
-		pmsg = &msgs[i];
-		if (pmsg->flags & I2C_M_TEN) {
-			dev_err(&(adap->dev), "i2c-bfin-twi: 10 bits addr "
-				"not supported !\n");
-			rc = -EINVAL;
-			break;
-		}
+	iface->pmsg = msgs;
+	iface->msg_num = num;
+	iface->cur_msg = 0;
 
-		iface->cur_mode = TWI_I2C_MODE_STANDARD;
-		iface->manual_stop = 0;
-		iface->transPtr = pmsg->buf;
-		iface->writeNum = iface->readNum = pmsg->len;
-		iface->result = 0;
-		iface->timeout_count = 10;
-		/* Set Transmit device address */
-		bfin_write_TWI_MASTER_ADDR(pmsg->addr);
-
-		/* FIFO Initiation. Data in FIFO should be
-		 *  discarded before start a new operation.
-		 */
-		bfin_write_TWI_FIFO_CTL(0x3);
-		SSYNC();
-		bfin_write_TWI_FIFO_CTL(0);
-		SSYNC();
+	pmsg = &msgs[0];
+	if (pmsg->flags & I2C_M_TEN) {
+		dev_err(&adap->dev, "10 bits addr not supported!\n");
+		return -EINVAL;
+	}
 
-		if (pmsg->flags & I2C_M_RD)
-			iface->read_write = I2C_SMBUS_READ;
-		else {
-			iface->read_write = I2C_SMBUS_WRITE;
-			/* Transmit first data */
-			if (iface->writeNum > 0) {
-				bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
-				iface->writeNum--;
-				SSYNC();
-			}
+	iface->cur_mode = TWI_I2C_MODE_REPEAT;
+	iface->manual_stop = 0;
+	iface->transPtr = pmsg->buf;
+	iface->writeNum = iface->readNum = pmsg->len;
+	iface->result = 0;
+	iface->timeout_count = 10;
+	/* Set Transmit device address */
+	bfin_write_TWI_MASTER_ADDR(pmsg->addr);
+
+	/* FIFO Initiation. Data in FIFO should be
+	 *  discarded before start a new operation.
+	 */
+	bfin_write_TWI_FIFO_CTL(0x3);
+	SSYNC();
+	bfin_write_TWI_FIFO_CTL(0);
+	SSYNC();
+
+	if (pmsg->flags & I2C_M_RD)
+		iface->read_write = I2C_SMBUS_READ;
+	else {
+		iface->read_write = I2C_SMBUS_WRITE;
+		/* Transmit first data */
+		if (iface->writeNum > 0) {
+			bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
+			iface->writeNum--;
+			SSYNC();
 		}
+	}
 
-		/* clear int stat */
-		bfin_write_TWI_INT_STAT(MERR|MCOMP|XMTSERV|RCVSERV);
+	/* clear int stat */
+	bfin_write_TWI_INT_STAT(MERR | MCOMP | XMTSERV | RCVSERV);
 
-		/* Interrupt mask . Enable XMT, RCV interrupt */
-		bfin_write_TWI_INT_MASK(MCOMP | MERR |
-			((iface->read_write == I2C_SMBUS_READ)?
-			RCVSERV : XMTSERV));
-		SSYNC();
+	/* Interrupt mask . Enable XMT, RCV interrupt */
+	bfin_write_TWI_INT_MASK(MCOMP | MERR | RCVSERV | XMTSERV);
+	SSYNC();
 
-		if (pmsg->len > 0 && pmsg->len <= 255)
-			bfin_write_TWI_MASTER_CTL(pmsg->len << 6);
-		else if (pmsg->len > 255) {
-			bfin_write_TWI_MASTER_CTL(0xff << 6);
-			iface->manual_stop = 1;
-		} else
-			break;
+	if (pmsg->len <= 255)
+		bfin_write_TWI_MASTER_CTL(pmsg->len << 6);
+	else if (pmsg->len > 255) {
+		bfin_write_TWI_MASTER_CTL(0xff << 6);
+		iface->manual_stop = 1;
+	}
 
-		iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
-		add_timer(&iface->timeout_timer);
+	iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
+	add_timer(&iface->timeout_timer);
 
-		/* Master enable */
-		bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
-			((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
-			((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
-		SSYNC();
+	/* Master enable */
+	bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
+		((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
+		((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0));
+	SSYNC();
 
-		wait_for_completion(&iface->complete);
+	wait_for_completion(&iface->complete);
 
-		rc = iface->result;
-		if (rc == 1)
-			ret++;
-		else if (rc == -1)
-			break;
-	}
+	rc = iface->result;
 
-	return ret;
+	if (rc == 1)
+		return num;
+	else
+		return rc;
 }
 
 /*
Signature · 7 lines
1.5.4.3


_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): 1205479360-25240-2-git-send-email-cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

#2 lore raw

Hi Bryan,

Quoted 8 lines · Bryan Wu

From: Sonic Zhang <[email protected]>

- Create a new mode TWI_I2C_MODE_REPEAT.
- No change to smbus operation.

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

Just one Signed-off-by per person, please.

drivers/i2c/busses/i2c-bfin-twi.c

Quoted 9 lines
---
 drivers/i2c/busses/i2c-bfin-twi.c |  183 +++++++++++++++++++++++--------------
 1 files changed, 113 insertions(+), 70 deletions(-)
drivers/i2c/busses/i2c-bfin-twi.c +0 -0
index 7dbdaeb..ea15125 100644
(...)

It seems that this patch is the exact same as the one you posted back in November 2007:
http://lists.lm-sensors.org/pipermail/i2c/2007-November/002119.html

I did review this patch:
http://lists.lm-sensors.org/pipermail/i2c/2007-November/002121.html

So I would appreciate if you could take my comments into account and post an updated version.

Thanks,

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw

Hi Jean,

I have already discussed your questions with you in attached email. See bellow. The twi controller is designed to support SMBus protocol in hardware. That's why we implement bfin_twi_smbus_xfer().

Sonic
=====================================
from Jean Delvare <[email protected]>
to Sonic Zhang <[email protected]>,
date Wed, Nov 14, 2007 at 9:20 PM
subject Re: [PATCH 1/4] Blackfin I2C/TWI driver: Add repeat start
feature to avoid break of a bundle of i2c master xfer operation.

hide details 11/14/07

Reply

Hi Sonic,
- Hide quoted text -

Quoted 20 lines · Sonic Zhang

TWI_I2C_MODE_REPEAT:
static int bfin_twi_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);

From transaction 1 to N do simple read or write
{
start,
read or write,
address,
data,
if Not N
{ restart }
else
{ stop }
}

TWI_I2C_MODE_COMBINED:
static int bfin_twi_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num);

Quoted 14 lines

Write then read transactions in SMBus
{
start,
write,
address,
data,
restart,
read,
address,
data,
stop.
- Hide quoted text -
}

OK, thanks for the clarification. The above looks OK to me, if that's what the driver implements then there's no problem. However, please realize that TWI_I2C_MODE_COMBINED is a subset of TWI_I2C_MODE_REPEAT (with N = 2, first message is a write and second message is a read.) This means that TWI_I2C_MODE_COMBINED is not strictly needed and could be implemented using TWI_I2C_MODE_REPEAT. As a matter of fact most I2C master drivers in Linux do that, i.e. they implement .master_xfer and not .smbus_xfer. i2c-core can emulate .smbus_xfer from .master_xfer.

I don't know the i2c-bfin-twi driver in details so there might be a reason why you implemented both methods that I don't understand, but from my point of view, you should be able to delete bfin_twi_smbus_xfer() and your driver would still work the exact same as it does today. The advantage would be a smaller driver, so a lower risk to have hidden bugs.

Thanks,
--
Jean Delvare
Reply

Forward

Sonic Zhang to Jean

show details 11/15/07

Reply

from Sonic Zhang <[email protected]>
to Jean Delvare <[email protected]>,
date Thu, Nov 15, 2007 at 10:50 AM
subject Re: [PATCH 1/4] Blackfin I2C/TWI driver: Add repeat start
feature to avoid break of a bundle of i2c master xfer operation.
mailed-by gmail.com

hide details 11/15/07

Reply

Thank you for your great advises.

Sonic

-----Original Message-----
From: Jean Delvare [mailto:[email protected]]
Sent: Tuesday, March 25, 2008 2:21 AM
To: Bryan Wu
Cc: [email protected]; Sonic Zhang
Subject: Re: [PATCH 1/6] Blackfin I2C/TWI driver: Add repeat start feature to avoid break of a bundle of i2c master xfer operation.

Hi Bryan,

Quoted 8 lines · Bryan Wu

From: Sonic Zhang <[email protected]>

- Create a new mode TWI_I2C_MODE_REPEAT.
- No change to smbus operation.

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

Just one Signed-off-by per person, please.

drivers/i2c/busses/i2c-bfin-twi.c

Quoted 11 lines
---
 drivers/i2c/busses/i2c-bfin-twi.c |  183 
+++++++++++++++++++++++--------------
 1 files changed, 113 insertions(+), 70 deletions(-)
drivers/i2c/busses/i2c-bfin-twi.c +0 -0
index 7dbdaeb..ea15125 100644
(...)

It seems that this patch is the exact same as the one you posted back in November 2007:
http://lists.lm-sensors.org/pipermail/i2c/2007-November/002119.html

I did review this patch:
http://lists.lm-sensors.org/pipermail/i2c/2007-November/002121.html

So I would appreciate if you could take my comments into account and post an updated version.

Thanks,

A message that is not in the archive (sent off-list or not yet received): 0F1B54C89D5F954D8535DB252AF412FA01B17710-SGdA1W8gREmuVPpjEGsWsTcYPEmu4y7e@public.gmane.org

#6 lore raw

Hi Sonic,

Quoted 3 lines · Sonic

I have already discussed your questions with you in attached email. See bellow. The twi controller is designed to support SMBus protocol in hardware. That's why we implement bfin_twi_smbus_xfer().

This is news to me, you did not tell this to me before. If there are hardware optimizations for SMBus then indeed it makes sense to have different code in the driver for I2C support and SMBus support.

Still, I had several objections to the patch that was submitted in November 2007 (and resubmitted unchanged one week ago). I really would like to see them addressed.

Thanks,

Signature · 6 lines
Jean Delvare

_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw

Hi Jean,

I just fixed rest issues in the twi driver according to your last comments in
http://lists.lm-sensors.org/pipermail/i2c/2007-November/002121.html . Bryan will send you the latest patch.

Thank you for your suggestion.

Sonic Zhang

-----Original Message-----
From: Jean Delvare [mailto:[email protected]]
Sent: Tuesday, March 25, 2008 10:04 PM
To: Zhang, Sonic
Cc: Bryan Wu; [email protected]
Subject: Re: [PATCH 1/6] Blackfin I2C/TWI driver: Add repeat start feature to avoid break of a bundle of i2c master xfer operation.

Hi Sonic,

Quoted 3 lines · Sonic

I have already discussed your questions with you in attached email. See bellow. The twi controller is designed to support SMBus protocol in hardware. That's why we implement bfin_twi_smbus_xfer().

This is news to me, you did not tell this to me before. If there are hardware optimizations for SMBus then indeed it makes sense to have different code in the driver for I2C support and SMBus support.

Still, I had several objections to the patch that was submitted in November 2007 (and resubmitted unchanged one week ago). I really would like to see them addressed.

Thanks,