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

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

[Bryan Wu]

From: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

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

Signed-off-by: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Wu <bryan.wu-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bryan Wu <cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/i2c/busses/i2c-bfin-twi.c |  183 +++++++++++++++++++++++--------------
 1 files changed, 113 insertions(+), 70 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index 7dbdaeb..ea15125 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -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;
 }
 
 /*
-- 
1.5.4.3


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi Bryan,

On Fri, 14 Mar 2008 00:22:35 -0700, Bryan Wu wrote:
> From: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
>  - Create a new mode TWI_I2C_MODE_REPEAT.
>  - No change to smbus operation.
> 
> Signed-off-by: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Wu <bryan.wu-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Wu <cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

> ---
>  drivers/i2c/busses/i2c-bfin-twi.c |  183 +++++++++++++++++++++++--------------
>  1 files changed, 113 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 7dbdaeb..ea15125 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> (...)

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,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Zhang, Sonic]

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 <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
to	Sonic Zhang <sonic.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
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 -

On Thu, 8 Nov 2007 13:14:56 +0800, Sonic Zhang wrote:
> 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);
>
> 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 <sonic.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
to	Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>,
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:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org] 
Sent: Tuesday, March 25, 2008 2:21 AM
To: Bryan Wu
Cc: i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org; 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,

On Fri, 14 Mar 2008 00:22:35 -0700, Bryan Wu wrote:
> From: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
>  - Create a new mode TWI_I2C_MODE_REPEAT.
>  - No change to smbus operation.
> 
> Signed-off-by: Sonic Zhang <sonic.zhang-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Wu <bryan.wu-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bryan Wu <cooloney-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

> ---
>  drivers/i2c/busses/i2c-bfin-twi.c |  183 
> +++++++++++++++++++++++--------------
>  1 files changed, 113 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c 
> b/drivers/i2c/busses/i2c-bfin-twi.c
> index 7dbdaeb..ea15125 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> (...)

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,

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

[Jean Delvare]

Hi Sonic,

On Tue, 25 Mar 2008 11:01:18 +0800, Zhang, Sonic wrote:
> 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,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Zhang, Sonic]

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:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org] 
Sent: Tuesday, March 25, 2008 10:04 PM
To: Zhang, Sonic
Cc: Bryan Wu; i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
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,

On Tue, 25 Mar 2008 11:01:18 +0800, Zhang, Sonic wrote:
> 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,