rockchip: i2c: fix >32 byte writes
cover rockchip: i2c: fix >32 byte writes
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(-)
Signature · 1 line
2.9.3.728.g30b24b4.dirty
PATCH 1/3 rockchip: i2c: use named constant when appropriate
Make it clear that we are using the same value in two adjacent lines.
---
drivers/i2c/rk_i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/rk_i2c.c +1 -1
@@ -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 );
Signature · 1 line
2.9.3.728.g30b24b4.dirty
PATCH 1/3 rockchip: i2c: use named constant when appropriate
Quoted 7 lines · John Keeping
Make it clear that we are using the same value in two adjacent lines.
Signed-off-by John Keeping <[email protected]>---
drivers/i2c/rk_i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
PATCH 2/3 rockchip: i2c: move register write out of inner loop
In reply to John KeepingThere 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.
---
drivers/i2c/rk_i2c.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/i2c/rk_i2c.c +2 -2
@@ -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, ®s->txdata[i]); } -debug ("I2c Write TXDATA[%d] = 0x%x\n" , i, txdata); +writel (txdata, ®s->txdata[i]); +debug ("I2c Write TXDATA[%d] = 0x%08x\n" , i, txdata); }writel (I2C_CON_EN |I2C_CON_MOD (I2C_MODE_TX ), ®s->con);
Signature · 1 line
2.9.3.728.g30b24b4.dirty
PATCH 2/3 rockchip: i2c: move register write out of inner loop
Quoted 11 lines · 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 <[email protected]>---
drivers/i2c/rk_i2c.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
PATCH 3/3 rockchip: i2c: fix >32 byte writes
In reply to John KeepingThe 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.
---
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(-)
drivers/i2c/rk_i2c.c +2 -2
@@ -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 {
Signature · 1 line
2.9.3.728.g30b24b4.dirty
PATCH 3/3 rockchip: i2c: fix >32 byte writes
Hi John,
Quoted 34 lines · 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 <[email protected]>---
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 dropr_lencompletely 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(-)drivers/i2c/rk_i2c.c +2
-3@@ -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 { --
Quoted 2 lines
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.
Regards,
Simon