diff mbox

i2c: rk3x: fix bug that cause transfer fails in master receive mode

Message ID 1408643457-7126-1-git-send-email-addy.ke@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

addy ke Aug. 21, 2014, 5:50 p.m. UTC
In rk3x SOC, the I2C controller can receive/transmit up to 32 bytes data
in one transaction, so the size of data to be write/read to/from
TXDATAx/RXDATAx must be less than or equal 32 bytes at a time.

Test on pinky board, elan receive 158 bytes data.

Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
---
 drivers/i2c/busses/i2c-rk3x.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Sergei Shtylyov Aug. 21, 2014, 6:04 p.m. UTC | #1
Hello.

On 08/21/2014 09:50 PM, Addy Ke wrote:

> In rk3x SOC, the I2C controller can receive/transmit up to 32 bytes data
> in one transaction, so the size of data to be write/read to/from
> TXDATAx/RXDATAx must be less than or equal 32 bytes at a time.

> Test on pinky board, elan receive 158 bytes data.

> Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> ---
>   drivers/i2c/busses/i2c-rk3x.c | 3 +++
>   1 file changed, 3 insertions(+)

> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 69e1185..dc0aa64 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -323,6 +323,9 @@ static void rk3x_i2c_handle_read(struct rk3x_i2c *i2c, unsigned int ipd)
>   	/* ack interrupt */
>   	i2c_writel(i2c, REG_INT_MBRF, REG_IPD);
>
> +	/* Can only handle a maximum of 32 bytes at a time */
> +	len = (len > 32) ? 32 : len;

    Why not min(len, 32)? Or even:

	if (len > 32)
		len = 32;

WBR, Sergei
Wolfram Sang Aug. 21, 2014, 6:17 p.m. UTC | #2
On Thu, Aug 21, 2014 at 10:04:31PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 08/21/2014 09:50 PM, Addy Ke wrote:
> 
> >In rk3x SOC, the I2C controller can receive/transmit up to 32 bytes data
> >in one transaction, so the size of data to be write/read to/from
> >TXDATAx/RXDATAx must be less than or equal 32 bytes at a time.
> 
> >Test on pinky board, elan receive 158 bytes data.
> 
> >Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> >---
> >  drivers/i2c/busses/i2c-rk3x.c | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> >diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> >index 69e1185..dc0aa64 100644
> >--- a/drivers/i2c/busses/i2c-rk3x.c
> >+++ b/drivers/i2c/busses/i2c-rk3x.c
> >@@ -323,6 +323,9 @@ static void rk3x_i2c_handle_read(struct rk3x_i2c *i2c, unsigned int ipd)
> >  	/* ack interrupt */
> >  	i2c_writel(i2c, REG_INT_MBRF, REG_IPD);
> >
> >+	/* Can only handle a maximum of 32 bytes at a time */
> >+	len = (len > 32) ? 32 : len;
> 
>    Why not min(len, 32)? Or even:
> 
> 	if (len > 32)
> 		len = 32;

No silent trimming, please. The message should be rejected when the transfer is
set up. We could assign -EOVERFLOW to that type of failures, so users
will know. Sadly, I have seen other controllers having such limits :(
Max Schwarz Aug. 21, 2014, 6:54 p.m. UTC | #3
Hi,

On Thursday 21 August 2014 at 13:17:38, Wolfram Sang wrote:
> On Thu, Aug 21, 2014 at 10:04:31PM +0400, Sergei Shtylyov wrote:
> > Hello.
> > 
> > On 08/21/2014 09:50 PM, Addy Ke wrote:
> > >In rk3x SOC, the I2C controller can receive/transmit up to 32 bytes data
> > >in one transaction, so the size of data to be write/read to/from
> > >TXDATAx/RXDATAx must be less than or equal 32 bytes at a time.
> > >
> > >Test on pinky board, elan receive 158 bytes data.
> > >
> > >Signed-off-by: Addy Ke <addy.ke@rock-chips.com>
> > >---
> > >
> > >  drivers/i2c/busses/i2c-rk3x.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > >diff --git a/drivers/i2c/busses/i2c-rk3x.c
> > >b/drivers/i2c/busses/i2c-rk3x.c
> > >index 69e1185..dc0aa64 100644
> > >--- a/drivers/i2c/busses/i2c-rk3x.c
> > >+++ b/drivers/i2c/busses/i2c-rk3x.c
> > >@@ -323,6 +323,9 @@ static void rk3x_i2c_handle_read(struct rk3x_i2c
> > >*i2c, unsigned int ipd)> >
> > >  	/* ack interrupt */
> > >  	i2c_writel(i2c, REG_INT_MBRF, REG_IPD);
> > >
> > >+	/* Can only handle a maximum of 32 bytes at a time */
> > >+	len = (len > 32) ? 32 : len;
> > >
> >    Why not min(len, 32)? Or even:
> > 	if (len > 32)
> > 	
> > 		len = 32;
> 
> No silent trimming, please. The message should be rejected when the transfer
> is set up. We could assign -EOVERFLOW to that type of failures, so users
> will know. Sadly, I have seen other controllers having such limits :(

Actually, reads with >32 bytes of data are possible with the controller. The 
hw returns the data in 32 byte chunks. Our chunk handling code is just buggy. 
We always read the number of missing bytes from the current chunk, instead of 
truncating that to 32.
The fix is correct. The following lines in the rk3x_i2c_handle_read() function 
already check if we got everything and trigger an additional chunk read if 
necessary:

	/* are we finished? */
	if (i2c->processed == i2c->msg->len)
		rk3x_i2c_stop(i2c, i2c->error);
	else
		rk3x_i2c_prepare_read(i2c);

I'd like a clearer syntax as suggested by Sergei, though.

Apart from that,
Acked-By: Max Schwarz <max.schwarz@online.de>

Cheers,
  Max
Wolfram Sang Aug. 21, 2014, 8:12 p.m. UTC | #4
> Actually, reads with >32 bytes of data are possible with the controller. The 
> hw returns the data in 32 byte chunks. Our chunk handling code is just buggy. 

Understood. Then, I'd prefer 'chunk' in the commit message instead of
'transaction'.
diff mbox

Patch

diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
index 69e1185..dc0aa64 100644
--- a/drivers/i2c/busses/i2c-rk3x.c
+++ b/drivers/i2c/busses/i2c-rk3x.c
@@ -323,6 +323,9 @@  static void rk3x_i2c_handle_read(struct rk3x_i2c *i2c, unsigned int ipd)
 	/* ack interrupt */
 	i2c_writel(i2c, REG_INT_MBRF, REG_IPD);
 
+	/* Can only handle a maximum of 32 bytes at a time */
+	len = (len > 32) ? 32 : len;
+
 	/* read the data from receive buffer */
 	for (i = 0; i < len; ++i) {
 		if (i % 4 == 0)