diff mbox

I2C: S3C24X0: Bug fixes in i2c_transfer

Message ID 1361276385-2600-1-git-send-email-rajeshwari.s@samsung.com
State Accepted
Commit cb466c056a878dcae27b76eb86a124e0c5203e7a
Headers show

Commit Message

Rajeshwari Shinde Feb. 19, 2013, 12:19 p.m. UTC
This patch corrects the following issues

1) Write the correct M/T Stop value to I2CSTAT after i2c write.
   According to the spec, after finish the data transmission, we should
   write a M/T Stop (I2C_MODE_MT | I2C_TXRX_ENA) to I2CSTAT instead of
   a M/R Stop (I2C_MODE_MR | I2C_TXRX_ENA).
2) Not split the write to I2CSTAT into 2 steps in i2c read.
   According to the spec, we should write the combined M/R Start value to
   I2CSTAT after setting the slave address to I2CDS
3) Fix the mistake of making an equality check to an assignment.
   In the case of I2C write with the zero-length address, while tranfering the
   data, it should be an equality check (==) instead of an assignment (=).

Signed-off-by: Tom Wai-Hong Tam <waihong@chromium.org>
Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com>
---
 drivers/i2c/s3c24x0_i2c.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

Comments

Hung-ying Tyan March 6, 2013, 9:11 a.m. UTC | #1
Tested-by: Hung-ying Tyan <tyanh@chromium.org>


On Tue, Feb 19, 2013 at 8:19 PM, Rajeshwari Shinde <rajeshwari.s@samsung.com
> wrote:

> This patch corrects the following issues
>
> 1) Write the correct M/T Stop value to I2CSTAT after i2c write.
>    According to the spec, after finish the data transmission, we should
>    write a M/T Stop (I2C_MODE_MT | I2C_TXRX_ENA) to I2CSTAT instead of
>    a M/R Stop (I2C_MODE_MR | I2C_TXRX_ENA).
> 2) Not split the write to I2CSTAT into 2 steps in i2c read.
>    According to the spec, we should write the combined M/R Start value to
>    I2CSTAT after setting the slave address to I2CDS
> 3) Fix the mistake of making an equality check to an assignment.
>    In the case of I2C write with the zero-length address, while tranfering
> the
>    data, it should be an equality check (==) instead of an assignment (=).
>
> Signed-off-by: Tom Wai-Hong Tam <waihong@chromium.org>
> Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com>
> ---
>  drivers/i2c/s3c24x0_i2c.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
> index 00308b5..46d2506 100644
> --- a/drivers/i2c/s3c24x0_i2c.c
> +++ b/drivers/i2c/s3c24x0_i2c.c
> @@ -324,7 +324,7 @@ static int i2c_transfer(struct s3c24x0_i2c *i2c,
>                         writel(I2C_MODE_MT | I2C_TXRX_ENA | I2C_START_STOP,
>                                &i2c->iicstat);
>                         i = 0;
> -                       while ((i < data_len) && (result = I2C_OK)) {
> +                       while ((i < data_len) && (result == I2C_OK)) {
>                                 result = WaitForXfer(i2c);
>                                 writel(data[i], &i2c->iicds);
>                                 ReadWriteByte(i2c);
> @@ -336,17 +336,16 @@ static int i2c_transfer(struct s3c24x0_i2c *i2c,
>                         result = WaitForXfer(i2c);
>
>                 /* send STOP */
> -               writel(I2C_MODE_MR | I2C_TXRX_ENA, &i2c->iicstat);
> +               writel(I2C_MODE_MT | I2C_TXRX_ENA, &i2c->iicstat);
>                 ReadWriteByte(i2c);
>                 break;
>
>         case I2C_READ:
>                 if (addr && addr_len) {
> -                       writel(I2C_MODE_MT | I2C_TXRX_ENA, &i2c->iicstat);
>                         writel(chip, &i2c->iicds);
>                         /* send START */
> -                       writel(readl(&i2c->iicstat) | I2C_START_STOP,
> -                              &i2c->iicstat);
> +                       writel(I2C_MODE_MT | I2C_TXRX_ENA | I2C_START_STOP,
> +                               &i2c->iicstat);
>                         result = WaitForXfer(i2c);
>                         if (IsACK(i2c)) {
>                                 i = 0;
> @@ -380,11 +379,10 @@ static int i2c_transfer(struct s3c24x0_i2c *i2c,
>                         }
>
>                 } else {
> -                       writel(I2C_MODE_MR | I2C_TXRX_ENA, &i2c->iicstat);
>                         writel(chip, &i2c->iicds);
>                         /* send START */
> -                       writel(readl(&i2c->iicstat) | I2C_START_STOP,
> -                              &i2c->iicstat);
> +                       writel(I2C_MODE_MR | I2C_TXRX_ENA | I2C_START_STOP,
> +                               &i2c->iicstat);
>                         result = WaitForXfer(i2c);
>
>                         if (IsACK(i2c)) {
> --
> 1.7.4.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Hung-ying Tyan March 6, 2013, 9:27 a.m. UTC | #2
On Tue, Feb 19, 2013 at 8:19 PM, Rajeshwari Shinde <rajeshwari.s@samsung.com
> wrote:

> This patch corrects the following issues
>
> 1) Write the correct M/T Stop value to I2CSTAT after i2c write.
>    According to the spec, after finish the data transmission, we should
>    write a M/T Stop (I2C_MODE_MT | I2C_TXRX_ENA) to I2CSTAT instead of
>    a M/R Stop (I2C_MODE_MR | I2C_TXRX_ENA).
> 2) Not split the write to I2CSTAT into 2 steps in i2c read.
>    According to the spec, we should write the combined M/R Start value to
>    I2CSTAT after setting the slave address to I2CDS
> 3) Fix the mistake of making an equality check to an assignment.
>    In the case of I2C write with the zero-length address, while tranfering
> the
>    data, it should be an equality check (==) instead of an assignment (=).
>
> Signed-off-by: Tom Wai-Hong Tam <waihong@chromium.org>
> Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com>


 Tested-by: Hung-ying Tyan <tyanh@chromium.org>
Rajeshwari Birje March 12, 2013, 4:39 a.m. UTC | #3
Hi Heiko,

Do let me know if you have any comments on this patch.
Would you consider it for I2C branch or should it go through u-boot-samsung?

Regards,
Rajeshwari


On Wed, Mar 6, 2013 at 2:57 PM, Hung-ying Tyan <tyanh@chromium.org> wrote:
> On Tue, Feb 19, 2013 at 8:19 PM, Rajeshwari Shinde <rajeshwari.s@samsung.com
>> wrote:
>
>> This patch corrects the following issues
>>
>> 1) Write the correct M/T Stop value to I2CSTAT after i2c write.
>>    According to the spec, after finish the data transmission, we should
>>    write a M/T Stop (I2C_MODE_MT | I2C_TXRX_ENA) to I2CSTAT instead of
>>    a M/R Stop (I2C_MODE_MR | I2C_TXRX_ENA).
>> 2) Not split the write to I2CSTAT into 2 steps in i2c read.
>>    According to the spec, we should write the combined M/R Start value to
>>    I2CSTAT after setting the slave address to I2CDS
>> 3) Fix the mistake of making an equality check to an assignment.
>>    In the case of I2C write with the zero-length address, while tranfering
>> the
>>    data, it should be an equality check (==) instead of an assignment (=).
>>
>> Signed-off-by: Tom Wai-Hong Tam <waihong@chromium.org>
>> Signed-off-by: Rajeshwari Shinde <rajeshwari.s@samsung.com>
>
>
>  Tested-by: Hung-ying Tyan <tyanh@chromium.org>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
diff mbox

Patch

diff --git a/drivers/i2c/s3c24x0_i2c.c b/drivers/i2c/s3c24x0_i2c.c
index 00308b5..46d2506 100644
--- a/drivers/i2c/s3c24x0_i2c.c
+++ b/drivers/i2c/s3c24x0_i2c.c
@@ -324,7 +324,7 @@  static int i2c_transfer(struct s3c24x0_i2c *i2c,
 			writel(I2C_MODE_MT | I2C_TXRX_ENA | I2C_START_STOP,
 			       &i2c->iicstat);
 			i = 0;
-			while ((i < data_len) && (result = I2C_OK)) {
+			while ((i < data_len) && (result == I2C_OK)) {
 				result = WaitForXfer(i2c);
 				writel(data[i], &i2c->iicds);
 				ReadWriteByte(i2c);
@@ -336,17 +336,16 @@  static int i2c_transfer(struct s3c24x0_i2c *i2c,
 			result = WaitForXfer(i2c);
 
 		/* send STOP */
-		writel(I2C_MODE_MR | I2C_TXRX_ENA, &i2c->iicstat);
+		writel(I2C_MODE_MT | I2C_TXRX_ENA, &i2c->iicstat);
 		ReadWriteByte(i2c);
 		break;
 
 	case I2C_READ:
 		if (addr && addr_len) {
-			writel(I2C_MODE_MT | I2C_TXRX_ENA, &i2c->iicstat);
 			writel(chip, &i2c->iicds);
 			/* send START */
-			writel(readl(&i2c->iicstat) | I2C_START_STOP,
-			       &i2c->iicstat);
+			writel(I2C_MODE_MT | I2C_TXRX_ENA | I2C_START_STOP,
+				&i2c->iicstat);
 			result = WaitForXfer(i2c);
 			if (IsACK(i2c)) {
 				i = 0;
@@ -380,11 +379,10 @@  static int i2c_transfer(struct s3c24x0_i2c *i2c,
 			}
 
 		} else {
-			writel(I2C_MODE_MR | I2C_TXRX_ENA, &i2c->iicstat);
 			writel(chip, &i2c->iicds);
 			/* send START */
-			writel(readl(&i2c->iicstat) | I2C_START_STOP,
-			       &i2c->iicstat);
+			writel(I2C_MODE_MR | I2C_TXRX_ENA | I2C_START_STOP,
+				&i2c->iicstat);
 			result = WaitForXfer(i2c);
 
 			if (IsACK(i2c)) {