[RFC PATCH 0/3] I2C: Report the actual transferred bytes

Currently the I2C core reports only a negative number on errors.
However some of the users in case of errors want to re-transfer only
the remaining bytes.

The patch series tries to add a actual field to the msg which
can be checked by the user.

Also a patch to implement the actual bytes transferred in case of
i2c-omap.


Felipe Balbi (1):
  i2c: omap: implement handling for 'actual' bytes transferred

Shubhrajyoti D (2):
  i2c: add 'actual' field to struct i2c_msg
  i2c: inititalise the actual transferred to zero

 drivers/i2c/busses/i2c-omap.c |    5 +++++
 drivers/i2c/i2c-core.c        |    4 ++--
 include/linux/i2c.h           |    1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

-- 
1.7.5.4

In case of a NACK, it's wise to tell our clients
drivers about how many bytes were actually transferred.

Support this by adding an extra field to the struct
i2c_msg which gets incremented the amount of bytes
actually transferred.

Signed-off-by: Shubhrajyoti D <redacted>
Signed-off-by: Felipe Balbi <redacted>
---
 include/linux/i2c.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index ddfa041..0cb6b83 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h

@@ -547,6 +547,7 @@ struct i2c_msg {
 #define I2C_M_NO_RD_ACK		0x0800	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_RECV_LEN		0x0400	/* length will be first received byte */
 	__u16 len;		/* msg length				*/
+	__u16 actual;		/* actual bytes transferred             */
 	__u8 *buf;		/* pointer to msg data			*/
 };
 

-- 
1.7.5.4

From: Felipe Balbi <redacted>

this is important in cases where client driver
wants to know how many bytes were actually
transferred.

Signed-off-by: Felipe Balbi <redacted>
Signed-off-by: Shubhrajyoti D <redacted>
---
 drivers/i2c/busses/i2c-omap.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 266bba7..fc726a6 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c

@@ -189,6 +189,7 @@ struct omap_i2c_dev {
 	u8			*buf;
 	u8			*regs;
 	size_t			buf_len;
+	u16			actual;
 	struct i2c_adapter	adapter;
 	u8			threshold;
 	u8			fifo_size;	/* use as flag and value

@@ -601,6 +602,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		omap_i2c_init(dev);
 		return -ETIMEDOUT;
 	}
+	msg->actual = dev->actual;
 
 	if (likely(!dev->cmd_err))
 		return 0;

@@ -828,6 +830,7 @@ static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes,
 		w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
 		*dev->buf++ = w;
 		dev->buf_len--;
+		dev->actual++;
 
 		/*
 		 * Data reg in 2430, omap3 and

@@ -848,6 +851,7 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes,
 	while (num_bytes--) {
 		w = *dev->buf++;
 		dev->buf_len--;
+		dev->actual++;
 
 		/*
 		 * Data reg in 2430, omap3 and

@@ -908,6 +912,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 
 		if (stat & OMAP_I2C_STAT_NACK) {
 			err |= OMAP_I2C_STAT_NACK;
+			dev->actual--;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
 			goto out;
 		}

-- 
1.7.5.4

In i2c_smbus_xfer_emulated initialise the actual bytes
to zero.

Signed-off-by: Shubhrajyoti D <redacted>
---
 drivers/i2c/i2c-core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a6ad32b..fa7f799 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c

@@ -1949,8 +1949,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
 	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
 	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
-	struct i2c_msg msg[2] = { { addr, flags, 1, msgbuf0 },
-	                          { addr, flags | I2C_M_RD, 0, msgbuf1 }
+	struct i2c_msg msg[2] = { { addr, flags, 1, 0, msgbuf0 },
+				  { addr, flags | I2C_M_RD, 0, 0, msgbuf1 }
 	                        };
 	int i;
 	u8 partial_pec = 0;

-- 
1.7.5.4

On Fri, 29 Jun 2012 16:35:25 +0530, Shubhrajyoti D wrote:

David Brownell discussed this a long time ago:
http://marc.info/?l=linux-i2c&m=121103523020494&w=2

This structure is exported to user-space through i2c-dev, so changing
it is tough if possible at all. You first have to ensure that the
change doesn't alter the structure size. In other words, the field you
add must fit in a padding hole the structure had previously. This
appears to be the case here, but I'm not sure how to prove it for all
supported architectures.

Then, as you are inserting a field in the middle of the structure, you
must ensure that every initialization follows C99. And you have to do
that _before_ the above change, not after as you did in patch 3/3. This
needs to be done in both the whole kernel tree and as many user-space
applications as possible. At least i2c-tools is clean, but there must
be other (possibly unpublished) tools using this API, which your change
may break. We can't fix them all, but at least we will have to document
the change clearly and explain how to fix in case of breakage. The i2c
wiki might be the right place for that.

-- 
Jean Delvare

On Fri, 29 Jun 2012 16:35:27 +0530, Shubhrajyoti D wrote:

Please convert to C99-style initialization while you're there. And this
should be done first. Initializing i2c_msg.actual maybe rather belong
to i2c_transfer though, so that all callers don't have to care.

Are you sure there are no other places that need the same fix in the
kernel tree?

-- 
Jean Delvare

On Fri, 29 Jun 2012 14:40:02 +0200, Jean Delvare wrote:

On Fri, 29 Jun 2012 16:35:27 +0530, Shubhrajyoti D wrote:

quoted

In i2c_smbus_xfer_emulated initialise the actual bytes
to zero.

Signed-off-by: Shubhrajyoti D <redacted>
---
 drivers/i2c/i2c-core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a6ad32b..fa7f799 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c

@@ -1949,8 +1949,8 @@ static s32 i2c_smbus_xfer_emulated(struct i2c_adapter *adapter, u16 addr,
 	unsigned char msgbuf0[I2C_SMBUS_BLOCK_MAX+3];
 	unsigned char msgbuf1[I2C_SMBUS_BLOCK_MAX+2];
 	int num = read_write == I2C_SMBUS_READ ? 2 : 1;
-	struct i2c_msg msg[2] = { { addr, flags, 1, msgbuf0 },
-	                          { addr, flags | I2C_M_RD, 0, msgbuf1 }
+	struct i2c_msg msg[2] = { { addr, flags, 1, 0, msgbuf0 },
+				  { addr, flags | I2C_M_RD, 0, 0, msgbuf1 }
 	                        };
 	int i;
 	u8 partial_pec = 0;

Please convert to C99-style initialization while you're there. And this
should be done first. Initializing i2c_msg.actual maybe rather belong
to i2c_transfer though, so that all callers don't have to care.

Are you sure there are no other places that need the same fix in the
kernel tree?

Actually I'm sure there are. At least:

drivers/video/matrox/matroxfb_maven.c: In function ?maven_get_reg?:
drivers/video/matrox/matroxfb_maven.c:140:9: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/video/matrox/matroxfb_maven.c:140:9: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/video/matrox/matroxfb_maven.c:141:6: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/video/matrox/matroxfb_maven.c:141:6: warning: (near initialization for ?msgs[1].actual?) [enabled by default]

drivers/media/video/ks0127.c: In function ?ks0127_read?:
drivers/media/video/ks0127.c:322:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/ks0127.c:322:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/media/video/ks0127.c:323:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/ks0127.c:323:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]

drivers/media/video/tvaudio.c: In function ?chip_read2?:
drivers/media/video/tvaudio.c:221:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/tvaudio.c:221:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/media/video/tvaudio.c:222:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/tvaudio.c:222:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]

drivers/rtc/rtc-x1205.c: In function ?x1205_get_datetime?:
drivers/rtc/rtc-x1205.c:100:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:100:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c:101:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:101:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c: In function ?x1205_get_status?:
drivers/rtc/rtc-x1205.c:145:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:145:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c:146:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:146:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c: In function ?x1205_get_dtrim?:
drivers/rtc/rtc-x1205.c:282:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:282:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c:283:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:283:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c: In function ?x1205_get_atrim?:
drivers/rtc/rtc-x1205.c:314:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:314:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c:315:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:315:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c: In function ?x1205_validate_client?:
drivers/rtc/rtc-x1205.c:384:4: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:384:4: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c:385:4: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:385:4: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c:412:4: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:412:4: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c:413:4: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:413:4: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c: In function ?x1205_rtc_read_alarm?:
drivers/rtc/rtc-x1205.c:447:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:447:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-x1205.c:448:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-x1205.c:448:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]

drivers/rtc/rtc-s35390a.c: In function ?s35390a_set_reg?:
drivers/rtc/rtc-s35390a.c:53:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-s35390a.c:53:3: warning: (near initialization for ?msg[0].actual?) [enabled by default]
drivers/rtc/rtc-s35390a.c: In function ?s35390a_get_reg?:
drivers/rtc/rtc-s35390a.c:66:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-s35390a.c:66:3: warning: (near initialization for ?msg[0].actual?) [enabled by default]

  CC [M]  drivers/media/video/msp3400-kthreads.o
drivers/media/video/msp3400-driver.c: In function ?msp_reset?:
drivers/media/video/msp3400-driver.c:122:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/msp3400-driver.c:122:3: warning: (near initialization for ?reset[0].actual?) [enabled by default]
drivers/media/video/msp3400-driver.c:123:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/msp3400-driver.c:123:3: warning: (near initialization for ?reset[1].actual?) [enabled by default]
drivers/media/video/msp3400-driver.c:126:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/msp3400-driver.c:126:3: warning: (near initialization for ?test[0].actual?) [enabled by default]
drivers/media/video/msp3400-driver.c:127:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/msp3400-driver.c:127:3: warning: (near initialization for ?test[1].actual?) [enabled by default]
drivers/media/video/msp3400-driver.c: In function ?msp_read?:
drivers/media/video/msp3400-driver.c:146:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/msp3400-driver.c:146:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/media/video/msp3400-driver.c:147:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/video/msp3400-driver.c:147:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]

drivers/rtc/rtc-rs5c372.c: In function ?rs5c_get_regs?:
drivers/rtc/rtc-rs5c372.c:107:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-rs5c372.c:107:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]

drivers/rtc/rtc-pcf8563.c: In function ?pcf8563_get_datetime?:
drivers/rtc/rtc-pcf8563.c:80:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-pcf8563.c:80:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-pcf8563.c:81:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-pcf8563.c:81:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]

drivers/rtc/rtc-isl1208.c: In function ?isl1208_i2c_read_regs?:
drivers/rtc/rtc-isl1208.c:71:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-isl1208.c:71:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-isl1208.c:73:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-isl1208.c:73:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-isl1208.c: In function ?isl1208_i2c_set_regs?:
drivers/rtc/rtc-isl1208.c:93:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-isl1208.c:93:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]

drivers/rtc/rtc-em3027.c: In function ?em3027_get_time?:
drivers/rtc/rtc-em3027.c:52:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-em3027.c:52:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-em3027.c:53:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-em3027.c:53:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-em3027.c: In function ?em3027_set_time?:
drivers/rtc/rtc-em3027.c:79:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-em3027.c:79:3: warning: (near initialization for ?msg.actual?) [enabled by default]

drivers/rtc/rtc-ds1672.c: In function ?ds1672_get_datetime?:
drivers/rtc/rtc-ds1672.c:40:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-ds1672.c:40:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-ds1672.c:41:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-ds1672.c:41:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]
drivers/rtc/rtc-ds1672.c: In function ?ds1672_get_control?:
drivers/rtc/rtc-ds1672.c:102:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-ds1672.c:102:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/rtc/rtc-ds1672.c:103:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/rtc/rtc-ds1672.c:103:3: warning: (near initialization for ?msgs[1].actual?) [enabled by default]

drivers/media/radio/saa7706h.c: In function ?saa7706h_get_reg16?:
drivers/media/radio/saa7706h.c:202:9: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/radio/saa7706h.c:202:9: warning: (near initialization for ?msg[0].actual?) [enabled by default]
drivers/media/radio/saa7706h.c:203:5: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/radio/saa7706h.c:203:5: warning: (near initialization for ?msg[1].actual?) [enabled by default]

drivers/media/radio/radio-tea5764.c: In function ?tea5764_i2c_read?:
drivers/media/radio/radio-tea5764.c:155:4: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/radio/radio-tea5764.c:155:4: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/media/radio/radio-tea5764.c: In function ?tea5764_i2c_write?:
drivers/media/radio/radio-tea5764.c:170:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/radio/radio-tea5764.c:170:3: warning: (near initialization for ?msgs[0].actual?) [enabled by default]

drivers/media/radio/si470x/radio-si470x-i2c.c: In function ?si470x_get_register?:
drivers/media/radio/si470x/radio-si470x-i2c.c:102:4: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/radio/si470x/radio-si470x-i2c.c:102:4: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/media/radio/si470x/radio-si470x-i2c.c: In function ?si470x_set_register?:
drivers/media/radio/si470x/radio-si470x-i2c.c:123:4: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/radio/si470x/radio-si470x-i2c.c:123:4: warning: (near initialization for ?msgs[0].actual?) [enabled by default]
drivers/media/radio/si470x/radio-si470x-i2c.c: In function ?si470x_get_all_registers?:
drivers/media/radio/si470x/radio-si470x-i2c.c:150:4: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/media/radio/si470x/radio-si470x-i2c.c:150:4: warning: (near initialization for ?msgs[0].actual?) [enabled by default]

drivers/gpu/drm/nouveau/nouveau_bios.c: In function ?init_i2c_long_if?:
drivers/gpu/drm/nouveau/nouveau_bios.c:3534:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/gpu/drm/nouveau/nouveau_bios.c:3534:3: warning: (near initialization for ?msg[0].actual?) [enabled by default]
drivers/gpu/drm/nouveau/nouveau_bios.c:3535:3: warning: initialization makes integer from pointer without a cast [enabled by default]
drivers/gpu/drm/nouveau/nouveau_bios.c:3535:3: warning: (near initialization for ?msg[1].actual?) [enabled by default]

This needs to be all fixed (converted to C99-style struct
initialization) before your patch is considered for inclusion. And
there may be more that my config did not spot.

-- 
Jean Delvare

On Friday 29 June 2012 06:27 PM, Jean Delvare wrote:

drivers/gpu/drm/nouveau/nouveau_bios.c:3535:3: warning: (near initialization for ?msg[1].actual?) [enabled by default]

This needs to be all fixed (converted to C99-style struct
initialization) before your patch is considered for inclusion. And
there may be more that my config did not spot.

Yes,agree.

The idea of this patch was if the idea is agreed upon then.
then fixing all the callers could be worked on.

However do you agree with this approach?

On Fri, 29 Jun 2012 18:42:08 +0530, Shubhrajyoti wrote:

On Friday 29 June 2012 06:27 PM, Jean Delvare wrote:

quoted

drivers/gpu/drm/nouveau/nouveau_bios.c:3535:3: warning: (near initialization for ?msg[1].actual?) [enabled by default]

This needs to be all fixed (converted to C99-style struct
initialization) before your patch is considered for inclusion. And
there may be more that my config did not spot.

Yes,agree.

The idea of this patch was if the idea is agreed upon then.
then fixing all the callers could be worked on.

This is worth fixing anyway, C99-style initialization is always good to
move to.

However do you agree with this approach?

Well you've seen the caveats I mentioned, this will be no easy ride. If
you are willing to take the risk, spend the time documenting the
change, and help people if there are issues, then I'm OK. At least as
long as someone else doesn't come saying it's a very bad idea ;)

-- 
Jean Delvare

Hi,

On Fri, Jun 29, 2012 at 03:18:32PM +0200, Jean Delvare wrote:

On Fri, 29 Jun 2012 18:42:08 +0530, Shubhrajyoti wrote:

quoted

On Friday 29 June 2012 06:27 PM, Jean Delvare wrote:

quoted

drivers/gpu/drm/nouveau/nouveau_bios.c:3535:3: warning: (near initialization for ?msg[1].actual?) [enabled by default]

This needs to be all fixed (converted to C99-style struct
initialization) before your patch is considered for inclusion. And
there may be more that my config did not spot.

Yes,agree.

The idea of this patch was if the idea is agreed upon then.
then fixing all the callers could be worked on.

This is worth fixing anyway, C99-style initialization is always good to
move to.

quoted

However do you agree with this approach?

Well you've seen the caveats I mentioned, this will be no easy ride. If
you are willing to take the risk, spend the time documenting the
change, and help people if there are issues, then I'm OK. At least as
long as someone else doesn't come saying it's a very bad idea ;)

This approach is a hard-requirement for devices which will pose any
interface/feedback with user. We have been working with a piezo driver
from TI (drv2665) and it will be used (in most cases at least) to give
the user a feedback based on e.g. touchscreen event.

Imagine drv2665 responds with a NAK while we're in the middle of driving
a wave through it (keep in mind a wave could take seconds to finish,
depending on the usecase), if we don't have a way to tell users that we
have writen X bytes, how should we make sure to continue driving the
wave from the exact byte where we got a NAK ?

If can't make sure that detail is true, then such usecases (as piezo
drivers) will never work.

Another approach would be to not add any field to struct i2c_msg but
instead return either the amount of bytes transferred, or an error case.
This would mean a series that would:

1) fix all users of struct i2c_master_send() and the like to check for
	errors as if (ret < 0) instead of if (ret);
2) fix all i2c buses to return the amount of bytes written instead of
	zero or error case
3) fix Documentation to state that we're now returning the amount of
	bytes, instead of zero or errno.

I'm not sure what will have minimum impact to userland, though. What do
you say Jean ? What would you prefer ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120702/29c54463/attachment.sig>

Hi Felipe,

On Mon, 2 Jul 2012 14:54:23 +0300, Felipe Balbi wrote:

On Fri, Jun 29, 2012 at 03:18:32PM +0200, Jean Delvare wrote:

quoted

Well you've seen the caveats I mentioned, this will be no easy ride. If
you are willing to take the risk, spend the time documenting the
change, and help people if there are issues, then I'm OK. At least as
long as someone else doesn't come saying it's a very bad idea ;)

This approach is a hard-requirement for devices which will pose any
interface/feedback with user. We have been working with a piezo driver
from TI (drv2665) and it will be used (in most cases at least) to give
the user a feedback based on e.g. touchscreen event.

Imagine drv2665 responds with a NAK while we're in the middle of driving
a wave through it (keep in mind a wave could take seconds to finish,
depending on the usecase), if we don't have a way to tell users that we
have writen X bytes, how should we make sure to continue driving the
wave from the exact byte where we got a NAK ?

If can't make sure that detail is true, then such usecases (as piezo
drivers) will never work.

Another approach would be to not add any field to struct i2c_msg but
instead return either the amount of bytes transferred, or an error case.
This would mean a series that would:

1) fix all users of struct i2c_master_send() and the like to check for
	errors as if (ret < 0) instead of if (ret);

You confuse me here. i2c_master_send is a function, not a structure.

Have you checked the code as it currently exists? i2c_master_send()
already returns a positive number on success, not 0. I'm not claiming
this number is necessarily very useful with the current implementation,
but it's not 0. The API looks sane at least, and with Shubhrajyoti's
proposed change, we could finally implement it properly.

And its backend i2c_transfer() also doesn't return 0 on success, but
the number of transferred messages. The problem@the moment is that
it's not clear what the bus driver should return in case of partial
success : a negative error code, or the number of messages successfully
processed. I suspect implementations are mixed. Plus, as you and
Shubhrajyoti found out, the caller doesn't know where in the last
message the problem occurred.

Are you really suggesting that we could change the meaning of the value
returned by i2c_transfer from "number of messages processed" to "number
of bytes processed"? This would be a real API change. I'm not claiming
the current API is very useful, but callers expect it that way, and I
mean in-tree kernel drivers, out-of-tree kernel drivers, and user-space
alike. Changing it has a huge risk of breakage (with lots of people mad
at you.)

2) fix all i2c buses to return the amount of bytes written instead of
	zero or error case

adap->algo->master_xfer is not returning 0 on success today, so your
proposal makes little sense to me.

3) fix Documentation to state that we're now returning the amount of
	bytes, instead of zero or errno.

I'm not sure what will have minimum impact to userland, though. What do
you say Jean ? What would you prefer ?

Shubhrajyoti's proposal (which is much in line with what David Brownell
proposed 4 years ago) seems at least possible to implement, while so
far your own proposal is fuzzy at best (an actual patch may make your
intents clearer.)

What I like about Shubhrajyoti's proposal is that it adds optional
information for the caller. It isn't changing the values returned, so
the risk of breaking current driver code is quite low. Actually I think
the only issue is with i2c_msg structures not being initialized using
the C99 style. Other than this, it should be pretty safe.

-- 
Jean Delvare

On Fri, 29 Jun 2012 16:35:25 +0530, Shubhrajyoti D wrote:

Oh, as a side note: I don't much like the name "actual". It's not
really clear what it means. I don't have a perfect name to propose as a
replacement, but maybe "transferred" or even just "done" would do.

-- 
Jean Delvare

Hi,

On Mon, Jul 02, 2012 at 03:20:58PM +0200, Jean Delvare wrote:

Hi Felipe,

On Mon, 2 Jul 2012 14:54:23 +0300, Felipe Balbi wrote:

quoted

On Fri, Jun 29, 2012 at 03:18:32PM +0200, Jean Delvare wrote:

quoted

Well you've seen the caveats I mentioned, this will be no easy ride. If
you are willing to take the risk, spend the time documenting the
change, and help people if there are issues, then I'm OK. At least as
long as someone else doesn't come saying it's a very bad idea ;)

This approach is a hard-requirement for devices which will pose any
interface/feedback with user. We have been working with a piezo driver
from TI (drv2665) and it will be used (in most cases at least) to give
the user a feedback based on e.g. touchscreen event.

Imagine drv2665 responds with a NAK while we're in the middle of driving
a wave through it (keep in mind a wave could take seconds to finish,
depending on the usecase), if we don't have a way to tell users that we
have writen X bytes, how should we make sure to continue driving the
wave from the exact byte where we got a NAK ?

If can't make sure that detail is true, then such usecases (as piezo
drivers) will never work.

Another approach would be to not add any field to struct i2c_msg but
instead return either the amount of bytes transferred, or an error case.
This would mean a series that would:

1) fix all users of struct i2c_master_send() and the like to check for
	errors as if (ret < 0) instead of if (ret);

You confuse me here. i2c_master_send is a function, not a structure.

exactly. The proposal was to change the semantics of i2c_master_send()
and maybe users understand that it returns negative errno or the number
of bytes transfered. No need to change any structure. Just read again
where I said:

"Another approach would be to not add any field to struct i2c_msg but
instead return either the amount of bytes transferred, or an error
case."

Have you checked the code as it currently exists? i2c_master_send()
already returns a positive number on success, not 0. I'm not claiming
this number is necessarily very useful with the current implementation,
but it's not 0. The API looks sane at least, and with Shubhrajyoti's
proposed change, we could finally implement it properly.

And its backend i2c_transfer() also doesn't return 0 on success, but
the number of transferred messages. The problem at the moment is that
it's not clear what the bus driver should return in case of partial
success : a negative error code, or the number of messages successfully
processed. I suspect implementations are mixed. Plus, as you and
Shubhrajyoti found out, the caller doesn't know where in the last
message the problem occurred.

indeed.

Are you really suggesting that we could change the meaning of the value
returned by i2c_transfer from "number of messages processed" to "number
of bytes processed"? This would be a real API change. I'm not claiming
the current API is very useful, but callers expect it that way, and I
mean in-tree kernel drivers, out-of-tree kernel drivers, and user-space
alike. Changing it has a huge risk of breakage (with lots of people mad
at you.)

So does adding an extra field to the i2c_msg structure, right ? We just
need to make sure to implement the one which will cause less problems.

quoted

2) fix all i2c buses to return the amount of bytes written instead of
	zero or error case

adap->algo->master_xfer is not returning 0 on success today, so your
proposal makes little sense to me.

s/zero/number of messages

quoted

3) fix Documentation to state that we're now returning the amount of
	bytes, instead of zero or errno.

I'm not sure what will have minimum impact to userland, though. What do
you say Jean ? What would you prefer ?

Shubhrajyoti's proposal (which is much in line with what David Brownell
proposed 4 years ago) seems at least possible to implement, while so
far your own proposal is fuzzy at best (an actual patch may make your
intents clearer.)

What I like about Shubhrajyoti's proposal is that it adds optional
information for the caller. It isn't changing the values returned, so
the risk of breaking current driver code is quite low. Actually I think
the only issue is with i2c_msg structures not being initialized using
the C99 style. Other than this, it should be pretty safe.

fair enough.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120716/bbdcd452/attachment.sig>