[PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Yuantian.tang]

From: Tang Yuantian <B29983@freescale.com>

Add support for SMBUS_READ_BLOCK_DATA to the i2c-mpc bus driver.

Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
tested on p1022ds

 drivers/i2c/busses/i2c-mpc.c |   63 ++++++++++++++++++++++++++++++++---------
 1 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 107397a..bba9a07 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -454,7 +454,7 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
 }
 
 static int mpc_read(struct mpc_i2c *i2c, int target,
-		    u8 *data, int length, int restart)
+		    u8 *data, int length, int restart, bool recv_len)
 {
 	unsigned timeout = i2c->adap.timeout;
 	int i, result;
@@ -470,7 +470,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
 		return result;
 
 	if (length) {
-		if (length == 1)
+		if (length == 1 && !recv_len)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
 		else
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
@@ -479,17 +479,46 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
 	}
 
 	for (i = 0; i < length; i++) {
+		u8 byte;
+
 		result = i2c_wait(i2c, timeout, 0);
 		if (result < 0)
 			return result;
 
-		/* Generate txack on next to last byte */
-		if (i == length - 2)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
-		/* Do not generate stop on last byte */
-		if (i == length - 1)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
-		data[i] = readb(i2c->base + MPC_I2C_DR);
+		/*
+		 * For block reads, we have to know the total length (1st byte)
+		 * before we can determine if we are done.
+		 */
+		if (i || !recv_len) {
+			/* Generate txack on next to last byte */
+			if (i == length - 2)
+				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
+					 | CCR_TXAK);
+			/* Do not generate stop on last byte */
+			if (i == length - 1)
+				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
+					 | CCR_MTX);
+		}
+
+		byte = readb(i2c->base + MPC_I2C_DR);
+
+		/*
+		 * Adjust length if first received byte is length.
+		 * The length is 1 length byte plus actually data length
+		 */
+		if (i == 0 && recv_len) {
+			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
+				return -EPROTO;
+			length += byte;
+			/*
+			 * For block reads, generate txack here if data length
+			 * is 1 byte (total length is 2 bytes).
+			 */
+			if (length == 2)
+				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
+					 | CCR_TXAK);
+		}
+		data[i] = byte;
 	}
 
 	return length;
@@ -532,12 +561,17 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
 			pmsg->flags & I2C_M_RD ? "read" : "write",
 			pmsg->len, pmsg->addr, i + 1, num);
-		if (pmsg->flags & I2C_M_RD)
-			ret =
-			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
-		else
+		if (pmsg->flags & I2C_M_RD) {
+			bool recv_len = pmsg->flags & I2C_M_RECV_LEN;
+
+			ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
+				       recv_len);
+			if (recv_len && ret > 0)
+				pmsg->len = ret;
+		} else {
 			ret =
 			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
+		}
 	}
 	mpc_i2c_stop(i2c);
 	return (ret < 0) ? ret : num;
@@ -545,7 +579,8 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 static u32 mpc_functionality(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
+	  | I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
 }
 
 static const struct i2c_algorithm mpc_algo = {
-- 
1.6.4

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

On Sun, Dec 11, 2011 at 11:10:10PM -0500, Yuantian.tang@freescale.com wrote:
> From: Tang Yuantian <B29983@freescale.com>
> 
> Add support for SMBUS_READ_BLOCK_DATA to the i2c-mpc bus driver.
> 
> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>

You can not do this. While I may have written this patch originally,
I abandoned it, and you can not use my signature.

Guenter

> ---
> tested on p1022ds
> 
>  drivers/i2c/busses/i2c-mpc.c |   63 ++++++++++++++++++++++++++++++++---------
>  1 files changed, 49 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 107397a..bba9a07 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -454,7 +454,7 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
>  }
>  
>  static int mpc_read(struct mpc_i2c *i2c, int target,
> -		    u8 *data, int length, int restart)
> +		    u8 *data, int length, int restart, bool recv_len)
>  {
>  	unsigned timeout = i2c->adap.timeout;
>  	int i, result;
> @@ -470,7 +470,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
>  		return result;
>  
>  	if (length) {
> -		if (length == 1)
> +		if (length == 1 && !recv_len)
>  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
>  		else
>  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
> @@ -479,17 +479,46 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
>  	}
>  
>  	for (i = 0; i < length; i++) {
> +		u8 byte;
> +
>  		result = i2c_wait(i2c, timeout, 0);
>  		if (result < 0)
>  			return result;
>  
> -		/* Generate txack on next to last byte */
> -		if (i == length - 2)
> -			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
> -		/* Do not generate stop on last byte */
> -		if (i == length - 1)
> -			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
> -		data[i] = readb(i2c->base + MPC_I2C_DR);
> +		/*
> +		 * For block reads, we have to know the total length (1st byte)
> +		 * before we can determine if we are done.
> +		 */
> +		if (i || !recv_len) {
> +			/* Generate txack on next to last byte */
> +			if (i == length - 2)
> +				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> +					 | CCR_TXAK);
> +			/* Do not generate stop on last byte */
> +			if (i == length - 1)
> +				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> +					 | CCR_MTX);
> +		}
> +
> +		byte = readb(i2c->base + MPC_I2C_DR);
> +
> +		/*
> +		 * Adjust length if first received byte is length.
> +		 * The length is 1 length byte plus actually data length
> +		 */
> +		if (i == 0 && recv_len) {
> +			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> +				return -EPROTO;
> +			length += byte;
> +			/*
> +			 * For block reads, generate txack here if data length
> +			 * is 1 byte (total length is 2 bytes).
> +			 */
> +			if (length == 2)
> +				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> +					 | CCR_TXAK);
> +		}
> +		data[i] = byte;
>  	}
>  
>  	return length;
> @@ -532,12 +561,17 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
>  			pmsg->flags & I2C_M_RD ? "read" : "write",
>  			pmsg->len, pmsg->addr, i + 1, num);
> -		if (pmsg->flags & I2C_M_RD)
> -			ret =
> -			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> -		else
> +		if (pmsg->flags & I2C_M_RD) {
> +			bool recv_len = pmsg->flags & I2C_M_RECV_LEN;
> +
> +			ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
> +				       recv_len);
> +			if (recv_len && ret > 0)
> +				pmsg->len = ret;
> +		} else {
>  			ret =
>  			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> +		}
>  	}
>  	mpc_i2c_stop(i2c);
>  	return (ret < 0) ? ret : num;
> @@ -545,7 +579,8 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  
>  static u32 mpc_functionality(struct i2c_adapter *adap)
>  {
> -	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> +	  | I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
>  }
>  
>  static const struct i2c_algorithm mpc_algo = {
> -- 
> 1.6.4
> 
> 

[PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

Add support for SMBUS_READ_BLOCK_DATA to the i2c-mpc bus driver.
Required to support the PMBus zl6100 driver with i2c-mpc.

Reported-by: Tang Yuantian <B29983@freescale.com>
Cc: Tang Yuantian <B29983@freescale.com>
Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
 drivers/i2c/busses/i2c-mpc.c |   33 +++++++++++++++++++++++++--------
 1 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 107397a..77aade7 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -454,7 +454,7 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
 }
 
 static int mpc_read(struct mpc_i2c *i2c, int target,
-		    u8 *data, int length, int restart)
+		    u8 *data, int length, int restart, bool block)
 {
 	unsigned timeout = i2c->adap.timeout;
 	int i, result;
@@ -470,7 +470,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
 		return result;
 
 	if (length) {
-		if (length == 1)
+		if (length == 1 && !block)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
 		else
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
@@ -479,17 +479,28 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
 	}
 
 	for (i = 0; i < length; i++) {
+		u8 byte;
+
 		result = i2c_wait(i2c, timeout, 0);
 		if (result < 0)
 			return result;
 
+		byte = readb(i2c->base + MPC_I2C_DR);
+		/*
+		 * Adjust length if first received byte is length
+		 */
+		if (i == 0 && block) {
+			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
+				return -EPROTO;
+			length += byte;
+		}
+		data[i] = byte;
 		/* Generate txack on next to last byte */
 		if (i == length - 2)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
 		/* Do not generate stop on last byte */
 		if (i == length - 1)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
-		data[i] = readb(i2c->base + MPC_I2C_DR);
 	}
 
 	return length;
@@ -532,12 +543,17 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
 			pmsg->flags & I2C_M_RD ? "read" : "write",
 			pmsg->len, pmsg->addr, i + 1, num);
-		if (pmsg->flags & I2C_M_RD)
-			ret =
-			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
-		else
+		if (pmsg->flags & I2C_M_RD) {
+			bool block = pmsg->flags & I2C_M_RECV_LEN;
+
+			ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
+				       block);
+			if (block && ret > 0)
+				pmsg->len = ret;
+		} else {
 			ret =
 			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
+		}
 	}
 	mpc_i2c_stop(i2c);
 	return (ret < 0) ? ret : num;
@@ -545,7 +561,8 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 static u32 mpc_functionality(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
+	  | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
 }
 
 static const struct i2c_algorithm mpc_algo = {
-- 
1.7.3.1

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Jean Delvare]

Hi Guenter,

On Mon, 14 Nov 2011 22:27:42 -0800, Guenter Roeck wrote:
> Add support for SMBUS_READ_BLOCK_DATA to the i2c-mpc bus driver.
> Required to support the PMBus zl6100 driver with i2c-mpc.
> 
> Reported-by: Tang Yuantian <B29983@freescale.com>
> Cc: Tang Yuantian <B29983@freescale.com>
> Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> ---
>  drivers/i2c/busses/i2c-mpc.c |   33 +++++++++++++++++++++++++--------
>  1 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 107397a..77aade7 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -454,7 +454,7 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
>  }
>  
>  static int mpc_read(struct mpc_i2c *i2c, int target,
> -		    u8 *data, int length, int restart)
> +		    u8 *data, int length, int restart, bool block)

bool block would be better named bool recv_len IMHO. It will be set to
0 for I2C block reads, which is confusing.

>  {
>  	unsigned timeout = i2c->adap.timeout;
>  	int i, result;
> @@ -470,7 +470,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
>  		return result;
>  
>  	if (length) {
> -		if (length == 1)
> +		if (length == 1 && !block)
>  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
>  		else
>  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
> @@ -479,17 +479,28 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
>  	}
>  
>  	for (i = 0; i < length; i++) {
> +		u8 byte;
> +
>  		result = i2c_wait(i2c, timeout, 0);
>  		if (result < 0)
>  			return result;
>  
> +		byte = readb(i2c->base + MPC_I2C_DR);
> +		/*
> +		 * Adjust length if first received byte is length
> +		 */
> +		if (i == 0 && block) {
> +			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> +				return -EPROTO;
> +			length += byte;
> +		}
> +		data[i] = byte;
>  		/* Generate txack on next to last byte */
>  		if (i == length - 2)
>  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
>  		/* Do not generate stop on last byte */
>  		if (i == length - 1)
>  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
> -		data[i] = readb(i2c->base + MPC_I2C_DR);
>  	}

This needs careful testing (which I can't do.) There may have been a
reason why the read was done after the writes. Swapping the commands
may be the wrong thing to do. The dummy read earlier in this function
suggests that maybe changes to CCR do not take effect until you read
from (or write to) the DR register.

Can't the above be rewritten to keep the order of the commands as it
was before? AFAICS it would only take one or two extra tests.

Note that the hardware implementation may make it difficult or even
impossible to properly support SMBus block reads of 1 byte. Not sure
what should be done when this can be supported and still happens...
Returning -EOPNOTSUPP I guess, and then probably the I2C engine needs
some form of reset.

>  
>  	return length;
> @@ -532,12 +543,17 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
>  			pmsg->flags & I2C_M_RD ? "read" : "write",
>  			pmsg->len, pmsg->addr, i + 1, num);
> -		if (pmsg->flags & I2C_M_RD)
> -			ret =
> -			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> -		else
> +		if (pmsg->flags & I2C_M_RD) {
> +			bool block = pmsg->flags & I2C_M_RECV_LEN;

Here again I'd rather name it bool recv_len for clarity.

> +
> +			ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
> +				       block);

That's a lot of parameters, most coming from pmsg. It would be more
efficient to pass pmsg itself. Not directly related to your patch,
admittedly, but it makes the problem more obvious. Maybe a cleanup for
later.

> +			if (block && ret > 0)
> +				pmsg->len = ret;
> +		} else {
>  			ret =
>  			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> +		}
>  	}
>  	mpc_i2c_stop(i2c);
>  	return (ret < 0) ? ret : num;
> @@ -545,7 +561,8 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>  
>  static u32 mpc_functionality(struct i2c_adapter *adap)
>  {
> -	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> +	  | I2C_FUNC_SMBUS_READ_BLOCK_DATA;

You could add I2C_FUNC_SMBUS_BLOCK_PROC_CALL too, even though I
don't know of any slave driver using it.

>  }
>  
>  static const struct i2c_algorithm mpc_algo = {


-- 
Jean Delvare

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

Hi Jean,

On Tue, 2011-11-15 at 03:54 -0500, Jean Delvare wrote:
> Hi Guenter,
> 
> On Mon, 14 Nov 2011 22:27:42 -0800, Guenter Roeck wrote:
> > Add support for SMBUS_READ_BLOCK_DATA to the i2c-mpc bus driver.
> > Required to support the PMBus zl6100 driver with i2c-mpc.
> > 
> > Reported-by: Tang Yuantian <B29983@freescale.com>
> > Cc: Tang Yuantian <B29983@freescale.com>
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> >  drivers/i2c/busses/i2c-mpc.c |   33 +++++++++++++++++++++++++--------
> >  1 files changed, 25 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index 107397a..77aade7 100644
> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -454,7 +454,7 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
> >  }
> >  
> >  static int mpc_read(struct mpc_i2c *i2c, int target,
> > -		    u8 *data, int length, int restart)
> > +		    u8 *data, int length, int restart, bool block)
> 
> bool block would be better named bool recv_len IMHO. It will be set to
> 0 for I2C block reads, which is confusing.
> 
Ok.

> >  {
> >  	unsigned timeout = i2c->adap.timeout;
> >  	int i, result;
> > @@ -470,7 +470,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
> >  		return result;
> >  
> >  	if (length) {
> > -		if (length == 1)
> > +		if (length == 1 && !block)
> >  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
> >  		else
> >  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
> > @@ -479,17 +479,28 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
> >  	}
> >  
> >  	for (i = 0; i < length; i++) {
> > +		u8 byte;
> > +
> >  		result = i2c_wait(i2c, timeout, 0);
> >  		if (result < 0)
> >  			return result;
> >  
> > +		byte = readb(i2c->base + MPC_I2C_DR);
> > +		/*
> > +		 * Adjust length if first received byte is length
> > +		 */
> > +		if (i == 0 && block) {
> > +			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > +				return -EPROTO;
> > +			length += byte;
> > +		}
> > +		data[i] = byte;
> >  		/* Generate txack on next to last byte */
> >  		if (i == length - 2)
> >  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
> >  		/* Do not generate stop on last byte */
> >  		if (i == length - 1)
> >  			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
> > -		data[i] = readb(i2c->base + MPC_I2C_DR);
> >  	}
> 
> This needs careful testing (which I can't do.) There may have been a
> reason why the read was done after the writes. Swapping the commands
> may be the wrong thing to do. The dummy read earlier in this function
> suggests that maybe changes to CCR do not take effect until you read
> from (or write to) the DR register.
> 
Interestingly there is not always a read after a write to ccr. If a read
is necessary, I'd rather add a dummy read after writeccr().

> Can't the above be rewritten to keep the order of the commands as it
> was before? AFAICS it would only take one or two extra tests.
> 
The resulting code would not support 1-byte block reads. That seems to
be unnecessary and undesirable.

> Note that the hardware implementation may make it difficult or even
> impossible to properly support SMBus block reads of 1 byte. Not sure
> what should be done when this can be supported and still happens...
> Returning -EOPNOTSUPP I guess, and then probably the I2C engine needs
> some form of reset.
> 
The code generates stop after returning from mpc_read(). That should
hopefully take care of error conditions. I could add 
	writeccr(i2c, 0);
into the error path, as is done for timeouts, but I am not sure if that
would be helpful or not.

Either case, I'd like to avoid that case. I think it would be better to
get some test coverage from someone who has access to a board, or even
better feedback from someone who knows the chip.

Yuantian indicated that my raw patch worked with the zl6100 driver, and
the PMBus driver does a lot of accesses, so we do have some test
coverage already.

> >  
> >  	return length;
> > @@ -532,12 +543,17 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> >  			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
> >  			pmsg->flags & I2C_M_RD ? "read" : "write",
> >  			pmsg->len, pmsg->addr, i + 1, num);
> > -		if (pmsg->flags & I2C_M_RD)
> > -			ret =
> > -			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> > -		else
> > +		if (pmsg->flags & I2C_M_RD) {
> > +			bool block = pmsg->flags & I2C_M_RECV_LEN;
> 
> Here again I'd rather name it bool recv_len for clarity.
> 
Ok.

> > +
> > +			ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
> > +				       block);
> 
> That's a lot of parameters, most coming from pmsg. It would be more
> efficient to pass pmsg itself. Not directly related to your patch,
> admittedly, but it makes the problem more obvious. Maybe a cleanup for
> later.
> 
later ...

> > +			if (block && ret > 0)
> > +				pmsg->len = ret;
> > +		} else {
> >  			ret =
> >  			    mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> > +		}
> >  	}
> >  	mpc_i2c_stop(i2c);
> >  	return (ret < 0) ? ret : num;
> > @@ -545,7 +561,8 @@ static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> >  
> >  static u32 mpc_functionality(struct i2c_adapter *adap)
> >  {
> > -	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> > +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> > +	  | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> 
> You could add I2C_FUNC_SMBUS_BLOCK_PROC_CALL too, even though I
> don't know of any slave driver using it.
> 
Sure.

> >  }
> >  
> >  static const struct i2c_algorithm mpc_algo = {
> 
> 
Thanks,
Guenter

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Tabi Timur-B04825]

On Tue, Nov 15, 2011 at 2:54 AM, Jean Delvare <khali@linux-fr.org> wrote:

> This needs careful testing (which I can't do.)

I can test these patches, so please CC me on them.

> There may have been a
> reason why the read was done after the writes. Swapping the commands
> may be the wrong thing to do. The dummy read earlier in this function
> suggests that maybe changes to CCR do not take effect until you read
> from (or write to) the DR register.

Unfortunately, I don't know enough about the I2C protocol to answer
this, but I can provide this description on the DR register from the
reference manual:

         Transmission starts when an address and the R/W bit are
written to the data register and the I2C interface
         performs as the master. A data transfer is initiated when
data is written to the I2CDR. The most significant bit
         is sent first in both cases. In master receive mode, reading
the data register allows the read to occur, but also
         allows the I2C module to receive the next byte of data on the
I2C interface. In slave mode, the same function
         is available after it is addressed. Note that in both master
receive and slave receive modes, the very first read
         is always a dummy read.

> Can't the above be rewritten to keep the order of the commands as it
> was before? AFAICS it would only take one or two extra tests.

I think it's very important to keep the order as-is, if at all
possible.  This driver is very old and the hardware is used on all of
our SOCs, dating back several years.  It would be a nightmare to test
it all over again.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

On Tue, 2011-11-15 at 14:02 -0500, Tabi Timur-B04825 wrote:
> On Tue, Nov 15, 2011 at 2:54 AM, Jean Delvare <khali@linux-fr.org> wrote:
> 
> > This needs careful testing (which I can't do.)
> 
> I can test these patches, so please CC me on them.
> 
Ok, I'll copy you on the next version of the patch. Turns out I have a
board as well, so I can at least do some basic testing.

> > There may have been a
> > reason why the read was done after the writes. Swapping the commands
> > may be the wrong thing to do. The dummy read earlier in this function
> > suggests that maybe changes to CCR do not take effect until you read
> > from (or write to) the DR register.
> 
> Unfortunately, I don't know enough about the I2C protocol to answer
> this, but I can provide this description on the DR register from the
> reference manual:
> 
>          Transmission starts when an address and the R/W bit are
> written to the data register and the I2C interface
>          performs as the master. A data transfer is initiated when
> data is written to the I2CDR. The most significant bit
>          is sent first in both cases. In master receive mode, reading
> the data register allows the read to occur, but also
>          allows the I2C module to receive the next byte of data on the
> I2C interface. In slave mode, the same function
>          is available after it is addressed. Note that in both master
> receive and slave receive modes, the very first read
>          is always a dummy read.
> 
> > Can't the above be rewritten to keep the order of the commands as it
> > was before? AFAICS it would only take one or two extra tests.
> 
> I think it's very important to keep the order as-is, if at all
> possible.  This driver is very old and the hardware is used on all of
> our SOCs, dating back several years.  It would be a nightmare to test
> it all over again.
> 
I'll see if I can work around that. 1-byte block reads may be tricky,
though, but maybe I can get that working as well.

Thanks,
Guenter

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Jean Delvare]

On Tue, 15 Nov 2011 11:14:47 -0800, Guenter Roeck wrote:
> On Tue, 2011-11-15 at 14:02 -0500, Tabi Timur-B04825 wrote:
> > I think it's very important to keep the order as-is, if at all
> > possible.  This driver is very old and the hardware is used on all of
> > our SOCs, dating back several years.  It would be a nightmare to test
> > it all over again.
>
> I'll see if I can work around that. 1-byte block reads may be tricky,
> though, but maybe I can get that working as well.

IMHO it's OK if 1-byte blocks are not supported. In practice the length
of blocks seems to be known in advance and it's never 1. We can revisit
if this is ever needed in practice for this specific bus driver.

-- 
Jean Delvare

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

On Tue, 2011-11-15 at 15:05 -0500, Jean Delvare wrote:
> On Tue, 15 Nov 2011 11:14:47 -0800, Guenter Roeck wrote:
> > On Tue, 2011-11-15 at 14:02 -0500, Tabi Timur-B04825 wrote:
> > > I think it's very important to keep the order as-is, if at all
> > > possible.  This driver is very old and the hardware is used on all of
> > > our SOCs, dating back several years.  It would be a nightmare to test
> > > it all over again.
> >
> > I'll see if I can work around that. 1-byte block reads may be tricky,
> > though, but maybe I can get that working as well.
> 
> IMHO it's OK if 1-byte blocks are not supported. In practice the length
> of blocks seems to be known in advance and it's never 1. We can revisit
> if this is ever needed in practice for this specific bus driver.
> 
I think I have a workaround for that condition. The workaround will only
apply if the length is 1, so the risk associated with it is low. I'll
test it and let you know if it works.

Guenter

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[York Sun]

I am not sure if this mail will reach the list as I am not subscribed.

In the v2 patch (actually both version), you wrote

> +		byte = readb(i2c->base + MPC_I2C_DR);
> +
> +		/* Adjust length if first received byte is length */
> +		if (i == 0 && recv_len) {
> +			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> +				return -EPROTO;
> +			length += byte;
> +			/*
> +			 * For block reads, generate txack here if data length
> +			 * is 1 byte (total length is 2 bytes).
> +			 */
> +			if (length == 2)
> +				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> +					 | CCR_TXAK);
> +		}
> +		data[i] = byte;

I am wondering if the first byte should be left out for data[] if the
recv_len is non-zero. If I understand correctly, this patch is to
support of SMBus with multiple byte read. The SMBus is different from
I2C bus on multiple byte. The first data is byte count vs slave data for
I2C. So you will receive all data preceded by the byte count, which is
one more than your loop.

York



On Wed, 2011-11-16 at 08:56 -0600, Timur Tabi wrote:
> 
> On Nov 16, 2011, at 12:01 AM, sun york-R58495 <R58495@freescale.com> wrote:
> 
> > Timur,
> > 
> > Which silicon has the i2c-mpc? Without the user's manual, I can only guess from the code. It looks like the code is going to deal with a block transaction with the first byte is the length. If that's true, there might be a problem with 
> > 
> > data[i]=byte
> > 
> > if the block is true. Since the first byte is the length, it shouldn't be the data at the same time, right?
> 
> Can you check thr mailing list thread?  It should explain everything.
> 
> 
> > 
> > York
> > 
> > ________________________________________
> > From: Tabi Timur-B04825
> > Sent: Tuesday, November 15, 2011 11:04 AM
> > To: sun york-R58495
> > Subject: Fwd: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
> > 
> > York,
> > 
> > If you have a moment, can you take a look at this patch?  I don't know
> > enough about I2C to really understand it.
> > 
> > ---------- Forwarded message ----------
> > From: Guenter Roeck <guenter.roeck@ericsson.com>
> > Date: Tue, Nov 15, 2011 at 12:27 AM
> > Subject: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
> > To: Jean Delvare <khali@linux-fr.org>, Ben Dooks <ben-linux@fluff.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>,
> > linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Guenter Roeck
> > <guenter.roeck@ericsson.com>, Tang Yuantian <B29983@freescale.com>
> > 
> > 
> > Add support for SMBUS_READ_BLOCK_DATA to the i2c-mpc bus driver.
> > Required to support the PMBus zl6100 driver with i2c-mpc.
> > 
> > Reported-by: Tang Yuantian <B29983@freescale.com>
> > Cc: Tang Yuantian <B29983@freescale.com>
> > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > ---
> > drivers/i2c/busses/i2c-mpc.c |   33 +++++++++++++++++++++++++--------
> > 1 files changed, 25 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index 107397a..77aade7 100644
> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -454,7 +454,7 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
> > }
> > 
> > static int mpc_read(struct mpc_i2c *i2c, int target,
> > -                   u8 *data, int length, int restart)
> > +                   u8 *data, int length, int restart, bool block)
> > {
> >       unsigned timeout = i2c->adap.timeout;
> >       int i, result;
> > @@ -470,7 +470,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
> >               return result;
> > 
> >       if (length) {
> > -               if (length == 1)
> > +               if (length == 1 && !block)
> >                       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
> >               else
> >                       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
> > @@ -479,17 +479,28 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
> >       }
> > 
> >       for (i = 0; i < length; i++) {
> > +               u8 byte;
> > +
> >               result = i2c_wait(i2c, timeout, 0);
> >               if (result < 0)
> >                       return result;
> > 
> > +               byte = readb(i2c->base + MPC_I2C_DR);
> > +               /*
> > +                * Adjust length if first received byte is length
> > +                */
> > +               if (i == 0 && block) {
> > +                       if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > +                               return -EPROTO;
> > +                       length += byte;
> > +               }
> > +               data[i] = byte;
> >               /* Generate txack on next to last byte */
> >               if (i == length - 2)
> >                       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
> >               /* Do not generate stop on last byte */
> >               if (i == length - 1)
> >                       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
> > -               data[i] = readb(i2c->base + MPC_I2C_DR);
> >       }
> > 
> >       return length;
> > @@ -532,12 +543,17 @@ static int mpc_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs, int num)
> >                       "Doing %s %d bytes to 0x%02x - %d of %d messages\n",
> >                       pmsg->flags & I2C_M_RD ? "read" : "write",
> >                       pmsg->len, pmsg->addr, i + 1, num);
> > -               if (pmsg->flags & I2C_M_RD)
> > -                       ret =
> > -                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> > -               else
> > +               if (pmsg->flags & I2C_M_RD) {
> > +                       bool block = pmsg->flags & I2C_M_RECV_LEN;
> > +
> > +                       ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
> > +                                      block);
> > +                       if (block && ret > 0)
> > +                               pmsg->len = ret;
> > +               } else {
> >                       ret =
> >                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> > +               }
> >       }
> >       mpc_i2c_stop(i2c);
> >       return (ret < 0) ? ret : num;
> > @@ -545,7 +561,8 @@ static int mpc_xfer(struct i2c_adapter *adap,
> > struct i2c_msg *msgs, int num)
> > 
> > static u32 mpc_functionality(struct i2c_adapter *adap)
> > {
> > -       return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> > +       return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> > +         | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> > }
> > 
> > static const struct i2c_algorithm mpc_algo = {
> > --
> > 1.7.3.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> > 
> > --
> > Timur Tabi
> > Linux kernel developer at Freescale

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

On Wed, 2011-11-16 at 12:28 -0500, York Sun wrote:
> I am not sure if this mail will reach the list as I am not subscribed.
> 
> In the v2 patch (actually both version), you wrote
> 
> > +		byte = readb(i2c->base + MPC_I2C_DR);
> > +
> > +		/* Adjust length if first received byte is length */
> > +		if (i == 0 && recv_len) {
> > +			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > +				return -EPROTO;
> > +			length += byte;
> > +			/*
> > +			 * For block reads, generate txack here if data length
> > +			 * is 1 byte (total length is 2 bytes).
> > +			 */
> > +			if (length == 2)
> > +				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> > +					 | CCR_TXAK);
> > +		}
> > +		data[i] = byte;
> 
> I am wondering if the first byte should be left out for data[] if the
> recv_len is non-zero. If I understand correctly, this patch is to
> support of SMBus with multiple byte read. The SMBus is different from
> I2C bus on multiple byte. The first data is byte count vs slave data for
> I2C. So you will receive all data preceded by the byte count, which is
> one more than your loop.
> 
York,

The calling code expects the data length in data[0], and the actual data
in data[1] .. data[<byte_count>]. The initial value for length is 1; the
byte count is added to it, so <byte count + 1> bytes are placed into the
buffer.

Thanks,
Guenter

> York
> 
> 
> 
> On Wed, 2011-11-16 at 08:56 -0600, Timur Tabi wrote:
> > 
> > On Nov 16, 2011, at 12:01 AM, sun york-R58495 <R58495@freescale.com> wrote:
> > 
> > > Timur,
> > > 
> > > Which silicon has the i2c-mpc? Without the user's manual, I can only guess from the code. It looks like the code is going to deal with a block transaction with the first byte is the length. If that's true, there might be a problem with 
> > > 
> > > data[i]=byte
> > > 
> > > if the block is true. Since the first byte is the length, it shouldn't be the data at the same time, right?
> > 
> > Can you check thr mailing list thread?  It should explain everything.
> > 
> > 
> > > 
> > > York
> > > 
> > > ________________________________________
> > > From: Tabi Timur-B04825
> > > Sent: Tuesday, November 15, 2011 11:04 AM
> > > To: sun york-R58495
> > > Subject: Fwd: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
> > > 
> > > York,
> > > 
> > > If you have a moment, can you take a look at this patch?  I don't know
> > > enough about I2C to really understand it.
> > > 
> > > ---------- Forwarded message ----------
> > > From: Guenter Roeck <guenter.roeck@ericsson.com>
> > > Date: Tue, Nov 15, 2011 at 12:27 AM
> > > Subject: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA
> > > To: Jean Delvare <khali@linux-fr.org>, Ben Dooks <ben-linux@fluff.org>
> > > Cc: Grant Likely <grant.likely@secretlab.ca>,
> > > linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Guenter Roeck
> > > <guenter.roeck@ericsson.com>, Tang Yuantian <B29983@freescale.com>
> > > 
> > > 
> > > Add support for SMBUS_READ_BLOCK_DATA to the i2c-mpc bus driver.
> > > Required to support the PMBus zl6100 driver with i2c-mpc.
> > > 
> > > Reported-by: Tang Yuantian <B29983@freescale.com>
> > > Cc: Tang Yuantian <B29983@freescale.com>
> > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > ---
> > > drivers/i2c/busses/i2c-mpc.c |   33 +++++++++++++++++++++++++--------
> > > 1 files changed, 25 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > > index 107397a..77aade7 100644
> > > --- a/drivers/i2c/busses/i2c-mpc.c
> > > +++ b/drivers/i2c/busses/i2c-mpc.c
> > > @@ -454,7 +454,7 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
> > > }
> > > 
> > > static int mpc_read(struct mpc_i2c *i2c, int target,
> > > -                   u8 *data, int length, int restart)
> > > +                   u8 *data, int length, int restart, bool block)
> > > {
> > >       unsigned timeout = i2c->adap.timeout;
> > >       int i, result;
> > > @@ -470,7 +470,7 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
> > >               return result;
> > > 
> > >       if (length) {
> > > -               if (length == 1)
> > > +               if (length == 1 && !block)
> > >                       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
> > >               else
> > >                       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA);
> > > @@ -479,17 +479,28 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
> > >       }
> > > 
> > >       for (i = 0; i < length; i++) {
> > > +               u8 byte;
> > > +
> > >               result = i2c_wait(i2c, timeout, 0);
> > >               if (result < 0)
> > >                       return result;
> > > 
> > > +               byte = readb(i2c->base + MPC_I2C_DR);
> > > +               /*
> > > +                * Adjust length if first received byte is length
> > > +                */
> > > +               if (i == 0 && block) {
> > > +                       if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > > +                               return -EPROTO;
> > > +                       length += byte;
> > > +               }
> > > +               data[i] = byte;
> > >               /* Generate txack on next to last byte */
> > >               if (i == length - 2)
> > >                       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
> > >               /* Do not generate stop on last byte */
> > >               if (i == length - 1)
> > >                       writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
> > > -               data[i] = readb(i2c->base + MPC_I2C_DR);
> > >       }
> > > 
> > >       return length;
> > > @@ -532,12 +543,17 @@ static int mpc_xfer(struct i2c_adapter *adap,
> > > struct i2c_msg *msgs, int num)
> > >                       "Doing %s %d bytes to 0x%02x - %d of %d messages\n",
> > >                       pmsg->flags & I2C_M_RD ? "read" : "write",
> > >                       pmsg->len, pmsg->addr, i + 1, num);
> > > -               if (pmsg->flags & I2C_M_RD)
> > > -                       ret =
> > > -                           mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> > > -               else
> > > +               if (pmsg->flags & I2C_M_RD) {
> > > +                       bool block = pmsg->flags & I2C_M_RECV_LEN;
> > > +
> > > +                       ret = mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i,
> > > +                                      block);
> > > +                       if (block && ret > 0)
> > > +                               pmsg->len = ret;
> > > +               } else {
> > >                       ret =
> > >                           mpc_write(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
> > > +               }
> > >       }
> > >       mpc_i2c_stop(i2c);
> > >       return (ret < 0) ? ret : num;
> > > @@ -545,7 +561,8 @@ static int mpc_xfer(struct i2c_adapter *adap,
> > > struct i2c_msg *msgs, int num)
> > > 
> > > static u32 mpc_functionality(struct i2c_adapter *adap)
> > > {
> > > -       return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> > > +       return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
> > > +         | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> > > }
> > > 
> > > static const struct i2c_algorithm mpc_algo = {
> > > --
> > > 1.7.3.1
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > 
> > > 
> > > --
> > > Timur Tabi
> > > Linux kernel developer at Freescale
> 
> 
> 

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[York Sun]

On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> On Wed, 2011-11-16 at 12:28 -0500, York Sun wrote:
> > I am not sure if this mail will reach the list as I am not subscribed.
> > 
> > In the v2 patch (actually both version), you wrote
> > 
> > > +		byte = readb(i2c->base + MPC_I2C_DR);
> > > +
> > > +		/* Adjust length if first received byte is length */
> > > +		if (i == 0 && recv_len) {
> > > +			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > > +				return -EPROTO;
> > > +			length += byte;
> > > +			/*
> > > +			 * For block reads, generate txack here if data length
> > > +			 * is 1 byte (total length is 2 bytes).
> > > +			 */
> > > +			if (length == 2)
> > > +				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> > > +					 | CCR_TXAK);
> > > +		}
> > > +		data[i] = byte;
> > 
> > I am wondering if the first byte should be left out for data[] if the
> > recv_len is non-zero. If I understand correctly, this patch is to
> > support of SMBus with multiple byte read. The SMBus is different from
> > I2C bus on multiple byte. The first data is byte count vs slave data for
> > I2C. So you will receive all data preceded by the byte count, which is
> > one more than your loop.
> > 
> York,
> 
> The calling code expects the data length in data[0], and the actual data
> in data[1] .. data[<byte_count>]. The initial value for length is 1; the
> byte count is added to it, so <byte count + 1> bytes are placed into the
> buffer.
> 
> Thanks,
> Guenter

Thanks for explanation. I am more confused by the length += byte now.
For I2C bus, if you need length of byte, just keep reading until you get
all of them. Of course you need to deal with the ACK. For SMBus, it is
similar but you shouldn't read more after the byte count which is in the
first data. If you want to read length of data but the block size is
bigger than length, you should call block read at first place. If the
block size is smaller than length, why increase the length? Does your
SMBus controller only support fixed block size and not support single
byte read? If it does, I would do

Block, Block, Block, byte, byte... until length of data

Regards,

York

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

On Wed, 2011-11-16 at 12:55 -0500, York Sun wrote:
> On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> > On Wed, 2011-11-16 at 12:28 -0500, York Sun wrote:
> > > I am not sure if this mail will reach the list as I am not subscribed.
> > > 
> > > In the v2 patch (actually both version), you wrote
> > > 
> > > > +		byte = readb(i2c->base + MPC_I2C_DR);
> > > > +
> > > > +		/* Adjust length if first received byte is length */
> > > > +		if (i == 0 && recv_len) {
> > > > +			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > > > +				return -EPROTO;
> > > > +			length += byte;
> > > > +			/*
> > > > +			 * For block reads, generate txack here if data length
> > > > +			 * is 1 byte (total length is 2 bytes).
> > > > +			 */
> > > > +			if (length == 2)
> > > > +				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> > > > +					 | CCR_TXAK);
> > > > +		}
> > > > +		data[i] = byte;
> > > 
> > > I am wondering if the first byte should be left out for data[] if the
> > > recv_len is non-zero. If I understand correctly, this patch is to
> > > support of SMBus with multiple byte read. The SMBus is different from
> > > I2C bus on multiple byte. The first data is byte count vs slave data for
> > > I2C. So you will receive all data preceded by the byte count, which is
> > > one more than your loop.
> > > 
> > York,
> > 
> > The calling code expects the data length in data[0], and the actual data
> > in data[1] .. data[<byte_count>]. The initial value for length is 1; the
> > byte count is added to it, so <byte count + 1> bytes are placed into the
> > buffer.
> > 
> > Thanks,
> > Guenter
> 
> Thanks for explanation. I am more confused by the length += byte now.
> For I2C bus, if you need length of byte, just keep reading until you get
> all of them. Of course you need to deal with the ACK. For SMBus, it is
> similar but you shouldn't read more after the byte count which is in the
> first data. If you want to read length of data but the block size is
> bigger than length, you should call block read at first place. If the
> block size is smaller than length, why increase the length? Does your
> SMBus controller only support fixed block size and not support single
> byte read? If it does, I would do
> 
> Block, Block, Block, byte, byte... until length of data
> 
York,

you lost me there. The added code only applies to SMBus block reads
(recv_len is true), for which the length of the to-be-received block is
in the first data byte, and must be added to the total number of bytes
to receive. The implementation is exactly the same in all other drivers
supporting the RECV_LEN flag.

Guenter

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Jean Delvare]

On Wed, 16 Nov 2011 09:55:35 -0800, York Sun wrote:
> On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> > York,
> > 
> > The calling code expects the data length in data[0], and the actual data
> > in data[1] .. data[<byte_count>]. The initial value for length is 1; the
> > byte count is added to it, so <byte count + 1> bytes are placed into the
> > buffer.
> > 
> > Thanks,
> > Guenter
> 
> Thanks for explanation. I am more confused by the length += byte now.
> For I2C bus, if you need length of byte, just keep reading until you get
> all of them. Of course you need to deal with the ACK. For SMBus, it is
> similar but you shouldn't read more after the byte count which is in the
> first data.

You shouldn't read less either. The slave tells how much bytes it wants
to send, and the master must honor that.

> If you want to read length of data but the block size is
> bigger than length, you should call block read at first place. If the
> block size is smaller than length, why increase the length? Does your
> SMBus controller only support fixed block size and not support single
> byte read? If it does, I would do
> 
> Block, Block, Block, byte, byte... until length of data

Your thinking is too focused on I2C block reads (or even block read of
data over the network or on disk). SMBus block read is something
completely different. It's not about reading 200 bytes of data and
receiving it in 16-byte chunks (I2C block read works that way, on
EEPROMs in particular.) There is no "data length" and "block size" to
compare to each other. It's about reading the value of _one_ register
and this value happens to be multi-byte. There is typically _no_
register pointer increment (automatic or not) involved as can happen
with EEPROMs. If an SMBus block read from register N returns 10 bytes,
you're not going to read the next 10 bytes from register N+10. There
are no "next 10 bytes" to read, and register N+10 is something
completely unrelated.

And for this reason, it is not possible to mix SMBus block reads with
byte reads, as can be done with I2C block reads.

Also note that there is a limit of 32 bytes for SMBus block transfers,
per SMBus specification. All slaves and masters must comply with it.

I hope I managed to clarify the case this time...

-- 
Jean Delvare

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[York Sun]

On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> On Wed, 16 Nov 2011 09:55:35 -0800, York Sun wrote:
> > On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> > > York,
> > > 
> > > The calling code expects the data length in data[0], and the actual data
> > > in data[1] .. data[<byte_count>]. The initial value for length is 1; the
> > > byte count is added to it, so <byte count + 1> bytes are placed into the
> > > buffer.
> > > 
> > > Thanks,
> > > Guenter
> > 
> > Thanks for explanation. I am more confused by the length += byte now.
> > For I2C bus, if you need length of byte, just keep reading until you get
> > all of them. Of course you need to deal with the ACK. For SMBus, it is
> > similar but you shouldn't read more after the byte count which is in the
> > first data.
> 
> You shouldn't read less either. The slave tells how much bytes it wants
> to send, and the master must honor that.
> 
> > If you want to read length of data but the block size is
> > bigger than length, you should call block read at first place. If the
> > block size is smaller than length, why increase the length? Does your
> > SMBus controller only support fixed block size and not support single
> > byte read? If it does, I would do
> > 
> > Block, Block, Block, byte, byte... until length of data
> 
> Your thinking is too focused on I2C block reads (or even block read of
> data over the network or on disk). SMBus block read is something
> completely different. It's not about reading 200 bytes of data and
> receiving it in 16-byte chunks (I2C block read works that way, on
> EEPROMs in particular.) There is no "data length" and "block size" to
> compare to each other. It's about reading the value of _one_ register
> and this value happens to be multi-byte. There is typically _no_
> register pointer increment (automatic or not) involved as can happen
> with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> you're not going to read the next 10 bytes from register N+10. There
> are no "next 10 bytes" to read, and register N+10 is something
> completely unrelated.
> 
> And for this reason, it is not possible to mix SMBus block reads with
> byte reads, as can be done with I2C block reads.
> 
> Also note that there is a limit of 32 bytes for SMBus block transfers,
> per SMBus specification. All slaves and masters must comply with it.
> 
> I hope I managed to clarify the case this time...
> 

You have made it much clear. If block size is fixed and block read
cannot mix with byte read, shall we do this

if length < block_size
   read block_size
else {
   while (length) {
       read block_size
       length -= block_size
   }

York

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

On Wed, 2011-11-16 at 13:20 -0500, York Sun wrote:
> On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > On Wed, 16 Nov 2011 09:55:35 -0800, York Sun wrote:
> > > On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> > > > York,
> > > > 
> > > > The calling code expects the data length in data[0], and the actual data
> > > > in data[1] .. data[<byte_count>]. The initial value for length is 1; the
> > > > byte count is added to it, so <byte count + 1> bytes are placed into the
> > > > buffer.
> > > > 
> > > > Thanks,
> > > > Guenter
> > > 
> > > Thanks for explanation. I am more confused by the length += byte now.
> > > For I2C bus, if you need length of byte, just keep reading until you get
> > > all of them. Of course you need to deal with the ACK. For SMBus, it is
> > > similar but you shouldn't read more after the byte count which is in the
> > > first data.
> > 
> > You shouldn't read less either. The slave tells how much bytes it wants
> > to send, and the master must honor that.
> > 
> > > If you want to read length of data but the block size is
> > > bigger than length, you should call block read at first place. If the
> > > block size is smaller than length, why increase the length? Does your
> > > SMBus controller only support fixed block size and not support single
> > > byte read? If it does, I would do
> > > 
> > > Block, Block, Block, byte, byte... until length of data
> > 
> > Your thinking is too focused on I2C block reads (or even block read of
> > data over the network or on disk). SMBus block read is something
> > completely different. It's not about reading 200 bytes of data and
> > receiving it in 16-byte chunks (I2C block read works that way, on
> > EEPROMs in particular.) There is no "data length" and "block size" to
> > compare to each other. It's about reading the value of _one_ register
> > and this value happens to be multi-byte. There is typically _no_
> > register pointer increment (automatic or not) involved as can happen
> > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > you're not going to read the next 10 bytes from register N+10. There
> > are no "next 10 bytes" to read, and register N+10 is something
> > completely unrelated.
> > 
> > And for this reason, it is not possible to mix SMBus block reads with
> > byte reads, as can be done with I2C block reads.
> > 
> > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > per SMBus specification. All slaves and masters must comply with it.
> > 
> > I hope I managed to clarify the case this time...
> > 
> 
> You have made it much clear. If block size is fixed and block read
> cannot mix with byte read, shall we do this
> 
> if length < block_size
>    read block_size
> else {
>    while (length) {
>        read block_size
>        length -= block_size
>    }
> 

I am giving up. I don't need SMBus block reads to work on our board, I
am having trouble installing my test image on it, I don't see
constructive feedback but only a theoretic discussion about how SMBus
block reads work or are perceived to work or should work, for which I
don't have time, and we already know that the zl6100 driver (which
originated this discussion and was the reason for the patch) is working
fine with the patch applied.

I simply don't have time for this, and spent way too much time on it
already. Jean, please drop this patch. If someone else needs it, he or
she can revive and re-submit it. If folks can complain about the patch
w/o understanding how SMBus block reads work, but are not willing to
test it, something is wrong anyway.

Thanks,
Guenter

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Timur Tabi]

Guenter Roeck wrote:
> I simply don't have time for this, and spent way too much time on it
> already. Jean, please drop this patch. If someone else needs it, he or
> she can revive and re-submit it. If folks can complain about the patch
> w/o understanding how SMBus block reads work, but are not willing to
> test it, something is wrong anyway.

I don't see York's posts as a complaint at all, just someone with a vested interest in this code trying to understand the patch.  I definitely appreciate the effort you have put into it already, and I'm disappointed that you think that this is now a waste of your time.

-- 
Timur Tabi
Linux kernel developer at Freescale

RE: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[sun york-R58495]

Come on, I am trying to help. You said in the patch you have not been able to test this patch.

I would support your patch only if I can understand why it works.

York

________________________________________
From: Guenter Roeck [guenter.roeck@ericsson.com]
Sent: Wednesday, November 16, 2011 10:51 AM
To: sun york-R58495
Cc: Jean Delvare; Tabi Timur-B04825; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Tang Yuantian-B29983
Subject: Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

On Wed, 2011-11-16 at 13:20 -0500, York Sun wrote:
> On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > On Wed, 16 Nov 2011 09:55:35 -0800, York Sun wrote:
> > > On Wed, 2011-11-16 at 09:36 -0800, Guenter Roeck wrote:
> > > > York,
> > > >
> > > > The calling code expects the data length in data[0], and the actual data
> > > > in data[1] .. data[<byte_count>]. The initial value for length is 1; the
> > > > byte count is added to it, so <byte count + 1> bytes are placed into the
> > > > buffer.
> > > >
> > > > Thanks,
> > > > Guenter
> > >
> > > Thanks for explanation. I am more confused by the length += byte now.
> > > For I2C bus, if you need length of byte, just keep reading until you get
> > > all of them. Of course you need to deal with the ACK. For SMBus, it is
> > > similar but you shouldn't read more after the byte count which is in the
> > > first data.
> >
> > You shouldn't read less either. The slave tells how much bytes it wants
> > to send, and the master must honor that.
> >
> > > If you want to read length of data but the block size is
> > > bigger than length, you should call block read at first place. If the
> > > block size is smaller than length, why increase the length? Does your
> > > SMBus controller only support fixed block size and not support single
> > > byte read? If it does, I would do
> > >
> > > Block, Block, Block, byte, byte... until length of data
> >
> > Your thinking is too focused on I2C block reads (or even block read of
> > data over the network or on disk). SMBus block read is something
> > completely different. It's not about reading 200 bytes of data and
> > receiving it in 16-byte chunks (I2C block read works that way, on
> > EEPROMs in particular.) There is no "data length" and "block size" to
> > compare to each other. It's about reading the value of _one_ register
> > and this value happens to be multi-byte. There is typically _no_
> > register pointer increment (automatic or not) involved as can happen
> > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > you're not going to read the next 10 bytes from register N+10. There
> > are no "next 10 bytes" to read, and register N+10 is something
> > completely unrelated.
> >
> > And for this reason, it is not possible to mix SMBus block reads with
> > byte reads, as can be done with I2C block reads.
> >
> > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > per SMBus specification. All slaves and masters must comply with it.
> >
> > I hope I managed to clarify the case this time...
> >
>
> You have made it much clear. If block size is fixed and block read
> cannot mix with byte read, shall we do this
>
> if length < block_size
>    read block_size
> else {
>    while (length) {
>        read block_size
>        length -= block_size
>    }
>

I am giving up. I don't need SMBus block reads to work on our board, I
am having trouble installing my test image on it, I don't see
constructive feedback but only a theoretic discussion about how SMBus
block reads work or are perceived to work or should work, for which I
don't have time, and we already know that the zl6100 driver (which
originated this discussion and was the reason for the patch) is working
fine with the patch applied.

I simply don't have time for this, and spent way too much time on it
already. Jean, please drop this patch. If someone else needs it, he or
she can revive and re-submit it. If folks can complain about the patch
w/o understanding how SMBus block reads work, but are not willing to
test it, something is wrong anyway.

Thanks,
Guenter

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Jean Delvare]

On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > Your thinking is too focused on I2C block reads (or even block read of
> > data over the network or on disk). SMBus block read is something
> > completely different. It's not about reading 200 bytes of data and
> > receiving it in 16-byte chunks (I2C block read works that way, on
> > EEPROMs in particular.) There is no "data length" and "block size" to
> > compare to each other. It's about reading the value of _one_ register
> > and this value happens to be multi-byte. There is typically _no_
> > register pointer increment (automatic or not) involved as can happen
> > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > you're not going to read the next 10 bytes from register N+10. There
> > are no "next 10 bytes" to read, and register N+10 is something
> > completely unrelated.
> > 
> > And for this reason, it is not possible to mix SMBus block reads with
> > byte reads, as can be done with I2C block reads.
> > 
> > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > per SMBus specification. All slaves and masters must comply with it.
> > 
> > I hope I managed to clarify the case this time...
> 
> You have made it much clear. If block size is fixed and block read
> cannot mix with byte read, shall we do this
> 
> if length < block_size
>    read block_size
> else {
>    while (length) {
>        read block_size
>        length -= block_size
>    }

Which part of

  There is no "data length" and "block size" to compare to each other.

did you not understand?

-- 
Jean Delvare

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[York Sun]

On Wed, 2011-11-16 at 20:10 +0100, Jean Delvare wrote:
> On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> > On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > > Your thinking is too focused on I2C block reads (or even block read of
> > > data over the network or on disk). SMBus block read is something
> > > completely different. It's not about reading 200 bytes of data and
> > > receiving it in 16-byte chunks (I2C block read works that way, on
> > > EEPROMs in particular.) There is no "data length" and "block size" to
> > > compare to each other. It's about reading the value of _one_ register
> > > and this value happens to be multi-byte. There is typically _no_
> > > register pointer increment (automatic or not) involved as can happen
> > > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > > you're not going to read the next 10 bytes from register N+10. There
> > > are no "next 10 bytes" to read, and register N+10 is something
> > > completely unrelated.
> > > 
> > > And for this reason, it is not possible to mix SMBus block reads with
> > > byte reads, as can be done with I2C block reads.
> > > 
> > > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > > per SMBus specification. All slaves and masters must comply with it.
> > > 
> > > I hope I managed to clarify the case this time...
> > 
> > You have made it much clear. If block size is fixed and block read
> > cannot mix with byte read, shall we do this
> > 
> > if length < block_size
> >    read block_size
> > else {
> >    while (length) {
> >        read block_size
> >        length -= block_size
> >    }
> 
> Which part of
> 
>   There is no "data length" and "block size" to compare to each other.
> 
> did you not understand?

For example, if the length is 40 and the block size is 32, are you going
to read 32, 72 byte or 64 byte?

York

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Jean Delvare]

On Wed, 16 Nov 2011 11:15:15 -0800, York Sun wrote:
> On Wed, 2011-11-16 at 20:10 +0100, Jean Delvare wrote:
> > On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> > > On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > > > Your thinking is too focused on I2C block reads (or even block read of
> > > > data over the network or on disk). SMBus block read is something
> > > > completely different. It's not about reading 200 bytes of data and
> > > > receiving it in 16-byte chunks (I2C block read works that way, on
> > > > EEPROMs in particular.) There is no "data length" and "block size" to
> > > > compare to each other. It's about reading the value of _one_ register
> > > > and this value happens to be multi-byte. There is typically _no_
> > > > register pointer increment (automatic or not) involved as can happen
> > > > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > > > you're not going to read the next 10 bytes from register N+10. There
> > > > are no "next 10 bytes" to read, and register N+10 is something
> > > > completely unrelated.
> > > > 
> > > > And for this reason, it is not possible to mix SMBus block reads with
> > > > byte reads, as can be done with I2C block reads.
> > > > 
> > > > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > > > per SMBus specification. All slaves and masters must comply with it.
> > > > 
> > > > I hope I managed to clarify the case this time...
> > > 
> > > You have made it much clear. If block size is fixed and block read
> > > cannot mix with byte read, shall we do this
> > > 
> > > if length < block_size
> > >    read block_size
> > > else {
> > >    while (length) {
> > >        read block_size
> > >        length -= block_size
> > >    }
> > 
> > Which part of
> > 
> >   There is no "data length" and "block size" to compare to each other.
> > 
> > did you not understand?
> 
> For example, if the length is 40 and the block size is 32, are you going
> to read 32, 72 byte or 64 byte?

The length is not 40. Which part of

  Also note that there is a limit of 32 bytes for SMBus block transfers,
  per SMBus specification. All slaves and masters must comply with it.

did you now understand?

Really, please. I understand you're only trying to help and understand
the patch, but at some point, if you have zero knowledge about the
topic, you're not the right person to review the patch, period. You are
still welcome to test the patch if you can and want, that's a
completely different task.

-- 
Jean Delvare

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[York Sun]

On Wed, 2011-11-16 at 20:18 +0100, Jean Delvare wrote:
> On Wed, 16 Nov 2011 11:15:15 -0800, York Sun wrote:
> > On Wed, 2011-11-16 at 20:10 +0100, Jean Delvare wrote:
> > > On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> > > > On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > > > > Your thinking is too focused on I2C block reads (or even block read of
> > > > > data over the network or on disk). SMBus block read is something
> > > > > completely different. It's not about reading 200 bytes of data and
> > > > > receiving it in 16-byte chunks (I2C block read works that way, on
> > > > > EEPROMs in particular.) There is no "data length" and "block size" to
> > > > > compare to each other. It's about reading the value of _one_ register
> > > > > and this value happens to be multi-byte. There is typically _no_
> > > > > register pointer increment (automatic or not) involved as can happen
> > > > > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > > > > you're not going to read the next 10 bytes from register N+10. There
> > > > > are no "next 10 bytes" to read, and register N+10 is something
> > > > > completely unrelated.
> > > > > 
> > > > > And for this reason, it is not possible to mix SMBus block reads with
> > > > > byte reads, as can be done with I2C block reads.
> > > > > 
> > > > > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > > > > per SMBus specification. All slaves and masters must comply with it.
> > > > > 
> > > > > I hope I managed to clarify the case this time...
> > > > 
> > > > You have made it much clear. If block size is fixed and block read
> > > > cannot mix with byte read, shall we do this
> > > > 
> > > > if length < block_size
> > > >    read block_size
> > > > else {
> > > >    while (length) {
> > > >        read block_size
> > > >        length -= block_size
> > > >    }
> > > 
> > > Which part of
> > > 
> > >   There is no "data length" and "block size" to compare to each other.
> > > 
> > > did you not understand?
> > 
> > For example, if the length is 40 and the block size is 32, are you going
> > to read 32, 72 byte or 64 byte?
> 
> The length is not 40. Which part of
> 
>   Also note that there is a limit of 32 bytes for SMBus block transfers,
>   per SMBus specification. All slaves and masters must comply with it.
> 
> did you now understand?
> 
> Really, please. I understand you're only trying to help and understand
> the patch, but at some point, if you have zero knowledge about the
> topic, you're not the right person to review the patch, period. You are
> still welcome to test the patch if you can and want, that's a
> completely different task.
> 

That's the point. Your patch doesn't check if the length is valid. You
rely on the caller to know no more than 32 bytes can be transferred. It
shouldn't limit the subfunction to transfer more than 32 bytes if
hardware can support it by multiple transactions. If not, print out an
error message.

York

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

On Wed, 2011-11-16 at 14:24 -0500, York Sun wrote:
> On Wed, 2011-11-16 at 20:18 +0100, Jean Delvare wrote:
> > On Wed, 16 Nov 2011 11:15:15 -0800, York Sun wrote:
> > > On Wed, 2011-11-16 at 20:10 +0100, Jean Delvare wrote:
> > > > On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> > > > > On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > > > > > Your thinking is too focused on I2C block reads (or even block read of
> > > > > > data over the network or on disk). SMBus block read is something
> > > > > > completely different. It's not about reading 200 bytes of data and
> > > > > > receiving it in 16-byte chunks (I2C block read works that way, on
> > > > > > EEPROMs in particular.) There is no "data length" and "block size" to
> > > > > > compare to each other. It's about reading the value of _one_ register
> > > > > > and this value happens to be multi-byte. There is typically _no_
> > > > > > register pointer increment (automatic or not) involved as can happen
> > > > > > with EEPROMs. If an SMBus block read from register N returns 10 bytes,
> > > > > > you're not going to read the next 10 bytes from register N+10. There
> > > > > > are no "next 10 bytes" to read, and register N+10 is something
> > > > > > completely unrelated.
> > > > > > 
> > > > > > And for this reason, it is not possible to mix SMBus block reads with
> > > > > > byte reads, as can be done with I2C block reads.
> > > > > > 
> > > > > > Also note that there is a limit of 32 bytes for SMBus block transfers,
> > > > > > per SMBus specification. All slaves and masters must comply with it.
> > > > > > 
> > > > > > I hope I managed to clarify the case this time...
> > > > > 
> > > > > You have made it much clear. If block size is fixed and block read
> > > > > cannot mix with byte read, shall we do this
> > > > > 
> > > > > if length < block_size
> > > > >    read block_size
> > > > > else {
> > > > >    while (length) {
> > > > >        read block_size
> > > > >        length -= block_size
> > > > >    }
> > > > 
> > > > Which part of
> > > > 
> > > >   There is no "data length" and "block size" to compare to each other.
> > > > 
> > > > did you not understand?
> > > 
> > > For example, if the length is 40 and the block size is 32, are you going
> > > to read 32, 72 byte or 64 byte?
> > 
> > The length is not 40. Which part of
> > 
> >   Also note that there is a limit of 32 bytes for SMBus block transfers,
> >   per SMBus specification. All slaves and masters must comply with it.
> > 
> > did you now understand?
> > 
> > Really, please. I understand you're only trying to help and understand
> > the patch, but at some point, if you have zero knowledge about the
> > topic, you're not the right person to review the patch, period. You are
> > still welcome to test the patch if you can and want, that's a
> > completely different task.
> > 
> 
> That's the point. Your patch doesn't check if the length is valid. You
> rely on the caller to know no more than 32 bytes can be transferred. It
> shouldn't limit the subfunction to transfer more than 32 bytes if
> hardware can support it by multiple transactions. If not, print out an
> error message.
> 

It is customary for kernel functions to only validate parameters
wherever a value enters or leaves the kernel, or at least a logical
boundary. Anything else would blow up the kernel size to a point where
it would be unusable. The patch checks if the block length received from
the SMBus slave is correct, which is exactly what it is supposed to do.

If you look closely, you may realize that the mpc_read also doesn't
validate of any of the other parameters. Are you going to request that
such validations be added as well ? How about the other functions in
this driver ? Should each function also validate each of its
parameters ? If not, where do you draw the line ?

Besides, the caller is the SMBus block read function, which does know
that SMBus block reads are limited to 32 bytes (plus length).

Guenter

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[York Sun]

> > That's the point. Your patch doesn't check if the length is valid. You
> > rely on the caller to know no more than 32 bytes can be transferred. It
> > shouldn't limit the subfunction to transfer more than 32 bytes if
> > hardware can support it by multiple transactions. If not, print out an
> > error message.
> > 
> 
> It is customary for kernel functions to only validate parameters
> wherever a value enters or leaves the kernel, or at least a logical
> boundary. Anything else would blow up the kernel size to a point where
> it would be unusable. The patch checks if the block length received from
> the SMBus slave is correct, which is exactly what it is supposed to do.
> 
> If you look closely, you may realize that the mpc_read also doesn't
> validate of any of the other parameters. Are you going to request that
> such validations be added as well ? How about the other functions in
> this driver ? Should each function also validate each of its
> parameters ? If not, where do you draw the line ?
> 
> Besides, the caller is the SMBus block read function, which does know
> that SMBus block reads are limited to 32 bytes (plus length).
> 

I am going to let it go for now. Obviously your patch is working when
length=1. Probably it will never be called with length other than 1 for
SMBus block read. It would be nicer if you could put a comment there
just in case in the future someone runs into a case where length+=byte
causes a problem.

York

RE: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Tang Yuantian-B29983]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1076 bytes --]

I have tested this patch. It works on p1022ds platform.
Thanks.

Regards,
Yuantian

> -----Original Message-----
> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> Sent: 2011年11月18日 2:19
> To: sun york-R58495
> Cc: Jean Delvare; Tabi Timur-B04825; linux-i2c@vger.kernel.org; linux-
> kernel@vger.kernel.org; Tang Yuantian-B29983
> Subject: Re: [PATCH] i2c/busses: (mpc) Add support for
> SMBUS_READ_BLOCK_DATA
> 
> If you look closely, you may realize that the mpc_read also doesn't
> validate of any of the other parameters. Are you going to request that
> such validations be added as well ? How about the other functions in this
> driver ? Should each function also validate each of its parameters ? If
> not, where do you draw the line ?
> 
> Besides, the caller is the SMBus block read function, which does know
> that SMBus block reads are limited to 32 bytes (plus length).
> 
> Guenter
> 
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Tang Yuantian-B29983]

Hi,

Has the patch been accepted?

Regards,
Yuantian

> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: 2011年11月17日 3:19
> To: sun york-R58495
> Cc: guenter.roeck@ericsson.com; Tabi Timur-B04825; linux-
> i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Tang Yuantian-B29983
> Subject: Re: [PATCH] i2c/busses: (mpc) Add support for
> SMBUS_READ_BLOCK_DATA
> 
> On Wed, 16 Nov 2011 11:15:15 -0800, York Sun wrote:
> > On Wed, 2011-11-16 at 20:10 +0100, Jean Delvare wrote:
> > > On Wed, 16 Nov 2011 10:20:38 -0800, York Sun wrote:
> > > > On Wed, 2011-11-16 at 19:09 +0100, Jean Delvare wrote:
> > > > > Your thinking is too focused on I2C block reads (or even block
> > > > > read of data over the network or on disk). SMBus block read is
> > > > > something completely different. It's not about reading 200 bytes
> > > > > of data and receiving it in 16-byte chunks (I2C block read works
> > > > > that way, on EEPROMs in particular.) There is no "data length"
> > > > > and "block size" to compare to each other. It's about reading
> > > > > the value of _one_ register and this value happens to be
> > > > > multi-byte. There is typically _no_ register pointer increment
> > > > > (automatic or not) involved as can happen with EEPROMs. If an
> > > > > SMBus block read from register N returns 10 bytes, you're not
> > > > > going to read the next 10 bytes from register N+10. There are no
> > > > > "next 10 bytes" to read, and register N+10 is something
> completely unrelated.
> > > > >
> > > > > And for this reason, it is not possible to mix SMBus block reads
> > > > > with byte reads, as can be done with I2C block reads.
> > > > >
> > > > > Also note that there is a limit of 32 bytes for SMBus block
> > > > > transfers, per SMBus specification. All slaves and masters must
> comply with it.
> > > > >
> > > > > I hope I managed to clarify the case this time...
> > > >
> > > > You have made it much clear. If block size is fixed and block read
> > > > cannot mix with byte read, shall we do this
> > > >
> > > > if length < block_size
> > > >    read block_size
> > > > else {
> > > >    while (length) {
> > > >        read block_size
> > > >        length -= block_size
> > > >    }
> > >
> > > Which part of
> > >
> > >   There is no "data length" and "block size" to compare to each other.
> > >
> > > did you not understand?
> >
> > For example, if the length is 40 and the block size is 32, are you
> > going to read 32, 72 byte or 64 byte?
> 
> The length is not 40. Which part of
> 
>   Also note that there is a limit of 32 bytes for SMBus block transfers,
>   per SMBus specification. All slaves and masters must comply with it.
> 
> did you now understand?
> 
> Really, please. I understand you're only trying to help and understand
> the patch, but at some point, if you have zero knowledge about the topic,
> you're not the right person to review the patch, period. You are still
> welcome to test the patch if you can and want, that's a completely
> different task.
> 
> --
> Jean Delvare

RE: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Tang Yuantian-B29983]

Hi Jean Delvare,

We agree to apply this patch. Do you have any concerns?
Although Guenter said that he give up, I don't think he really meant it.
Also it is improper if I repost similar patch.
I tested this patch, it works.

Regards,
Yuantian

> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: 2011年11月17日 3:19
> To: sun york-R58495
> Cc: guenter.roeck@ericsson.com; Tabi Timur-B04825; linux-
> i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Tang Yuantian-B29983
> Subject: Re: [PATCH] i2c/busses: (mpc) Add support for
> SMBUS_READ_BLOCK_DATA
> Really, please. I understand you're only trying to help and understand
> the patch, but at some point, if you have zero knowledge about the topic,
> you're not the right person to review the patch, period. You are still
> welcome to test the patch if you can and want, that's a completely
> different task.
> 
> --
> Jean Delvare

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Guenter Roeck]

On Tue, Dec 06, 2011 at 09:52:36PM -0500, Tang Yuantian-B29983 wrote:
> Hi Jean Delvare,
> 
> We agree to apply this patch. Do you have any concerns?
> Although Guenter said that he give up, I don't think he really meant it.

I did mean it, otherwise I would not have said it.

Guenter

> Also it is improper if I repost similar patch.
> I tested this patch, it works.
> 
> Regards,
> Yuantian
> 
> > -----Original Message-----
> > From: Jean Delvare [mailto:khali@linux-fr.org]
> > Sent: 2011年11月17日 3:19
> > To: sun york-R58495
> > Cc: guenter.roeck@ericsson.com; Tabi Timur-B04825; linux-
> > i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Tang Yuantian-B29983
> > Subject: Re: [PATCH] i2c/busses: (mpc) Add support for
> > SMBUS_READ_BLOCK_DATA
> > Really, please. I understand you're only trying to help and understand
> > the patch, but at some point, if you have zero knowledge about the topic,
> > you're not the right person to review the patch, period. You are still
> > welcome to test the patch if you can and want, that's a completely
> > different task.
> > 
> > --
> > Jean Delvare
> 
> 

RE: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Tang Yuantian-B29983]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1835 bytes --]

Hi Guenter,

I thought you were bore of discussion.
Do you mind if I post similar patch?

Regards,
Yuantian

> -----Original Message-----
> From: Guenter Roeck [mailto:guenter.roeck@ericsson.com]
> Sent: 2011年12月7日 11:20
> To: Tang Yuantian-B29983
> Cc: Jean Delvare; Tabi Timur-B04825; linux-i2c@vger.kernel.org; linux-
> kernel@vger.kernel.org; sun york-R58495; Durai Kumar-R34055; Huang
> Changming-R66093
> Subject: Re: [PATCH] i2c/busses: (mpc) Add support for
> SMBUS_READ_BLOCK_DATA
> 
> On Tue, Dec 06, 2011 at 09:52:36PM -0500, Tang Yuantian-B29983 wrote:
> > Hi Jean Delvare,
> >
> > We agree to apply this patch. Do you have any concerns?
> > Although Guenter said that he give up, I don't think he really meant it.
> 
> I did mean it, otherwise I would not have said it.
> 
> Guenter
> 
> > Also it is improper if I repost similar patch.
> > I tested this patch, it works.
> >
> > Regards,
> > Yuantian
> >
> > > -----Original Message-----
> > > From: Jean Delvare [mailto:khali@linux-fr.org]
> > > Sent: 2011年11月17日 3:19
> > > To: sun york-R58495
> > > Cc: guenter.roeck@ericsson.com; Tabi Timur-B04825; linux-
> > > i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Tang
> > > Yuantian-B29983
> > > Subject: Re: [PATCH] i2c/busses: (mpc) Add support for
> > > SMBUS_READ_BLOCK_DATA Really, please. I understand you're only
> > > trying to help and understand the patch, but at some point, if you
> > > have zero knowledge about the topic, you're not the right person to
> > > review the patch, period. You are still welcome to test the patch if
> > > you can and want, that's a completely different task.
> > >
> > > --
> > > Jean Delvare
> >
> >

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Jean Delvare]

Hi Yuantian,

Please don't top-post.

On Wed, 7 Dec 2011 05:25:49 +0000, Tang Yuantian-B29983 wrote:
> Hi Guenter,
> 
> I thought you were bore of discussion.
> Do you mind if I post similar patch?

I have no objection, as long as you give proper credit to the original
author.

Note that ultimately you'll have to get Ben Dooks to apply the patch. I
helped with the review but the i2c-mpc driver doesn't actually fall in
my jurisdiction.

-- 
Jean Delvare

RE: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Huang Changming-R66093]

We have sent the v2 since 12/12/2011, could you have any comment about it?
http://marc.info/?l=linux-i2c&m=132368362506671&w=2


Thanks
Jerry Huang


> -----Original Message-----
> From: Jean Delvare [mailto:khali@linux-fr.org]
> Sent: Wednesday, December 07, 2011 3:29 PM
> To: Tang Yuantian-B29983
> Cc: Guenter Roeck; Tabi Timur-B04825; linux-i2c@vger.kernel.org; linux-
> kernel@vger.kernel.org; sun york-R58495; Durai Kumar-R34055; Huang
> Changming-R66093
> Subject: Re: [PATCH] i2c/busses: (mpc) Add support for
> SMBUS_READ_BLOCK_DATA
> 
> Hi Yuantian,
> 
> Please don't top-post.
> 
> On Wed, 7 Dec 2011 05:25:49 +0000, Tang Yuantian-B29983 wrote:
> > Hi Guenter,
> >
> > I thought you were bore of discussion.
> > Do you mind if I post similar patch?
> 
> I have no objection, as long as you give proper credit to the original
> author.
> 
> Note that ultimately you'll have to get Ben Dooks to apply the patch. I
> helped with the review but the i2c-mpc driver doesn't actually fall in my
> jurisdiction.
> 
> --
> Jean Delvare

Re: [PATCH] i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA

[Jean Delvare]

On Wed, 16 Nov 2011 09:28:29 -0800, York Sun wrote:
> I am not sure if this mail will reach the list as I am not subscribed.
> 
> In the v2 patch (actually both version), you wrote
> 
> > +		byte = readb(i2c->base + MPC_I2C_DR);
> > +
> > +		/* Adjust length if first received byte is length */
> > +		if (i == 0 && recv_len) {
> > +			if (byte == 0 || byte > I2C_SMBUS_BLOCK_MAX)
> > +				return -EPROTO;
> > +			length += byte;
> > +			/*
> > +			 * For block reads, generate txack here if data length
> > +			 * is 1 byte (total length is 2 bytes).
> > +			 */
> > +			if (length == 2)
> > +				writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA
> > +					 | CCR_TXAK);
> > +		}
> > +		data[i] = byte;
> 
> I am wondering if the first byte should be left out for data[] if the
> recv_len is non-zero. If I understand correctly, this patch is to
> support of SMBus with multiple byte read. The SMBus is different from
> I2C bus on multiple byte. The first data is byte count vs slave data for
> I2C. So you will receive all data preceded by the byte count, which is
> one more than your loop.

That's why he wrote

	length += byte;

and not

	length = byte;

length is initialized to 1 for SMBus block reads.

-- 
Jean Delvare