2009-12-02 13:32:32

by Amit Kucheria

Subject: [PATCH] mfd: twl4030: clarify the return value for read and write

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 <[email protected]>
---
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

2009-12-02 13:42:04

by Felipe Balbi

Subject: Re: [PATCH] mfd: twl4030: clarify the return value for read and write

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

2009-12-02 14:01:10

by Amit Kucheria

Subject: [PATCHv2] mfd: twl4030: clarify the return value for read and write

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 <[email protected]>
---
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

2009-12-02 15:06:10

by Anand Gadiyar

Subject: RE: [PATCHv2] mfd: twl4030: clarify the return value for read and write

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 <[email protected]>
> ---
> 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

2009-12-07 12:17:36

by Amit Kucheria

Subject: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write

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 <[email protected]>
---
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

2009-12-07 13:53:24

by Eduardo Valentin

Subject: Re: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write

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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
Eduardo Valentin

2009-12-11 10:34:37

by Samuel Ortiz

Subject: Re: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write

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 <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Samuel Ortiz <[email protected]>

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/

2009-12-11 10:54:07

by Eduardo Valentin

Subject: Re: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write

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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Samuel Ortiz <[email protected]>
>
> 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

2009-12-11 12:09:56

by Samuel Ortiz

Subject: Re: [PATCH 1/1] mfd: twl4030: clarify the return value for read and write

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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> > 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 <[email protected]>
> > Signed-off-by: Samuel Ortiz <[email protected]>
> >
> > 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/