| Message ID | 1340967927-27354-4-git-send-email-shubhrajyoti@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 29 Jun 2012 16:35:27 +0530, Shubhrajyoti D wrote: > In i2c_smbus_xfer_emulated initialise the actual bytes > to zero. > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > 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?
On Fri, 29 Jun 2012 14:40:02 +0200, Jean Delvare wrote: > On Fri, 29 Jun 2012 16:35:27 +0530, Shubhrajyoti D wrote: > > In i2c_smbus_xfer_emulated initialise the actual bytes > > to zero. > > > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > > --- > > 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.
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? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 29 Jun 2012 18:42:08 +0530, Shubhrajyoti wrote: > 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. 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 ;)
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: > > 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. > > 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 ;) 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 ?
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: > > 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 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. 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.
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: > > On Fri, Jun 29, 2012 at 03:18:32PM +0200, Jean Delvare wrote: > > > 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. > > 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 > > 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.
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;
In i2c_smbus_xfer_emulated initialise the actual bytes to zero. Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> --- drivers/i2c/i2c-core.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)