[U-Boot] [PATCH 0/3] rockchip: i2c: fix >32 byte writes

[U-Boot] [PATCH 0/3] rockchip: i2c: fix >32 byte writes

[John Keeping]

The first two patches are just cleanup that I noticed while in the area,
the point of the series is the final patch which fixes an error when
trying to write more than 32 bytes via the Rockchip I2C driver.

John Keeping (3):
  rockchip: i2c: use named constant when appropriate
  rockchip: i2c: move register write out of inner loop
  rockchip: i2c: fix >32 byte writes

 drivers/i2c/rk_i2c.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.9.3.728.g30b24b4.dirty

[U-Boot] [PATCH 1/3] rockchip: i2c: use named constant when appropriate

[John Keeping]

Make it clear that we are using the same value in two adjacent lines.

Signed-off-by: John Keeping <john@metanate.com>
---

 drivers/i2c/rk_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c
index 63b1418..2597970 100644
--- a/drivers/i2c/rk_i2c.c
+++ b/drivers/i2c/rk_i2c.c
@@ -258,7 +258,7 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 
 	while (bytes_remain_len) {
 		if (bytes_remain_len > RK_I2C_FIFO_SIZE)
-			bytes_xferred = 32;
+			bytes_xferred = RK_I2C_FIFO_SIZE;
 		else
 			bytes_xferred = bytes_remain_len;
 		words_xferred = DIV_ROUND_UP(bytes_xferred, 4);
-- 
2.9.3.728.g30b24b4.dirty

[U-Boot] [PATCH 1/3] rockchip: i2c: use named constant when appropriate

[Simon Glass]

On 18 August 2016 at 13:08, John Keeping <john@metanate.com> wrote:
> Make it clear that we are using the same value in two adjacent lines.
>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>
>  drivers/i2c/rk_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/3] rockchip: i2c: move register write out of inner loop

[John Keeping]

There is no point in writing intermediate values to the txdata
registers.

Also add padding to the debug logging to make it easier to read when
there are leading zeroes.

Signed-off-by: John Keeping <john@metanate.com>
---

 drivers/i2c/rk_i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c
index 2597970..a4c0032 100644
--- a/drivers/i2c/rk_i2c.c
+++ b/drivers/i2c/rk_i2c.c
@@ -277,9 +277,9 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 				} else {
 					txdata |= (*pbuf++)<<(j * 8);
 				}
-				writel(txdata, &regs->txdata[i]);
 			}
-			debug("I2c Write TXDATA[%d] = 0x%x\n", i, txdata);
+			writel(txdata, &regs->txdata[i]);
+			debug("I2c Write TXDATA[%d] = 0x%08x\n", i, txdata);
 		}
 
 		writel(I2C_CON_EN | I2C_CON_MOD(I2C_MODE_TX), &regs->con);
-- 
2.9.3.728.g30b24b4.dirty

[U-Boot] [PATCH 2/3] rockchip: i2c: move register write out of inner loop

[Simon Glass]

On 18 August 2016 at 13:08, John Keeping <john@metanate.com> wrote:
> There is no point in writing intermediate values to the txdata
> registers.
>
> Also add padding to the debug logging to make it easier to read when
> there are leading zeroes.
>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
>
>  drivers/i2c/rk_i2c.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 3/3] rockchip: i2c: fix >32 byte writes

[John Keeping]

The special handling of the chip address and register address must only
happen before we send the data buffer, otherwise we will end up
inserting both of these every 32 bytes.

Signed-off-by: John Keeping <john@metanate.com>

---
I'm not entirely sure about this; it's the smallest change that fixes
the issue and I can't see another way to fix it without completely
rewriting the function.  I guess we could drop r_len completely since
the only caller always passes zero and that would make it all a bit
simpler, but I don't want to conflict with any plan to use this function
elsewhere.

 drivers/i2c/rk_i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c
index a4c0032..7c701cb 100644
--- a/drivers/i2c/rk_i2c.c
+++ b/drivers/i2c/rk_i2c.c
@@ -269,9 +269,9 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
 				if ((i * 4 + j) == bytes_xferred)
 					break;
 
-				if (i == 0 && j == 0) {
+				if (i == 0 && j == 0 && pbuf == buf) {
 					txdata |= (chip << 1);
-				} else if (i == 0 && j <= r_len) {
+				} else if (i == 0 && j <= r_len && pbuf == buf) {
 					txdata |= (reg &
 						(0xff << ((j - 1) * 8))) << 8;
 				} else {
-- 
2.9.3.728.g30b24b4.dirty

[U-Boot] [PATCH 3/3] rockchip: i2c: fix >32 byte writes

[Simon Glass]

Hi John,

On 18 August 2016 at 13:08, John Keeping <john@metanate.com> wrote:
> The special handling of the chip address and register address must only
> happen before we send the data buffer, otherwise we will end up
> inserting both of these every 32 bytes.
>
> Signed-off-by: John Keeping <john@metanate.com>
>
> ---
> I'm not entirely sure about this; it's the smallest change that fixes
> the issue and I can't see another way to fix it without completely
> rewriting the function.  I guess we could drop r_len completely since
> the only caller always passes zero and that would make it all a bit
> simpler, but I don't want to conflict with any plan to use this function
> elsewhere.
>
>  drivers/i2c/rk_i2c.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/rk_i2c.c b/drivers/i2c/rk_i2c.c
> index a4c0032..7c701cb 100644
> --- a/drivers/i2c/rk_i2c.c
> +++ b/drivers/i2c/rk_i2c.c
> @@ -269,9 +269,9 @@ static int rk_i2c_write(struct rk_i2c *i2c, uchar chip, uint reg, uint r_len,
>                                 if ((i * 4 + j) == bytes_xferred)
>                                         break;
>
> -                               if (i == 0 && j == 0) {
> +                               if (i == 0 && j == 0 && pbuf == buf) {
>                                         txdata |= (chip << 1);
> -                               } else if (i == 0 && j <= r_len) {
> +                               } else if (i == 0 && j <= r_len && pbuf == buf) {
>                                         txdata |= (reg &
>                                                 (0xff << ((j - 1) * 8))) << 8;
>                                 } else {
> --
> 2.9.3.728.g30b24b4.dirty
>

The original code is not great. I would rather that it puts the chip
and address info into txdata outside the loops.

But anyway your change looks right. I did think about using an
explicit boolean like 'addr_sent', set to false at the strart of the
function and true once sent. But given the existing code, we might as
well go with what you have.

Acked-by: Simon Glass <sjg@chromium.org>

Regards,
Simon