* [PATCH] mfd: twl4030: clarify the return value for read and write @ 2009-12-02 13:31 Amit Kucheria 2009-12-02 13:41 ` Felipe Balbi 2009-12-02 14:00 ` [PATCHv2] " Amit Kucheria 0 siblings, 2 replies; 9+ messages in thread From: Amit Kucheria @ 2009-12-02 13:31 UTC (permalink / raw) To: List Linux Kernel; +Cc: Samuel Ortiz, List Linux Omap We should be checking if all the messages were tranferred or not. Currently we return success even if none of messages were transferred successfully. Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> --- drivers/mfd/twl4030-core.c | 21 +++++++++++++-------- 1 files changed, 13 insertions(+), 8 deletions(-) diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c index cf462b9..6ace378 100644 --- a/drivers/mfd/twl4030-core.c +++ b/drivers/mfd/twl4030-core.c @@ -298,10 +298,12 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); mutex_unlock(&twl->xfer_lock); - /* i2cTransfer returns num messages.translate it pls.. */ - if (ret >= 0) - ret = 0; - return ret; + /* i2c_transfer returns number of messages transferred */ + if (ret != 1) { + pr_err("%s: twl4030_i2c_write failed to transfer all messages\n", DRIVER_NAME); + return ret; + } else + return 0; } EXPORT_SYMBOL(twl4030_i2c_write); @@ -350,10 +352,13 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); mutex_unlock(&twl->xfer_lock); - /* i2cTransfer returns num messages.translate it pls.. */ - if (ret >= 0) - ret = 0; - return ret; + /* i2c_transfer returns number of messages transferred */ + if (ret != 2) { + pr_err("%s: twl4030_i2c_read failed to transfer all messages\n", DRIVER_NAME); + return ret; + } + else + return 0; } EXPORT_SYMBOL(twl4030_i2c_read); -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] mfd: twl4030: clarify the return value for read and write 2009-12-02 13:31 [PATCH] mfd: twl4030: clarify the return value for read and write Amit Kucheria @ 2009-12-02 13:41 ` Felipe Balbi 2009-12-02 14:00 ` [PATCHv2] " Amit Kucheria 1 sibling, 0 replies; 9+ messages in thread From: Felipe Balbi @ 2009-12-02 13:41 UTC (permalink / raw) To: ext Amit Kucheria; +Cc: List Linux Kernel, Samuel Ortiz, List Linux Omap Hi, On Wed, Dec 02, 2009 at 02:31:18PM +0100, ext Amit Kucheria wrote: >@@ -298,10 +298,12 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); > mutex_unlock(&twl->xfer_lock); > >- /* i2cTransfer returns num messages.translate it pls.. */ >- if (ret >= 0) >- ret = 0; >- return ret; >+ /* i2c_transfer returns number of messages transferred */ >+ if (ret != 1) { >+ pr_err("%s: twl4030_i2c_write failed to transfer all messages\n", DRIVER_NAME); this line is over 80-chars >+ return ret; >+ } else you should have {} here as well. >+ return 0; > } > EXPORT_SYMBOL(twl4030_i2c_write); > >@@ -350,10 +352,13 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); > mutex_unlock(&twl->xfer_lock); > >- /* i2cTransfer returns num messages.translate it pls.. */ >- if (ret >= 0) >- ret = 0; >- return ret; >+ /* i2c_transfer returns number of messages transferred */ >+ if (ret != 2) { >+ pr_err("%s: twl4030_i2c_read failed to transfer all messages\n", DRIVER_NAME); over 80-chars >+ return ret; >+ } >+ else } else { >+ return 0; } -- balbi ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCHv2] mfd: twl4030: clarify the return value for read and write 2009-12-02 13:31 [PATCH] mfd: twl4030: clarify the return value for read and write Amit Kucheria 2009-12-02 13:41 ` Felipe Balbi @ 2009-12-02 14:00 ` Amit Kucheria 2009-12-02 15:05 ` Gadiyar, Anand 1 sibling, 1 reply; 9+ messages in thread From: Amit Kucheria @ 2009-12-02 14:00 UTC (permalink / raw) To: List Linux Kernel; +Cc: Samuel Ortiz, List Linux Omap Infact, we can just return -1 so that caller knows for sure that all messages were not tranferred. Please consider fixed patch instead. We should be checking if all the messages were tranferred or not. And return -1 for failure. Currently we return success (0) even if none of messages were transferred successfully. Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> --- drivers/mfd/twl4030-core.c | 25 +++++++++++++++++-------- 1 files changed, 17 insertions(+), 8 deletions(-) diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c index 56f1de5..8f9ba79 100644 --- a/drivers/mfd/twl4030-core.c +++ b/drivers/mfd/twl4030-core.c @@ -292,10 +292,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); mutex_unlock(&twl->xfer_lock); - /* i2cTransfer returns num messages.translate it pls.. */ - if (ret >= 0) - ret = 0; - return ret; + /* i2c_transfer returns number of messages transferred */ + if (ret != 1) { + pr_err("%s: i2c_write failed to transfer all messages\n", + DRIVER_NAME); + return -1; + } else { + return 0; + } } EXPORT_SYMBOL(twl4030_i2c_write); @@ -344,10 +348,15 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); mutex_unlock(&twl->xfer_lock); - /* i2cTransfer returns num messages.translate it pls.. */ - if (ret >= 0) - ret = 0; - return ret; + /* i2c_transfer returns number of messages transferred */ + if (ret != 2) { + pr_err("%s: i2c_read failed to transfer all messages\n", + DRIVER_NAME); + return -1; + } + else { + return 0; + } } EXPORT_SYMBOL(twl4030_i2c_read); -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCHv2] mfd: twl4030: clarify the return value for read and write 2009-12-02 14:00 ` [PATCHv2] " Amit Kucheria @ 2009-12-02 15:05 ` Gadiyar, Anand 2009-12-07 12:17 ` [PATCH 1/1] " Amit Kucheria 0 siblings, 1 reply; 9+ messages in thread From: Gadiyar, Anand @ 2009-12-02 15:05 UTC (permalink / raw) To: Amit Kucheria, List Linux Kernel; +Cc: Samuel Ortiz, List Linux Omap Amit Kucheria wrote: > Infact, we can just return -1 so that caller knows for sure that all messages > were not tranferred. Please consider fixed patch instead. > > We should be checking if all the messages were tranferred or not. And return > -1 for failure. Currently we return success (0) even if none of messages were > transferred successfully. > > Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> > --- > drivers/mfd/twl4030-core.c | 25 +++++++++++++++++-------- > 1 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c > index 56f1de5..8f9ba79 100644 > --- a/drivers/mfd/twl4030-core.c > +++ b/drivers/mfd/twl4030-core.c > @@ -292,10 +292,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); > mutex_unlock(&twl->xfer_lock); > > - /* i2cTransfer returns num messages.translate it pls.. */ > - if (ret >= 0) > - ret = 0; > - return ret; > + /* i2c_transfer returns number of messages transferred */ > + if (ret != 1) { > + pr_err("%s: i2c_write failed to transfer all messages\n", > + DRIVER_NAME); > + return -1; > + } else { > + return 0; > + } > } > EXPORT_SYMBOL(twl4030_i2c_write); > > @@ -344,10 +348,15 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); > mutex_unlock(&twl->xfer_lock); > > - /* i2cTransfer returns num messages.translate it pls.. */ > - if (ret >= 0) > - ret = 0; > - return ret; > + /* i2c_transfer returns number of messages transferred */ > + if (ret != 2) { > + pr_err("%s: i2c_read failed to transfer all messages\n", > + DRIVER_NAME); > + return -1; > + } > + else { > + return 0; > + } The else clause should be on the same line as the brace closing the if clause. like you did above So says Documentation/CodingStyle. - Anand ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/1] mfd: twl4030: clarify the return value for read and write 2009-12-02 15:05 ` Gadiyar, Anand @ 2009-12-07 12:17 ` Amit Kucheria 2009-12-07 13:53 ` Eduardo Valentin 0 siblings, 1 reply; 9+ messages in thread From: Amit Kucheria @ 2009-12-07 12:17 UTC (permalink / raw) To: Samuel Ortiz; +Cc: gadiyar, List Linux Kernel, List Linux Omap Infact, we can just return -EIO so that caller knows for sure that all messages were not tranferred. Please consider fixed patch instead. We should be checking if all the messages were tranferred or not. And return -1 for failure. Currently we return success (0) even if none of messages were transferred successfully. Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> --- drivers/mfd/twl4030-core.c | 24 ++++++++++++++++-------- 1 files changed, 16 insertions(+), 8 deletions(-) diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c index 56f1de5..3d2c413 100644 --- a/drivers/mfd/twl4030-core.c +++ b/drivers/mfd/twl4030-core.c @@ -292,10 +292,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); mutex_unlock(&twl->xfer_lock); - /* i2cTransfer returns num messages.translate it pls.. */ - if (ret >= 0) - ret = 0; - return ret; + /* i2c_transfer returns number of messages transferred */ + if (ret != 1) { + pr_err("%s: i2c_write failed to transfer all messages\n", + DRIVER_NAME); + return -EIO; + } else { + return 0; + } } EXPORT_SYMBOL(twl4030_i2c_write); @@ -344,10 +348,14 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); mutex_unlock(&twl->xfer_lock); - /* i2cTransfer returns num messages.translate it pls.. */ - if (ret >= 0) - ret = 0; - return ret; + /* i2c_transfer returns number of messages transferred */ + if (ret != 2) { + pr_err("%s: i2c_read failed to transfer all messages\n", + DRIVER_NAME); + return -EIO; + } else { + return 0; + } } EXPORT_SYMBOL(twl4030_i2c_read); -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write 2009-12-07 12:17 ` [PATCH 1/1] " Amit Kucheria @ 2009-12-07 13:53 ` Eduardo Valentin 2009-12-11 10:36 ` Samuel Ortiz 0 siblings, 1 reply; 9+ messages in thread From: Eduardo Valentin @ 2009-12-07 13:53 UTC (permalink / raw) To: ext Amit Kucheria Cc: Samuel Ortiz, gadiyar@ti.com, List Linux Kernel, List Linux Omap On Mon, Dec 07, 2009 at 01:17:29PM +0100, ext Amit Kucheria wrote: > Infact, we can just return -EIO so that caller knows for sure that all > messages were not tranferred. Please consider fixed patch instead. > > We should be checking if all the messages were tranferred or not. And return > -1 for failure. Currently we return success (0) even if none of messages were > transferred successfully. > > Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> > --- > drivers/mfd/twl4030-core.c | 24 ++++++++++++++++-------- > 1 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c > index 56f1de5..3d2c413 100644 > --- a/drivers/mfd/twl4030-core.c > +++ b/drivers/mfd/twl4030-core.c > @@ -292,10 +292,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); > mutex_unlock(&twl->xfer_lock); > > - /* i2cTransfer returns num messages.translate it pls.. */ > - if (ret >= 0) > - ret = 0; > - return ret; > + /* i2c_transfer returns number of messages transferred */ > + if (ret != 1) { > + pr_err("%s: i2c_write failed to transfer all messages\n", > + DRIVER_NAME); > + return -EIO; How about reporting the actual error that has occurred and reported by i2c_transfer? Instead of just masking it as EIO? If i2c_transfer returns something > 0 then EIO should be right. But if returns and error code, then that error code must be reported to upper layers. > + } else { > + return 0; > + } > } > EXPORT_SYMBOL(twl4030_i2c_write); > > @@ -344,10 +348,14 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); > mutex_unlock(&twl->xfer_lock); > > - /* i2cTransfer returns num messages.translate it pls.. */ > - if (ret >= 0) > - ret = 0; > - return ret; > + /* i2c_transfer returns number of messages transferred */ > + if (ret != 2) { > + pr_err("%s: i2c_read failed to transfer all messages\n", > + DRIVER_NAME); > + return -EIO; dito. > + } else { > + return 0; > + } > } > EXPORT_SYMBOL(twl4030_i2c_read); > > -- > 1.6.3.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Eduardo Valentin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write 2009-12-07 13:53 ` Eduardo Valentin @ 2009-12-11 10:36 ` Samuel Ortiz 2009-12-11 10:53 ` Eduardo Valentin 0 siblings, 1 reply; 9+ messages in thread From: Samuel Ortiz @ 2009-12-11 10:36 UTC (permalink / raw) To: Eduardo Valentin, Amit Kucheria Cc: gadiyar@ti.com, List Linux Kernel, List Linux Omap Hi Amit, On Mon, Dec 07, 2009 at 03:53:09PM +0200, Eduardo Valentin wrote: > On Mon, Dec 07, 2009 at 01:17:29PM +0100, ext Amit Kucheria wrote: > > Infact, we can just return -EIO so that caller knows for sure that all > > messages were not tranferred. Please consider fixed patch instead. > > > > We should be checking if all the messages were tranferred or not. And return > > -1 for failure. Currently we return success (0) even if none of messages were > > transferred successfully. > > > > Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> > > --- > > drivers/mfd/twl4030-core.c | 24 ++++++++++++++++-------- > > 1 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c > > index 56f1de5..3d2c413 100644 > > --- a/drivers/mfd/twl4030-core.c > > +++ b/drivers/mfd/twl4030-core.c > > @@ -292,10 +292,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); > > mutex_unlock(&twl->xfer_lock); > > > > - /* i2cTransfer returns num messages.translate it pls.. */ > > - if (ret >= 0) > > - ret = 0; > > - return ret; > > + /* i2c_transfer returns number of messages transferred */ > > + if (ret != 1) { > > + pr_err("%s: i2c_write failed to transfer all messages\n", > > + DRIVER_NAME); > > + return -EIO; > > How about reporting the actual error that has occurred and reported by i2c_transfer? > Instead of just masking it as EIO? If i2c_transfer returns something > 0 then EIO should be right. > But if returns and error code, then that error code must be reported to upper layers. > So, I applied a modified version of this patch, see below. If you disagree with it, please let me know and I wont push it upstream. commit 8aa7cfd5732dee80aaaf3caa0a1d2f9621126315 Author: Amit Kucheria <amit.kucheria@verdurent.com> Date: Fri Dec 11 11:31:11 2009 +0100 mfd: Clarify twl4030 return value for read and write We should be checking if all the messages were tranferred. If not, then we should propagate the i2c core error code. Currently we return success (0) even if none of messages were transferred successfully. Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com> diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c index e4a5d48..8add16c 100644 --- a/drivers/mfd/twl4030-core.c +++ b/drivers/mfd/twl4030-core.c @@ -306,10 +306,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); mutex_unlock(&twl->xfer_lock); - /* i2cTransfer returns num messages.translate it pls.. */ - if (ret >= 0) - ret = 0; - return ret; + /* i2c_transfer returns number of messages transferred */ + if (ret != 1) { + pr_err("%s: i2c_write failed to transfer all messages\n", + DRIVER_NAME); + return ret; + } else { + return 0; + } } EXPORT_SYMBOL(twl4030_i2c_write); @@ -358,10 +362,14 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); mutex_unlock(&twl->xfer_lock); - /* i2cTransfer returns num messages.translate it pls.. */ - if (ret >= 0) - ret = 0; - return ret; + /* i2c_transfer returns number of messages transferred */ + if (ret != 2) { + pr_err("%s: i2c_read failed to transfer all messages\n", + DRIVER_NAME); + return ret; + } else { + return 0; + } } EXPORT_SYMBOL(twl4030_i2c_read); -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write 2009-12-11 10:36 ` Samuel Ortiz @ 2009-12-11 10:53 ` Eduardo Valentin 2009-12-11 12:11 ` Samuel Ortiz 0 siblings, 1 reply; 9+ messages in thread From: Eduardo Valentin @ 2009-12-11 10:53 UTC (permalink / raw) To: ext Samuel Ortiz Cc: Valentin Eduardo (Nokia-D/Helsinki), Amit Kucheria, gadiyar@ti.com, List Linux Kernel, List Linux Omap On Fri, Dec 11, 2009 at 11:36:10AM +0100, ext Samuel Ortiz wrote: > Hi Amit, > > On Mon, Dec 07, 2009 at 03:53:09PM +0200, Eduardo Valentin wrote: > > On Mon, Dec 07, 2009 at 01:17:29PM +0100, ext Amit Kucheria wrote: > > > Infact, we can just return -EIO so that caller knows for sure that all > > > messages were not tranferred. Please consider fixed patch instead. > > > > > > We should be checking if all the messages were tranferred or not. And return > > > -1 for failure. Currently we return success (0) even if none of messages were > > > transferred successfully. > > > > > > Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> > > > --- > > > drivers/mfd/twl4030-core.c | 24 ++++++++++++++++-------- > > > 1 files changed, 16 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c > > > index 56f1de5..3d2c413 100644 > > > --- a/drivers/mfd/twl4030-core.c > > > +++ b/drivers/mfd/twl4030-core.c > > > @@ -292,10 +292,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > > > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); > > > mutex_unlock(&twl->xfer_lock); > > > > > > - /* i2cTransfer returns num messages.translate it pls.. */ > > > - if (ret >= 0) > > > - ret = 0; > > > - return ret; > > > + /* i2c_transfer returns number of messages transferred */ > > > + if (ret != 1) { > > > + pr_err("%s: i2c_write failed to transfer all messages\n", > > > + DRIVER_NAME); > > > + return -EIO; > > > > How about reporting the actual error that has occurred and reported by i2c_transfer? > > Instead of just masking it as EIO? If i2c_transfer returns something > 0 then EIO should be right. > > But if returns and error code, then that error code must be reported to upper layers. > > > So, I applied a modified version of this patch, see below. If you disagree > with it, please let me know and I wont push it upstream. > OK. > > commit 8aa7cfd5732dee80aaaf3caa0a1d2f9621126315 > Author: Amit Kucheria <amit.kucheria@verdurent.com> > Date: Fri Dec 11 11:31:11 2009 +0100 > > mfd: Clarify twl4030 return value for read and write > > We should be checking if all the messages were tranferred. If not, then we > should propagate the i2c core error code. > Currently we return success (0) even if none of messages were transferred > successfully. > > Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com> > > diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c > index e4a5d48..8add16c 100644 > --- a/drivers/mfd/twl4030-core.c > +++ b/drivers/mfd/twl4030-core.c > @@ -306,10 +306,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); > mutex_unlock(&twl->xfer_lock); > > - /* i2cTransfer returns num messages.translate it pls.. */ > - if (ret >= 0) > - ret = 0; > - return ret; > + /* i2c_transfer returns number of messages transferred */ > + if (ret != 1) { > + pr_err("%s: i2c_write failed to transfer all messages\n", > + DRIVER_NAME); > + return ret; For this case we will be returning success if i2c_transfer fails to transfer the one message. I guess we should return ret only if ret < 0 ? otherwise -EIO. > + } else { > + return 0; > + } > } > EXPORT_SYMBOL(twl4030_i2c_write); > > @@ -358,10 +362,14 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); > mutex_unlock(&twl->xfer_lock); > > - /* i2cTransfer returns num messages.translate it pls.. */ > - if (ret >= 0) > - ret = 0; > - return ret; > + /* i2c_transfer returns number of messages transferred */ > + if (ret != 2) { > + pr_err("%s: i2c_read failed to transfer all messages\n", > + DRIVER_NAME); > + return ret; Same case here, we will be returning success if i2c_transfer fails to transfer any of those messages. I guess we should return ret only if ret < 0 ? otherwise -EIO. > + } else { > + return 0; > + } > } > EXPORT_SYMBOL(twl4030_i2c_read); > > -- > Intel Open Source Technology Centre > http://oss.intel.com/ -- Eduardo Valentin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write 2009-12-11 10:53 ` Eduardo Valentin @ 2009-12-11 12:11 ` Samuel Ortiz 0 siblings, 0 replies; 9+ messages in thread From: Samuel Ortiz @ 2009-12-11 12:11 UTC (permalink / raw) To: Eduardo Valentin Cc: Amit Kucheria, gadiyar@ti.com, List Linux Kernel, List Linux Omap On Fri, Dec 11, 2009 at 12:53:45PM +0200, Eduardo Valentin wrote: > On Fri, Dec 11, 2009 at 11:36:10AM +0100, ext Samuel Ortiz wrote: > > Hi Amit, > > > > On Mon, Dec 07, 2009 at 03:53:09PM +0200, Eduardo Valentin wrote: > > > On Mon, Dec 07, 2009 at 01:17:29PM +0100, ext Amit Kucheria wrote: > > > > Infact, we can just return -EIO so that caller knows for sure that all > > > > messages were not tranferred. Please consider fixed patch instead. > > > > > > > > We should be checking if all the messages were tranferred or not. And return > > > > -1 for failure. Currently we return success (0) even if none of messages were > > > > transferred successfully. > > > > > > > > Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> > > > > --- > > > > drivers/mfd/twl4030-core.c | 24 ++++++++++++++++-------- > > > > 1 files changed, 16 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c > > > > index 56f1de5..3d2c413 100644 > > > > --- a/drivers/mfd/twl4030-core.c > > > > +++ b/drivers/mfd/twl4030-core.c > > > > @@ -292,10 +292,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > > > > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); > > > > mutex_unlock(&twl->xfer_lock); > > > > > > > > - /* i2cTransfer returns num messages.translate it pls.. */ > > > > - if (ret >= 0) > > > > - ret = 0; > > > > - return ret; > > > > + /* i2c_transfer returns number of messages transferred */ > > > > + if (ret != 1) { > > > > + pr_err("%s: i2c_write failed to transfer all messages\n", > > > > + DRIVER_NAME); > > > > + return -EIO; > > > > > > How about reporting the actual error that has occurred and reported by i2c_transfer? > > > Instead of just masking it as EIO? If i2c_transfer returns something > 0 then EIO should be right. > > > But if returns and error code, then that error code must be reported to upper layers. > > > > > So, I applied a modified version of this patch, see below. If you disagree > > with it, please let me know and I wont push it upstream. > > > > OK. > > > > > commit 8aa7cfd5732dee80aaaf3caa0a1d2f9621126315 > > Author: Amit Kucheria <amit.kucheria@verdurent.com> > > Date: Fri Dec 11 11:31:11 2009 +0100 > > > > mfd: Clarify twl4030 return value for read and write > > > > We should be checking if all the messages were tranferred. If not, then we > > should propagate the i2c core error code. > > Currently we return success (0) even if none of messages were transferred > > successfully. > > > > Signed-off-by: Amit Kucheria <amit.kucheria@verdurent.com> > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com> > > > > diff --git a/drivers/mfd/twl4030-core.c b/drivers/mfd/twl4030-core.c > > index e4a5d48..8add16c 100644 > > --- a/drivers/mfd/twl4030-core.c > > +++ b/drivers/mfd/twl4030-core.c > > @@ -306,10 +306,14 @@ int twl4030_i2c_write(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 1); > > mutex_unlock(&twl->xfer_lock); > > > > - /* i2cTransfer returns num messages.translate it pls.. */ > > - if (ret >= 0) > > - ret = 0; > > - return ret; > > + /* i2c_transfer returns number of messages transferred */ > > + if (ret != 1) { > > + pr_err("%s: i2c_write failed to transfer all messages\n", > > + DRIVER_NAME); > > + return ret; > > For this case we will be returning success if i2c_transfer fails to transfer the one message. > I guess we should return ret only if ret < 0 ? otherwise -EIO. Yes, that's correct, fixed now. Cheers, Samuel. > > + } else { > > + return 0; > > + } > > } > > EXPORT_SYMBOL(twl4030_i2c_write); > > > > @@ -358,10 +362,14 @@ int twl4030_i2c_read(u8 mod_no, u8 *value, u8 reg, unsigned num_bytes) > > ret = i2c_transfer(twl->client->adapter, twl->xfer_msg, 2); > > mutex_unlock(&twl->xfer_lock); > > > > - /* i2cTransfer returns num messages.translate it pls.. */ > > - if (ret >= 0) > > - ret = 0; > > - return ret; > > + /* i2c_transfer returns number of messages transferred */ > > + if (ret != 2) { > > + pr_err("%s: i2c_read failed to transfer all messages\n", > > + DRIVER_NAME); > > + return ret; > > Same case here, we will be returning success if i2c_transfer fails to transfer any of those messages. > I guess we should return ret only if ret < 0 ? otherwise -EIO. > > > > + } else { > > + return 0; > > + } > > } > > EXPORT_SYMBOL(twl4030_i2c_read); > > > > -- > > Intel Open Source Technology Centre > > http://oss.intel.com/ > > -- > Eduardo Valentin -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-12-11 12:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-12-02 13:31 [PATCH] mfd: twl4030: clarify the return value for read and write Amit Kucheria 2009-12-02 13:41 ` Felipe Balbi 2009-12-02 14:00 ` [PATCHv2] " Amit Kucheria 2009-12-02 15:05 ` Gadiyar, Anand 2009-12-07 12:17 ` [PATCH 1/1] " Amit Kucheria 2009-12-07 13:53 ` Eduardo Valentin 2009-12-11 10:36 ` Samuel Ortiz 2009-12-11 10:53 ` Eduardo Valentin 2009-12-11 12:11 ` Samuel Ortiz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox