[PATCH 0/4] Blackfin I2C/TWI driver updates and bug fixing according to Jean's review

[PATCH 0/4] Blackfin I2C/TWI driver updates and bug fixing according to Jean's review

[Bryan Wu]

Still remain in old I2C driver interface. I plan to move to new style I2C API recently.
Also intend to using new TWI register accessor functions as Jean's suggestions.
Then move that static pin_req setting to our Blackfin board files.

But currently version is OK and tested on Blackfin board with I2C devices.

[PATCH 1/4] 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@analog.com>

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

Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
Signed-off-by: Bryan Wu <bryan.wu@analog.com>
---
 drivers/i2c/busses/i2c-bfin-twi.c |  179 +++++++++++++++++++++++--------------
 1 files changed, 111 insertions(+), 68 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index 67224a4..6535852 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.3.4

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

[Jean Delvare]

Hi Bryan, 

Thanks for splitting the patches as requested. Here's a more complete
review:

On Fri,  2 Nov 2007 14:24:21 +0800, Bryan Wu wrote:
> From: Sonic Zhang <sonic.zhang@analog.com>
> 
>  - Create a new mode TWI_I2C_MODE_REPEAT.
>  - No change to smbus operation.

I don't understand the difference between TWI_I2C_MODE_COMBINED and
TWI_I2C_MODE_REPEAT. Both use RSTART to send multiple data chunks at
once. Can you please explain the difference?

> 
> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
> Signed-off-by: Bryan Wu <bryan.wu@analog.com>
> ---
>  drivers/i2c/busses/i2c-bfin-twi.c |  179 +++++++++++++++++++++++--------------
>  1 files changed, 111 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 67224a4..6535852 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

This is strange (and possibly confusing) to use values that make the
mode look like a bitfield, when all the modes are actually mutually
exclusive and can't be combined. Admittedly these values are arbitrary
and there's no bug, but it would still make more sense to number the
modes linearly.

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

This test will always succeed, so a simple "else" is enough.

> +				bfin_write_TWI_MASTER_CTL(0xff << 6);
> +				iface->manual_stop = 1;
> +			}

Does this mean that the Blackfin TWI controller doesn't support
messages of more than 255 bytes? If so, you should return an error
right away when this happens, rather than silently truncating the
message.

> +			/* 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;
> +	}

Same as above: if you can't achieve the transaction, return an error.

>  
> -		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;
>  }
>  
>  /*


-- 
Jean Delvare

[PATCH 2/4] Blackfin I2C/TWI driver: Add platform_resource interface to support multi-port TWI controllers

[Bryan Wu]

 - Dynamic alloc the resource of TWI driver data according to board information
 - TWI register read/write accessor based on dynamic regs_base
 - Support TWI0/TWI1 for BF54x

Signed-off-by: Bryan Wu <bryan.wu@analog.com>
---
 drivers/i2c/busses/i2c-bfin-twi.c |  269 +++++++++++++++++++++++--------------
 1 files changed, 166 insertions(+), 103 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index 6535852..e495454 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -62,43 +62,66 @@ struct bfin_twi_iface {
 	struct i2c_msg 		*pmsg;
 	int			msg_num;
 	int			cur_msg;
+	void __iomem		*regs_base;
 };
 
-static struct bfin_twi_iface twi_iface;
+
+#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 = bfin_read_TWI_INT_STAT();
-	unsigned short mast_stat = bfin_read_TWI_MASTER_STAT();
+	unsigned short twi_int_status = read_INT_STAT(iface);
+	unsigned short mast_stat = read_MASTER_STAT(iface);
 
 	if (twi_int_status & XMTSERV) {
 		/* Transmit next data */
 		if (iface->writeNum > 0) {
-			bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
+			write_XMT_DATA8(iface, *(iface->transPtr++));
 			iface->writeNum--;
 		}
 		/* start receive immediately after complete sending in
 		 * combine mode.
 		 */
 		else if (iface->cur_mode == TWI_I2C_MODE_COMBINED)
-			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
-				| MDIR | RSTART);
+			write_MASTER_CTL(iface,
+				read_MASTER_CTL(iface) | MDIR | RSTART);
 		else if (iface->manual_stop)
-			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
-				| STOP);
+			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)
-			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
-				| RSTART);
+			write_MASTER_CTL(iface,
+				read_MASTER_CTL(iface) | RSTART);
 		SSYNC();
 		/* Clear status */
-		bfin_write_TWI_INT_STAT(XMTSERV);
+		write_INT_STAT(iface, XMTSERV);
 		SSYNC();
 	}
 	if (twi_int_status & RCVSERV) {
 		if (iface->readNum > 0) {
 			/* Receive next data */
-			*(iface->transPtr) = bfin_read_TWI_RCV_DATA8();
+			*(iface->transPtr) = read_RCV_DATA8(iface);
 			if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
 				/* Change combine mode into sub mode after
 				 * read first data.
@@ -113,33 +136,33 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 			iface->transPtr++;
 			iface->readNum--;
 		} else if (iface->manual_stop) {
-			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
-				| STOP);
+			write_MASTER_CTL(iface,
+				read_MASTER_CTL(iface) | 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);
+			write_MASTER_CTL(iface,
+				read_MASTER_CTL(iface) | RSTART);
 			SSYNC();
 		}
 		/* Clear interrupt source */
-		bfin_write_TWI_INT_STAT(RCVSERV);
+		write_INT_STAT(iface, RCVSERV);
 		SSYNC();
 	}
 	if (twi_int_status & MERR) {
-		bfin_write_TWI_INT_STAT(MERR);
-		bfin_write_TWI_INT_MASK(0);
-		bfin_write_TWI_MASTER_STAT(0x3e);
-		bfin_write_TWI_MASTER_CTL(0);
+		write_INT_STAT(iface, MERR);
+		write_INT_MASK(iface, 0);
+		write_MASTER_STAT(iface, 0x3e);
+		write_MASTER_CTL(iface, 0);
 		SSYNC();
 		iface->result = -EIO;
 		/* if both err and complete int stats are set, return proper
 		 * results.
 		 */
 		if (twi_int_status & MCOMP) {
-			bfin_write_TWI_INT_STAT(MCOMP);
-			bfin_write_TWI_INT_MASK(0);
-			bfin_write_TWI_MASTER_CTL(0);
+			write_INT_STAT(iface, MCOMP);
+			write_INT_MASK(iface, 0);
+			write_MASTER_CTL(iface, 0);
 			SSYNC();
 			/* If it is a quick transfer, only address bug no data,
 			 * not an err, return 1.
@@ -156,7 +179,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 		return;
 	}
 	if (twi_int_status & MCOMP) {
-		bfin_write_TWI_INT_STAT(MCOMP);
+		write_INT_STAT(iface, MCOMP);
 		SSYNC();
 		if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
 			if (iface->readNum == 0) {
@@ -165,23 +188,22 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 				 */
 				iface->readNum = 1;
 				iface->manual_stop = 1;
-				bfin_write_TWI_MASTER_CTL(
-					bfin_read_TWI_MASTER_CTL()
-					| (0xff << 6));
+				write_MASTER_CTL(iface,
+					read_MASTER_CTL(iface) | (0xff << 6));
 			} else {
 				/* set the readd number in other
 				 * combine mode.
 				 */
-				bfin_write_TWI_MASTER_CTL(
-					(bfin_read_TWI_MASTER_CTL() &
+				write_MASTER_CTL(iface,
+					(read_MASTER_CTL(iface) &
 					(~(0xff << 6))) |
-					( iface->readNum << 6));
+					(iface->readNum << 6));
 			}
 			/* 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 | MDIR);
+			write_MASTER_CTL(iface,
+				read_MASTER_CTL(iface) & ~RSTART);
+			write_MASTER_CTL(iface,
+				read_MASTER_CTL(iface) | MEN | MDIR);
 			SSYNC();
 		} else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
 				iface->cur_msg+1 < iface->msg_num) {
@@ -190,7 +212,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 			iface->writeNum = iface->readNum =
 				iface->pmsg[iface->cur_msg].len;
 			/* Set Transmit device address */
-			bfin_write_TWI_MASTER_ADDR(
+			write_MASTER_ADDR(iface,
 				iface->pmsg[iface->cur_msg].addr);
 			if (iface->pmsg[iface->cur_msg].flags & I2C_M_RD)
 				iface->read_write = I2C_SMBUS_READ;
@@ -198,7 +220,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 				iface->read_write = I2C_SMBUS_WRITE;
 				/* Transmit first data */
 				if (iface->writeNum > 0) {
-					bfin_write_TWI_XMT_DATA8(
+					write_XMT_DATA8(iface,
 						*(iface->transPtr++));
 					iface->writeNum--;
 					SSYNC();
@@ -206,23 +228,23 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 			}
 
 			if (iface->pmsg[iface->cur_msg].len <= 255)
-				bfin_write_TWI_MASTER_CTL(
+				write_MASTER_CTL(iface,
 				iface->pmsg[iface->cur_msg].len << 6);
 			else if (iface->pmsg[iface->cur_msg].len > 255) {
-				bfin_write_TWI_MASTER_CTL(0xff << 6);
+				write_MASTER_CTL(iface, 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() |
+			write_MASTER_CTL(iface,
+				read_MASTER_CTL(iface) & ~RSTART);
+			write_MASTER_CTL(iface, read_MASTER_CTL(iface) |
 				MEN | ((iface->read_write == I2C_SMBUS_READ) ?
 				MDIR : 0));
 			SSYNC();
 		} else {
 			iface->result = 1;
-			bfin_write_TWI_INT_MASK(0);
-			bfin_write_TWI_MASTER_CTL(0);
+			write_INT_MASK(iface, 0);
+			write_MASTER_CTL(iface, 0);
 			SSYNC();
 			complete(&iface->complete);
 		}
@@ -272,12 +294,11 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
 	struct i2c_msg *pmsg;
 	int rc = 0;
 
-	if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
+	if (!(read_CONTROL(iface) & TWI_ENA))
 		return -ENXIO;
 
-	while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
+	while (read_MASTER_STAT(iface) & BUSBUSY)
 		yield();
-	}
 
 	iface->pmsg = msgs;
 	iface->msg_num = num;
@@ -296,14 +317,14 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
 	iface->result = 0;
 	iface->timeout_count = 10;
 	/* Set Transmit device address */
-	bfin_write_TWI_MASTER_ADDR(pmsg->addr);
+	write_MASTER_ADDR(iface, pmsg->addr);
 
 	/* FIFO Initiation. Data in FIFO should be
 	 *  discarded before start a new operation.
 	 */
-	bfin_write_TWI_FIFO_CTL(0x3);
+	write_FIFO_CTL(iface, 0x3);
 	SSYNC();
-	bfin_write_TWI_FIFO_CTL(0);
+	write_FIFO_CTL(iface, 0);
 	SSYNC();
 
 	if (pmsg->flags & I2C_M_RD)
@@ -312,23 +333,23 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
 		iface->read_write = I2C_SMBUS_WRITE;
 		/* Transmit first data */
 		if (iface->writeNum > 0) {
-			bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
+			write_XMT_DATA8(iface, *(iface->transPtr++));
 			iface->writeNum--;
 			SSYNC();
 		}
 	}
 
 	/* clear int stat */
-	bfin_write_TWI_INT_STAT(MERR | MCOMP | XMTSERV | RCVSERV);
+	write_INT_STAT(iface, MERR | MCOMP | XMTSERV | RCVSERV);
 
 	/* Interrupt mask . Enable XMT, RCV interrupt */
-	bfin_write_TWI_INT_MASK(MCOMP | MERR | RCVSERV | XMTSERV);
+	write_INT_MASK(iface, MCOMP | MERR | RCVSERV | XMTSERV);
 	SSYNC();
 
 	if (pmsg->len <= 255)
-		bfin_write_TWI_MASTER_CTL(pmsg->len << 6);
+		write_MASTER_CTL(iface, pmsg->len << 6);
 	else if (pmsg->len > 255) {
-		bfin_write_TWI_MASTER_CTL(0xff << 6);
+		write_MASTER_CTL(iface, 0xff << 6);
 		iface->manual_stop = 1;
 	}
 
@@ -336,7 +357,7 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
 	add_timer(&iface->timeout_timer);
 
 	/* Master enable */
-	bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
+	write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
 		((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
 		((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0));
 	SSYNC();
@@ -362,12 +383,11 @@ int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
 	struct bfin_twi_iface *iface = adap->algo_data;
 	int rc = 0;
 
-	if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
+	if (!(read_CONTROL(iface) & TWI_ENA))
 		return -ENXIO;
 
-	while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
+	while (read_MASTER_STAT(iface) & BUSBUSY)
 		yield();
-	}
 
 	iface->writeNum = 0;
 	iface->readNum = 0;
@@ -439,15 +459,15 @@ int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
 	/* FIFO Initiation. Data in FIFO should be discarded before
 	 * start a new operation.
 	 */
-	bfin_write_TWI_FIFO_CTL(0x3);
+	write_FIFO_CTL(iface, 0x3);
 	SSYNC();
-	bfin_write_TWI_FIFO_CTL(0);
+	write_FIFO_CTL(iface, 0);
 
 	/* clear int stat */
-	bfin_write_TWI_INT_STAT(MERR|MCOMP|XMTSERV|RCVSERV);
+	write_INT_STAT(iface, MERR | MCOMP | XMTSERV | RCVSERV);
 
 	/* Set Transmit device address */
-	bfin_write_TWI_MASTER_ADDR(addr);
+	write_MASTER_ADDR(iface, addr);
 	SSYNC();
 
 	iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
@@ -455,60 +475,64 @@ int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
 
 	switch (iface->cur_mode) {
 	case TWI_I2C_MODE_STANDARDSUB:
-		bfin_write_TWI_XMT_DATA8(iface->command);
-		bfin_write_TWI_INT_MASK(MCOMP | MERR |
+		write_XMT_DATA8(iface, iface->command);
+		write_INT_MASK(iface, MCOMP | MERR |
 			((iface->read_write == I2C_SMBUS_READ) ?
 			RCVSERV : XMTSERV));
 		SSYNC();
 
 		if (iface->writeNum + 1 <= 255)
-			bfin_write_TWI_MASTER_CTL((iface->writeNum + 1) << 6);
+			write_MASTER_CTL(iface, (iface->writeNum + 1) << 6);
 		else {
-			bfin_write_TWI_MASTER_CTL(0xff << 6);
+			write_MASTER_CTL(iface, 0xff << 6);
 			iface->manual_stop = 1;
 		}
 		/* Master enable */
-		bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
+		write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
 			((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
 		break;
 	case TWI_I2C_MODE_COMBINED:
-		bfin_write_TWI_XMT_DATA8(iface->command);
-		bfin_write_TWI_INT_MASK(MCOMP | MERR | RCVSERV | XMTSERV);
+		write_XMT_DATA8(iface, iface->command);
+		write_INT_MASK(iface, MCOMP | MERR | RCVSERV | XMTSERV);
 		SSYNC();
 
 		if (iface->writeNum > 0)
-			bfin_write_TWI_MASTER_CTL((iface->writeNum + 1) << 6);
+			write_MASTER_CTL(iface, (iface->writeNum + 1) << 6);
 		else
-			bfin_write_TWI_MASTER_CTL(0x1 << 6);
+			write_MASTER_CTL(iface, 0x1 << 6);
 		/* Master enable */
-		bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
+		write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
 			((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
 		break;
 	default:
-		bfin_write_TWI_MASTER_CTL(0);
+		write_MASTER_CTL(iface, 0);
 		if (size != I2C_SMBUS_QUICK) {
 			/* Don't access xmit data register when this is a
 			 * read operation.
 			 */
 			if (iface->read_write != I2C_SMBUS_READ) {
 				if (iface->writeNum > 0) {
-					bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
+					write_XMT_DATA8(iface,
+						*(iface->transPtr++));
 					if (iface->writeNum <= 255)
-						bfin_write_TWI_MASTER_CTL(iface->writeNum << 6);
+						write_MASTER_CTL(iface,
+							iface->writeNum << 6);
 					else {
-						bfin_write_TWI_MASTER_CTL(0xff << 6);
+						write_MASTER_CTL(iface,
+							0xff << 6);
 						iface->manual_stop = 1;
 					}
 					iface->writeNum--;
 				} else {
-					bfin_write_TWI_XMT_DATA8(iface->command);
-					bfin_write_TWI_MASTER_CTL(1 << 6);
+					write_XMT_DATA8(iface, iface->command);
+					write_MASTER_CTL(iface, 1 << 6);
 				}
 			} else {
 				if (iface->readNum > 0 && iface->readNum <= 255)
-					bfin_write_TWI_MASTER_CTL(iface->readNum << 6);
+					write_MASTER_CTL(iface,
+						iface->readNum << 6);
 				else if (iface->readNum > 255) {
-					bfin_write_TWI_MASTER_CTL(0xff << 6);
+					write_MASTER_CTL(iface, 0xff << 6);
 					iface->manual_stop = 1;
 				} else {
 					del_timer(&iface->timeout_timer);
@@ -516,13 +540,13 @@ int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
 				}
 			}
 		}
-		bfin_write_TWI_INT_MASK(MCOMP | MERR |
+		write_INT_MASK(iface, MCOMP | MERR |
 			((iface->read_write == I2C_SMBUS_READ) ?
 			RCVSERV : XMTSERV));
 		SSYNC();
 
 		/* Master enable */
-		bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
+		write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
 			((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
 			((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0));
 		break;
@@ -557,10 +581,10 @@ static struct i2c_algorithm bfin_twi_algorithm = {
 
 static int i2c_bfin_twi_suspend(struct platform_device *dev, pm_message_t state)
 {
-/*	struct bfin_twi_iface *iface = platform_get_drvdata(dev);*/
+	struct bfin_twi_iface *iface = platform_get_drvdata(dev);
 
 	/* Disable TWI */
-	bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() & ~TWI_ENA);
+	write_CONTROL(iface, read_CONTROL(iface) & ~TWI_ENA);
 	SSYNC();
 
 	return 0;
@@ -568,24 +592,53 @@ static int i2c_bfin_twi_suspend(struct platform_device *dev, pm_message_t state)
 
 static int i2c_bfin_twi_resume(struct platform_device *dev)
 {
-/*	struct bfin_twi_iface *iface = platform_get_drvdata(dev);*/
+	struct bfin_twi_iface *iface = platform_get_drvdata(dev);
 
 	/* Enable TWI */
-	bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() | TWI_ENA);
+	write_CONTROL(iface, read_CONTROL(iface) | TWI_ENA);
 	SSYNC();
 
 	return 0;
 }
 
-static int i2c_bfin_twi_probe(struct platform_device *dev)
+static int i2c_bfin_twi_probe(struct platform_device *pdev)
 {
-	struct bfin_twi_iface *iface = &twi_iface;
+	struct bfin_twi_iface *iface;
 	struct i2c_adapter *p_adap;
+	struct resource *res;
 	int rc;
 
+	iface = kzalloc(sizeof(struct bfin_twi_iface), GFP_KERNEL);
+	if (!iface) {
+		dev_err(&pdev->dev, "Cannot allocate memory\n");
+		rc = -ENOMEM;
+		goto out_error_nomem;
+	}
+
 	spin_lock_init(&(iface->lock));
 	init_completion(&(iface->complete));
-	iface->irq = IRQ_TWI;
+
+	/* Find and map our resources */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL) {
+		dev_err(&pdev->dev, "Cannot get IORESOURCE_MEM\n");
+		rc = -ENOENT;
+		goto out_error_get_res;
+	}
+
+	iface->regs_base = ioremap(res->start, (res->end - res->start)+1);
+	if (iface->regs_base == NULL) {
+		dev_err(&pdev->dev, "Cannot map IO\n");
+		rc = -ENXIO;
+		goto out_error_ioremap;
+	}
+
+	iface->irq = platform_get_irq(pdev, 0);
+	if (iface->irq < 0) {
+		dev_err(&pdev->dev, "No IRQ specified\n");
+		rc = -ENOENT;
+		goto out_error_no_irq;
+	}
 
 	init_timer(&(iface->timeout_timer));
 	iface->timeout_timer.function = bfin_twi_timeout;
@@ -593,38 +646,50 @@ static int i2c_bfin_twi_probe(struct platform_device *dev)
 
 	p_adap = &iface->adap;
 	p_adap->id = I2C_HW_BLACKFIN;
-	strlcpy(p_adap->name, dev->name, sizeof(p_adap->name));
+	strlcpy(p_adap->name, pdev->name, sizeof(p_adap->name));
 	p_adap->algo = &bfin_twi_algorithm;
 	p_adap->algo_data = iface;
 	p_adap->class = I2C_CLASS_ALL;
-	p_adap->dev.parent = &dev->dev;
+	p_adap->dev.parent = &pdev->dev;
 
 	rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
-		IRQF_DISABLED, dev->name, iface);
+		IRQF_DISABLED, pdev->name, iface);
 	if (rc) {
-		dev_err(&(p_adap->dev), "i2c-bfin-twi: can't get IRQ %d !\n",
-			iface->irq);
-		return -ENODEV;
+		dev_err(&pdev->dev, "can't get IRQ %d !\n", iface->irq);
+		rc = -ENODEV;
+		goto out_error_req_irq;
 	}
 
 	/* Set TWI internal clock as 10MHz */
-	bfin_write_TWI_CONTROL(((get_sclk() / 1024 / 1024 + 5) / 10) & 0x7F);
+	write_CONTROL(iface, ((get_sclk() / 1024 / 1024 + 5) / 10) & 0x7F);
 
 	/* Set Twi interface clock as specified */
-	bfin_write_TWI_CLKDIV((( 5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ )
-			<< 8) | (( 5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ )
+	write_CLKDIV(iface, ((5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ)
+			<< 8) | ((5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ)
 			& 0xFF));
 
 	/* Enable TWI */
-	bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() | TWI_ENA);
+	write_CONTROL(iface, read_CONTROL(iface) | TWI_ENA);
 	SSYNC();
 
 	rc = i2c_add_adapter(p_adap);
 	if (rc < 0)
 		free_irq(iface->irq, iface);
 	else
-		platform_set_drvdata(dev, iface);
+		platform_set_drvdata(pdev, iface);
 
+	dev_info(&pdev->dev, "Blackfin I2C TWI controller, regs_base@%p\n",
+		iface->regs_base);
+
+	return rc;
+
+out_error_req_irq:
+out_error_no_irq:
+	iounmap(iface->regs_base);
+out_error_ioremap:
+out_error_get_res:
+	kfree(iface);
+out_error_nomem:
 	return rc;
 }
 
@@ -653,8 +718,6 @@ static struct platform_driver i2c_bfin_twi_driver = {
 
 static int __init i2c_bfin_twi_init(void)
 {
-	pr_info("I2C: Blackfin I2C TWI driver\n");
-
 	return platform_driver_register(&i2c_bfin_twi_driver);
 }
 
-- 
1.5.3.4

Re: [PATCH 2/4] Blackfin I2C/TWI driver: Add platform_resource interface to support multi-port TWI controllers

[Jean Delvare]

On Fri,  2 Nov 2007 14:24:22 +0800, Bryan Wu wrote:
>  - Dynamic alloc the resource of TWI driver data according to board information
>  - TWI register read/write accessor based on dynamic regs_base
>  - Support TWI0/TWI1 for BF54x
> 
> Signed-off-by: Bryan Wu <bryan.wu@analog.com>
> ---
>  drivers/i2c/busses/i2c-bfin-twi.c |  269 +++++++++++++++++++++++--------------
>  1 files changed, 166 insertions(+), 103 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 6535852..e495454 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -62,43 +62,66 @@ struct bfin_twi_iface {
>  	struct i2c_msg 		*pmsg;
>  	int			msg_num;
>  	int			cur_msg;
> +	void __iomem		*regs_base;
>  };
>  
> -static struct bfin_twi_iface twi_iface;
> +
> +#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); }

Coding style: missing space after opening curly brace.

Missing parentheses around "off" (twice).

> +
> +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 = bfin_read_TWI_INT_STAT();
> -	unsigned short mast_stat = bfin_read_TWI_MASTER_STAT();
> +	unsigned short twi_int_status = read_INT_STAT(iface);
> +	unsigned short mast_stat = read_MASTER_STAT(iface);
>  
>  	if (twi_int_status & XMTSERV) {
>  		/* Transmit next data */
>  		if (iface->writeNum > 0) {
> -			bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> +			write_XMT_DATA8(iface, *(iface->transPtr++));
>  			iface->writeNum--;
>  		}
>  		/* start receive immediately after complete sending in
>  		 * combine mode.
>  		 */
>  		else if (iface->cur_mode == TWI_I2C_MODE_COMBINED)
> -			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> -				| MDIR | RSTART);
> +			write_MASTER_CTL(iface,
> +				read_MASTER_CTL(iface) | MDIR | RSTART);
>  		else if (iface->manual_stop)
> -			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> -				| STOP);
> +			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)
> -			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> -				| RSTART);
> +			write_MASTER_CTL(iface,
> +				read_MASTER_CTL(iface) | RSTART);
>  		SSYNC();
>  		/* Clear status */
> -		bfin_write_TWI_INT_STAT(XMTSERV);
> +		write_INT_STAT(iface, XMTSERV);
>  		SSYNC();
>  	}
>  	if (twi_int_status & RCVSERV) {
>  		if (iface->readNum > 0) {
>  			/* Receive next data */
> -			*(iface->transPtr) = bfin_read_TWI_RCV_DATA8();
> +			*(iface->transPtr) = read_RCV_DATA8(iface);
>  			if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
>  				/* Change combine mode into sub mode after
>  				 * read first data.
> @@ -113,33 +136,33 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
>  			iface->transPtr++;
>  			iface->readNum--;
>  		} else if (iface->manual_stop) {
> -			bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL()
> -				| STOP);
> +			write_MASTER_CTL(iface,
> +				read_MASTER_CTL(iface) | 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);
> +			write_MASTER_CTL(iface,
> +				read_MASTER_CTL(iface) | RSTART);
>  			SSYNC();
>  		}
>  		/* Clear interrupt source */
> -		bfin_write_TWI_INT_STAT(RCVSERV);
> +		write_INT_STAT(iface, RCVSERV);
>  		SSYNC();
>  	}
>  	if (twi_int_status & MERR) {
> -		bfin_write_TWI_INT_STAT(MERR);
> -		bfin_write_TWI_INT_MASK(0);
> -		bfin_write_TWI_MASTER_STAT(0x3e);
> -		bfin_write_TWI_MASTER_CTL(0);
> +		write_INT_STAT(iface, MERR);
> +		write_INT_MASK(iface, 0);
> +		write_MASTER_STAT(iface, 0x3e);
> +		write_MASTER_CTL(iface, 0);
>  		SSYNC();
>  		iface->result = -EIO;
>  		/* if both err and complete int stats are set, return proper
>  		 * results.
>  		 */
>  		if (twi_int_status & MCOMP) {
> -			bfin_write_TWI_INT_STAT(MCOMP);
> -			bfin_write_TWI_INT_MASK(0);
> -			bfin_write_TWI_MASTER_CTL(0);
> +			write_INT_STAT(iface, MCOMP);
> +			write_INT_MASK(iface, 0);
> +			write_MASTER_CTL(iface, 0);
>  			SSYNC();
>  			/* If it is a quick transfer, only address bug no data,
>  			 * not an err, return 1.
> @@ -156,7 +179,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
>  		return;
>  	}
>  	if (twi_int_status & MCOMP) {
> -		bfin_write_TWI_INT_STAT(MCOMP);
> +		write_INT_STAT(iface, MCOMP);
>  		SSYNC();
>  		if (iface->cur_mode == TWI_I2C_MODE_COMBINED) {
>  			if (iface->readNum == 0) {
> @@ -165,23 +188,22 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
>  				 */
>  				iface->readNum = 1;
>  				iface->manual_stop = 1;
> -				bfin_write_TWI_MASTER_CTL(
> -					bfin_read_TWI_MASTER_CTL()
> -					| (0xff << 6));
> +				write_MASTER_CTL(iface,
> +					read_MASTER_CTL(iface) | (0xff << 6));
>  			} else {
>  				/* set the readd number in other
>  				 * combine mode.
>  				 */
> -				bfin_write_TWI_MASTER_CTL(
> -					(bfin_read_TWI_MASTER_CTL() &
> +				write_MASTER_CTL(iface,
> +					(read_MASTER_CTL(iface) &
>  					(~(0xff << 6))) |
> -					( iface->readNum << 6));
> +					(iface->readNum << 6));
>  			}
>  			/* 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 | MDIR);
> +			write_MASTER_CTL(iface,
> +				read_MASTER_CTL(iface) & ~RSTART);
> +			write_MASTER_CTL(iface,
> +				read_MASTER_CTL(iface) | MEN | MDIR);
>  			SSYNC();
>  		} else if (iface->cur_mode == TWI_I2C_MODE_REPEAT &&
>  				iface->cur_msg+1 < iface->msg_num) {
> @@ -190,7 +212,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
>  			iface->writeNum = iface->readNum =
>  				iface->pmsg[iface->cur_msg].len;
>  			/* Set Transmit device address */
> -			bfin_write_TWI_MASTER_ADDR(
> +			write_MASTER_ADDR(iface,
>  				iface->pmsg[iface->cur_msg].addr);
>  			if (iface->pmsg[iface->cur_msg].flags & I2C_M_RD)
>  				iface->read_write = I2C_SMBUS_READ;
> @@ -198,7 +220,7 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
>  				iface->read_write = I2C_SMBUS_WRITE;
>  				/* Transmit first data */
>  				if (iface->writeNum > 0) {
> -					bfin_write_TWI_XMT_DATA8(
> +					write_XMT_DATA8(iface,
>  						*(iface->transPtr++));
>  					iface->writeNum--;
>  					SSYNC();
> @@ -206,23 +228,23 @@ static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
>  			}
>  
>  			if (iface->pmsg[iface->cur_msg].len <= 255)
> -				bfin_write_TWI_MASTER_CTL(
> +				write_MASTER_CTL(iface,
>  				iface->pmsg[iface->cur_msg].len << 6);
>  			else if (iface->pmsg[iface->cur_msg].len > 255) {
> -				bfin_write_TWI_MASTER_CTL(0xff << 6);
> +				write_MASTER_CTL(iface, 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() |
> +			write_MASTER_CTL(iface,
> +				read_MASTER_CTL(iface) & ~RSTART);
> +			write_MASTER_CTL(iface, read_MASTER_CTL(iface) |
>  				MEN | ((iface->read_write == I2C_SMBUS_READ) ?
>  				MDIR : 0));
>  			SSYNC();
>  		} else {
>  			iface->result = 1;
> -			bfin_write_TWI_INT_MASK(0);
> -			bfin_write_TWI_MASTER_CTL(0);
> +			write_INT_MASK(iface, 0);
> +			write_MASTER_CTL(iface, 0);
>  			SSYNC();
>  			complete(&iface->complete);
>  		}
> @@ -272,12 +294,11 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
>  	struct i2c_msg *pmsg;
>  	int rc = 0;
>  
> -	if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> +	if (!(read_CONTROL(iface) & TWI_ENA))
>  		return -ENXIO;
>  
> -	while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> +	while (read_MASTER_STAT(iface) & BUSBUSY)
>  		yield();
> -	}
>  
>  	iface->pmsg = msgs;
>  	iface->msg_num = num;
> @@ -296,14 +317,14 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
>  	iface->result = 0;
>  	iface->timeout_count = 10;
>  	/* Set Transmit device address */
> -	bfin_write_TWI_MASTER_ADDR(pmsg->addr);
> +	write_MASTER_ADDR(iface, pmsg->addr);
>  
>  	/* FIFO Initiation. Data in FIFO should be
>  	 *  discarded before start a new operation.
>  	 */
> -	bfin_write_TWI_FIFO_CTL(0x3);
> +	write_FIFO_CTL(iface, 0x3);
>  	SSYNC();
> -	bfin_write_TWI_FIFO_CTL(0);
> +	write_FIFO_CTL(iface, 0);
>  	SSYNC();
>  
>  	if (pmsg->flags & I2C_M_RD)
> @@ -312,23 +333,23 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
>  		iface->read_write = I2C_SMBUS_WRITE;
>  		/* Transmit first data */
>  		if (iface->writeNum > 0) {
> -			bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> +			write_XMT_DATA8(iface, *(iface->transPtr++));
>  			iface->writeNum--;
>  			SSYNC();
>  		}
>  	}
>  
>  	/* clear int stat */
> -	bfin_write_TWI_INT_STAT(MERR | MCOMP | XMTSERV | RCVSERV);
> +	write_INT_STAT(iface, MERR | MCOMP | XMTSERV | RCVSERV);
>  
>  	/* Interrupt mask . Enable XMT, RCV interrupt */
> -	bfin_write_TWI_INT_MASK(MCOMP | MERR | RCVSERV | XMTSERV);
> +	write_INT_MASK(iface, MCOMP | MERR | RCVSERV | XMTSERV);
>  	SSYNC();
>  
>  	if (pmsg->len <= 255)
> -		bfin_write_TWI_MASTER_CTL(pmsg->len << 6);
> +		write_MASTER_CTL(iface, pmsg->len << 6);
>  	else if (pmsg->len > 255) {
> -		bfin_write_TWI_MASTER_CTL(0xff << 6);
> +		write_MASTER_CTL(iface, 0xff << 6);
>  		iface->manual_stop = 1;
>  	}
>  
> @@ -336,7 +357,7 @@ static int bfin_twi_master_xfer(struct i2c_adapter *adap,
>  	add_timer(&iface->timeout_timer);
>  
>  	/* Master enable */
> -	bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> +	write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
>  		((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
>  		((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0));
>  	SSYNC();
> @@ -362,12 +383,11 @@ int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>  	struct bfin_twi_iface *iface = adap->algo_data;
>  	int rc = 0;
>  
> -	if (!(bfin_read_TWI_CONTROL() & TWI_ENA))
> +	if (!(read_CONTROL(iface) & TWI_ENA))
>  		return -ENXIO;
>  
> -	while (bfin_read_TWI_MASTER_STAT() & BUSBUSY) {
> +	while (read_MASTER_STAT(iface) & BUSBUSY)
>  		yield();
> -	}
>  
>  	iface->writeNum = 0;
>  	iface->readNum = 0;
> @@ -439,15 +459,15 @@ int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>  	/* FIFO Initiation. Data in FIFO should be discarded before
>  	 * start a new operation.
>  	 */
> -	bfin_write_TWI_FIFO_CTL(0x3);
> +	write_FIFO_CTL(iface, 0x3);
>  	SSYNC();
> -	bfin_write_TWI_FIFO_CTL(0);
> +	write_FIFO_CTL(iface, 0);
>  
>  	/* clear int stat */
> -	bfin_write_TWI_INT_STAT(MERR|MCOMP|XMTSERV|RCVSERV);
> +	write_INT_STAT(iface, MERR | MCOMP | XMTSERV | RCVSERV);
>  
>  	/* Set Transmit device address */
> -	bfin_write_TWI_MASTER_ADDR(addr);
> +	write_MASTER_ADDR(iface, addr);
>  	SSYNC();
>  
>  	iface->timeout_timer.expires = jiffies + POLL_TIMEOUT;
> @@ -455,60 +475,64 @@ int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>  
>  	switch (iface->cur_mode) {
>  	case TWI_I2C_MODE_STANDARDSUB:
> -		bfin_write_TWI_XMT_DATA8(iface->command);
> -		bfin_write_TWI_INT_MASK(MCOMP | MERR |
> +		write_XMT_DATA8(iface, iface->command);
> +		write_INT_MASK(iface, MCOMP | MERR |
>  			((iface->read_write == I2C_SMBUS_READ) ?
>  			RCVSERV : XMTSERV));
>  		SSYNC();
>  
>  		if (iface->writeNum + 1 <= 255)
> -			bfin_write_TWI_MASTER_CTL((iface->writeNum + 1) << 6);
> +			write_MASTER_CTL(iface, (iface->writeNum + 1) << 6);
>  		else {
> -			bfin_write_TWI_MASTER_CTL(0xff << 6);
> +			write_MASTER_CTL(iface, 0xff << 6);
>  			iface->manual_stop = 1;
>  		}
>  		/* Master enable */
> -		bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> +		write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
>  			((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
>  		break;
>  	case TWI_I2C_MODE_COMBINED:
> -		bfin_write_TWI_XMT_DATA8(iface->command);
> -		bfin_write_TWI_INT_MASK(MCOMP | MERR | RCVSERV | XMTSERV);
> +		write_XMT_DATA8(iface, iface->command);
> +		write_INT_MASK(iface, MCOMP | MERR | RCVSERV | XMTSERV);
>  		SSYNC();
>  
>  		if (iface->writeNum > 0)
> -			bfin_write_TWI_MASTER_CTL((iface->writeNum + 1) << 6);
> +			write_MASTER_CTL(iface, (iface->writeNum + 1) << 6);
>  		else
> -			bfin_write_TWI_MASTER_CTL(0x1 << 6);
> +			write_MASTER_CTL(iface, 0x1 << 6);
>  		/* Master enable */
> -		bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> +		write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
>  			((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ>100) ? FAST : 0));
>  		break;
>  	default:
> -		bfin_write_TWI_MASTER_CTL(0);
> +		write_MASTER_CTL(iface, 0);
>  		if (size != I2C_SMBUS_QUICK) {
>  			/* Don't access xmit data register when this is a
>  			 * read operation.
>  			 */
>  			if (iface->read_write != I2C_SMBUS_READ) {
>  				if (iface->writeNum > 0) {
> -					bfin_write_TWI_XMT_DATA8(*(iface->transPtr++));
> +					write_XMT_DATA8(iface,
> +						*(iface->transPtr++));
>  					if (iface->writeNum <= 255)
> -						bfin_write_TWI_MASTER_CTL(iface->writeNum << 6);
> +						write_MASTER_CTL(iface,
> +							iface->writeNum << 6);
>  					else {
> -						bfin_write_TWI_MASTER_CTL(0xff << 6);
> +						write_MASTER_CTL(iface,
> +							0xff << 6);
>  						iface->manual_stop = 1;
>  					}
>  					iface->writeNum--;
>  				} else {
> -					bfin_write_TWI_XMT_DATA8(iface->command);
> -					bfin_write_TWI_MASTER_CTL(1 << 6);
> +					write_XMT_DATA8(iface, iface->command);
> +					write_MASTER_CTL(iface, 1 << 6);
>  				}
>  			} else {
>  				if (iface->readNum > 0 && iface->readNum <= 255)
> -					bfin_write_TWI_MASTER_CTL(iface->readNum << 6);
> +					write_MASTER_CTL(iface,
> +						iface->readNum << 6);
>  				else if (iface->readNum > 255) {
> -					bfin_write_TWI_MASTER_CTL(0xff << 6);
> +					write_MASTER_CTL(iface, 0xff << 6);
>  					iface->manual_stop = 1;
>  				} else {
>  					del_timer(&iface->timeout_timer);
> @@ -516,13 +540,13 @@ int bfin_twi_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>  				}
>  			}
>  		}
> -		bfin_write_TWI_INT_MASK(MCOMP | MERR |
> +		write_INT_MASK(iface, MCOMP | MERR |
>  			((iface->read_write == I2C_SMBUS_READ) ?
>  			RCVSERV : XMTSERV));
>  		SSYNC();
>  
>  		/* Master enable */
> -		bfin_write_TWI_MASTER_CTL(bfin_read_TWI_MASTER_CTL() | MEN |
> +		write_MASTER_CTL(iface, read_MASTER_CTL(iface) | MEN |
>  			((iface->read_write == I2C_SMBUS_READ) ? MDIR : 0) |
>  			((CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ > 100) ? FAST : 0));
>  		break;
> @@ -557,10 +581,10 @@ static struct i2c_algorithm bfin_twi_algorithm = {
>  
>  static int i2c_bfin_twi_suspend(struct platform_device *dev, pm_message_t state)
>  {
> -/*	struct bfin_twi_iface *iface = platform_get_drvdata(dev);*/
> +	struct bfin_twi_iface *iface = platform_get_drvdata(dev);
>  
>  	/* Disable TWI */
> -	bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() & ~TWI_ENA);
> +	write_CONTROL(iface, read_CONTROL(iface) & ~TWI_ENA);
>  	SSYNC();
>  
>  	return 0;
> @@ -568,24 +592,53 @@ static int i2c_bfin_twi_suspend(struct platform_device *dev, pm_message_t state)
>  
>  static int i2c_bfin_twi_resume(struct platform_device *dev)
>  {
> -/*	struct bfin_twi_iface *iface = platform_get_drvdata(dev);*/
> +	struct bfin_twi_iface *iface = platform_get_drvdata(dev);
>  
>  	/* Enable TWI */
> -	bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() | TWI_ENA);
> +	write_CONTROL(iface, read_CONTROL(iface) | TWI_ENA);
>  	SSYNC();
>  
>  	return 0;
>  }
>  
> -static int i2c_bfin_twi_probe(struct platform_device *dev)
> +static int i2c_bfin_twi_probe(struct platform_device *pdev)
>  {
> -	struct bfin_twi_iface *iface = &twi_iface;
> +	struct bfin_twi_iface *iface;
>  	struct i2c_adapter *p_adap;
> +	struct resource *res;
>  	int rc;
>  
> +	iface = kzalloc(sizeof(struct bfin_twi_iface), GFP_KERNEL);
> +	if (!iface) {
> +		dev_err(&pdev->dev, "Cannot allocate memory\n");
> +		rc = -ENOMEM;
> +		goto out_error_nomem;
> +	}
> +
>  	spin_lock_init(&(iface->lock));
>  	init_completion(&(iface->complete));
> -	iface->irq = IRQ_TWI;
> +
> +	/* Find and map our resources */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res == NULL) {
> +		dev_err(&pdev->dev, "Cannot get IORESOURCE_MEM\n");
> +		rc = -ENOENT;
> +		goto out_error_get_res;
> +	}
> +
> +	iface->regs_base = ioremap(res->start, (res->end - res->start)+1);

Parentheses not needed. Suggested spaces around "+".

> +	if (iface->regs_base == NULL) {
> +		dev_err(&pdev->dev, "Cannot map IO\n");
> +		rc = -ENXIO;
> +		goto out_error_ioremap;
> +	}
> +
> +	iface->irq = platform_get_irq(pdev, 0);
> +	if (iface->irq < 0) {
> +		dev_err(&pdev->dev, "No IRQ specified\n");
> +		rc = -ENOENT;
> +		goto out_error_no_irq;
> +	}
>  
>  	init_timer(&(iface->timeout_timer));
>  	iface->timeout_timer.function = bfin_twi_timeout;
> @@ -593,38 +646,50 @@ static int i2c_bfin_twi_probe(struct platform_device *dev)
>  
>  	p_adap = &iface->adap;
>  	p_adap->id = I2C_HW_BLACKFIN;
> -	strlcpy(p_adap->name, dev->name, sizeof(p_adap->name));
> +	strlcpy(p_adap->name, pdev->name, sizeof(p_adap->name));
>  	p_adap->algo = &bfin_twi_algorithm;
>  	p_adap->algo_data = iface;
>  	p_adap->class = I2C_CLASS_ALL;
> -	p_adap->dev.parent = &dev->dev;
> +	p_adap->dev.parent = &pdev->dev;
>  
>  	rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
> -		IRQF_DISABLED, dev->name, iface);
> +		IRQF_DISABLED, pdev->name, iface);
>  	if (rc) {
> -		dev_err(&(p_adap->dev), "i2c-bfin-twi: can't get IRQ %d !\n",
> -			iface->irq);
> -		return -ENODEV;
> +		dev_err(&pdev->dev, "can't get IRQ %d !\n", iface->irq);

Suggesting a capital C for consistency.

> +		rc = -ENODEV;
> +		goto out_error_req_irq;
>  	}
>  
>  	/* Set TWI internal clock as 10MHz */
> -	bfin_write_TWI_CONTROL(((get_sclk() / 1024 / 1024 + 5) / 10) & 0x7F);
> +	write_CONTROL(iface, ((get_sclk() / 1024 / 1024 + 5) / 10) & 0x7F);
>  
>  	/* Set Twi interface clock as specified */
> -	bfin_write_TWI_CLKDIV((( 5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ )
> -			<< 8) | (( 5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ )
> +	write_CLKDIV(iface, ((5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ)
> +			<< 8) | ((5*1024 / CONFIG_I2C_BLACKFIN_TWI_CLK_KHZ)
>  			& 0xFF));
>  
>  	/* Enable TWI */
> -	bfin_write_TWI_CONTROL(bfin_read_TWI_CONTROL() | TWI_ENA);
> +	write_CONTROL(iface, read_CONTROL(iface) | TWI_ENA);
>  	SSYNC();
>  
>  	rc = i2c_add_adapter(p_adap);
>  	if (rc < 0)
>  		free_irq(iface->irq, iface);

This error path is not correct. You will display the info message below
as if things had worked fine, and you'll return an error without
freeing the allocated memory!

>  	else
> -		platform_set_drvdata(dev, iface);
> +		platform_set_drvdata(pdev, iface);
>  
> +	dev_info(&pdev->dev, "Blackfin I2C TWI controller, regs_base@%p\n",
> +		iface->regs_base);
> +
> +	return rc;

This should be a "return 0" - if rc is non-zero you should take the
error path.

> +
> +out_error_req_irq:
> +out_error_no_irq:
> +	iounmap(iface->regs_base);
> +out_error_ioremap:
> +out_error_get_res:
> +	kfree(iface);
> +out_error_nomem:
>  	return rc;
>  }
>  
> @@ -653,8 +718,6 @@ static struct platform_driver i2c_bfin_twi_driver = {
>  
>  static int __init i2c_bfin_twi_init(void)
>  {
> -	pr_info("I2C: Blackfin I2C TWI driver\n");
> -
>  	return platform_driver_register(&i2c_bfin_twi_driver);
>  }
>  


-- 
Jean Delvare

[PATCH 3/4] Blackfin I2C/TWI driver: add missing pin mux operation

[Bryan Wu]

Blackfin TWI controller hardware pin should be requested from GPIO port controller
Before BF54x, there is no need to do this. But as long as BF54x and BF52x
are supported by this generic driver, the missing pin mux operation should be
added.

Signed-off-by: Bryan Wu <bryan.wu@analog.com>
---
 drivers/i2c/busses/i2c-bfin-twi.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index e495454..727625b 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -34,6 +34,7 @@
 #include <linux/platform_device.h>
 
 #include <asm/blackfin.h>
+#include <asm/portmux.h>
 #include <asm/irq.h>
 
 #define POLL_TIMEOUT       (2 * HZ)
@@ -63,6 +64,7 @@ struct bfin_twi_iface {
 	int			msg_num;
 	int			cur_msg;
 	void __iomem		*regs_base;
+	int			bus_num;
 };
 
 
@@ -89,6 +91,24 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
 DEFINE_TWI_REG(RCV_DATA8, 0x88)
 DEFINE_TWI_REG(RCV_DATA16, 0x8C)
 
+static int setup_pin_mux(int action, struct bfin_twi_iface *iface)
+{
+
+	u16 pin_req[2][3] = {
+		{P_TWI0_SCL, P_TWI0_SDA, 0},
+		{P_TWI1_SCL, P_TWI1_SDA, 0},
+	};
+
+	if (action) {
+		if (peripheral_request_list(pin_req[iface->bus_num], DRV_NAME))
+			return -EFAULT;
+	} else {
+		peripheral_free_list(pin_req[iface->bus_num]);
+	}
+
+	return 0;
+}
+
 static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
 {
 	unsigned short twi_int_status = read_INT_STAT(iface);
@@ -640,6 +660,7 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
 		goto out_error_no_irq;
 	}
 
+	iface->bus_num = pdev->id;
 	init_timer(&(iface->timeout_timer));
 	iface->timeout_timer.function = bfin_twi_timeout;
 	iface->timeout_timer.data = (unsigned long)iface;
@@ -652,6 +673,8 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
 	p_adap->class = I2C_CLASS_ALL;
 	p_adap->dev.parent = &pdev->dev;
 
+	setup_pin_mux(1, iface);
+
 	rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
 		IRQF_DISABLED, pdev->name, iface);
 	if (rc) {
@@ -701,6 +724,7 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)
 
 	i2c_del_adapter(&(iface->adap));
 	free_irq(iface->irq, iface);
+	setup_pin_mux(0, iface);
 
 	return 0;
 }
-- 
1.5.3.4

Re: [PATCH 3/4] Blackfin I2C/TWI driver: add missing pin mux operation

[Jean Delvare]

Hi Bryan,

On Fri,  2 Nov 2007 14:24:23 +0800, Bryan Wu wrote:
> Blackfin TWI controller hardware pin should be requested from GPIO port controller
> Before BF54x, there is no need to do this. But as long as BF54x and BF52x
> are supported by this generic driver, the missing pin mux operation should be
> added.
> 
> Signed-off-by: Bryan Wu <bryan.wu@analog.com>
> ---
>  drivers/i2c/busses/i2c-bfin-twi.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index e495454..727625b 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -34,6 +34,7 @@
>  #include <linux/platform_device.h>
>  
>  #include <asm/blackfin.h>
> +#include <asm/portmux.h>
>  #include <asm/irq.h>
>  
>  #define POLL_TIMEOUT       (2 * HZ)
> @@ -63,6 +64,7 @@ struct bfin_twi_iface {
>  	int			msg_num;
>  	int			cur_msg;
>  	void __iomem		*regs_base;
> +	int			bus_num;
>  };
>  
>  
> @@ -89,6 +91,24 @@ DEFINE_TWI_REG(XMT_DATA16, 0x84)
>  DEFINE_TWI_REG(RCV_DATA8, 0x88)
>  DEFINE_TWI_REG(RCV_DATA16, 0x8C)
>  
> +static int setup_pin_mux(int action, struct bfin_twi_iface *iface)

I suggest swapping the order of the parameters - it makes more sense to
pass the object first and the action second than the other way around
IMHO.

Also, all other functions in this driver are named bfin_twi_<something>
so it would be nicer to do the same here.

> +{
> +
> +	u16 pin_req[2][3] = {
> +		{P_TWI0_SCL, P_TWI0_SDA, 0},
> +		{P_TWI1_SCL, P_TWI1_SDA, 0},
> +	};

See my previous review: this array should be const and possibly static.

> +
> +	if (action) {
> +		if (peripheral_request_list(pin_req[iface->bus_num], DRV_NAME))
> +			return -EFAULT;

Mike Frysinger suggested that you should return the error code from
peripheral_request_list() and I agree with him.

> +	} else {
> +		peripheral_free_list(pin_req[iface->bus_num]);
> +	}
> +
> +	return 0;
> +}
> +
>  static void bfin_twi_handle_interrupt(struct bfin_twi_iface *iface)
>  {
>  	unsigned short twi_int_status = read_INT_STAT(iface);
> @@ -640,6 +660,7 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
>  		goto out_error_no_irq;
>  	}
>  
> +	iface->bus_num = pdev->id;
>  	init_timer(&(iface->timeout_timer));
>  	iface->timeout_timer.function = bfin_twi_timeout;
>  	iface->timeout_timer.data = (unsigned long)iface;
> @@ -652,6 +673,8 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
>  	p_adap->class = I2C_CLASS_ALL;
>  	p_adap->dev.parent = &pdev->dev;
>  
> +	setup_pin_mux(1, iface);

You must check the error code and handle it.

> +
>  	rc = request_irq(iface->irq, bfin_twi_interrupt_entry,
>  		IRQF_DISABLED, pdev->name, iface);
>  	if (rc) {
> @@ -701,6 +724,7 @@ static int i2c_bfin_twi_remove(struct platform_device *pdev)
>  
>  	i2c_del_adapter(&(iface->adap));
>  	free_irq(iface->irq, iface);
> +	setup_pin_mux(0, iface);

If you do this here, then you should also do it in the error path in
i2c_bfin_twi_probe().

>  
>  	return 0;
>  }


-- 
Jean Delvare

[PATCH 4/4] Blackfin I2C/TWI driver: add driver descriptions, versions and some module useful information

[Bryan Wu]

Signed-off-by: Bryan Wu <bryan.wu@analog.com>
---
 drivers/i2c/busses/i2c-bfin-twi.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
index 727625b..ae41c0b 100644
--- a/drivers/i2c/busses/i2c-bfin-twi.c
+++ b/drivers/i2c/busses/i2c-bfin-twi.c
@@ -37,6 +37,15 @@
 #include <asm/portmux.h>
 #include <asm/irq.h>
 
+#define DRV_NAME	"i2c-bfin-twi"
+#define DRV_AUTHOR	"Sonic Zhang, Bryan Wu"
+#define DRV_DESC	"Blackfin BF5xx on-chip I2C TWI Contoller Driver"
+#define DRV_VERSION	"1.8"
+
+MODULE_AUTHOR(DRV_AUTHOR);
+MODULE_DESCRIPTION(DRV_DESC);
+MODULE_LICENSE("GPL");
+
 #define POLL_TIMEOUT       (2 * HZ)
 
 /* SMBus mode*/
@@ -701,8 +710,8 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
 	else
 		platform_set_drvdata(pdev, iface);
 
-	dev_info(&pdev->dev, "Blackfin I2C TWI controller, regs_base@%p\n",
-		iface->regs_base);
+	dev_info(&pdev->dev, "%s, Version %s, regs_base@%p\n",
+		DRV_DESC, DRV_VERSION, iface->regs_base);
 
 	return rc;
 
@@ -735,7 +744,7 @@ static struct platform_driver i2c_bfin_twi_driver = {
 	.suspend	= i2c_bfin_twi_suspend,
 	.resume		= i2c_bfin_twi_resume,
 	.driver		= {
-		.name	= "i2c-bfin-twi",
+		.name	= DRV_NAME,
 		.owner	= THIS_MODULE,
 	},
 };
@@ -750,9 +759,5 @@ static void __exit i2c_bfin_twi_exit(void)
 	platform_driver_unregister(&i2c_bfin_twi_driver);
 }
 
-MODULE_AUTHOR("Sonic Zhang <sonic.zhang@analog.com>");
-MODULE_DESCRIPTION("I2C-Bus adapter routines for Blackfin TWI");
-MODULE_LICENSE("GPL");
-
 module_init(i2c_bfin_twi_init);
 module_exit(i2c_bfin_twi_exit);
-- 
1.5.3.4

Re: [PATCH 4/4] Blackfin I2C/TWI driver: add driver descriptions, versions and some module useful information

[Jean Delvare]

On Fri,  2 Nov 2007 14:24:24 +0800, Bryan Wu wrote:
> Signed-off-by: Bryan Wu <bryan.wu@analog.com>
> ---
>  drivers/i2c/busses/i2c-bfin-twi.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 727625b..ae41c0b 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -37,6 +37,15 @@
>  #include <asm/portmux.h>
>  #include <asm/irq.h>
>  
> +#define DRV_NAME	"i2c-bfin-twi"
> +#define DRV_AUTHOR	"Sonic Zhang, Bryan Wu"

What's the point this define when you're using it only once, a few
lines below?

> +#define DRV_DESC	"Blackfin BF5xx on-chip I2C TWI Contoller Driver"
> +#define DRV_VERSION	"1.8"
> +
> +MODULE_AUTHOR(DRV_AUTHOR);
> +MODULE_DESCRIPTION(DRV_DESC);
> +MODULE_LICENSE("GPL");
> +
>  #define POLL_TIMEOUT       (2 * HZ)
>  
>  /* SMBus mode*/
> @@ -701,8 +710,8 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev)
>  	else
>  		platform_set_drvdata(pdev, iface);
>  
> -	dev_info(&pdev->dev, "Blackfin I2C TWI controller, regs_base@%p\n",
> -		iface->regs_base);
> +	dev_info(&pdev->dev, "%s, Version %s, regs_base@%p\n",
> +		DRV_DESC, DRV_VERSION, iface->regs_base);

You're mixing device and driver here. DRV_DESC describes the driver,
while this dev_info() refers to a specific device. So I don't see more
confusion than benefit with this change. If you want to expose the
driver version, you'd rather user MODULE_VERSION().

>  
>  	return rc;
>  
> @@ -735,7 +744,7 @@ static struct platform_driver i2c_bfin_twi_driver = {
>  	.suspend	= i2c_bfin_twi_suspend,
>  	.resume		= i2c_bfin_twi_resume,
>  	.driver		= {
> -		.name	= "i2c-bfin-twi",
> +		.name	= DRV_NAME,
>  		.owner	= THIS_MODULE,
>  	},
>  };
> @@ -750,9 +759,5 @@ static void __exit i2c_bfin_twi_exit(void)
>  	platform_driver_unregister(&i2c_bfin_twi_driver);
>  }
>  
> -MODULE_AUTHOR("Sonic Zhang <sonic.zhang@analog.com>");
> -MODULE_DESCRIPTION("I2C-Bus adapter routines for Blackfin TWI");
> -MODULE_LICENSE("GPL");
> -
>  module_init(i2c_bfin_twi_init);
>  module_exit(i2c_bfin_twi_exit);


-- 
Jean Delvare