i2c: (i2c-diolan-u2c) Fix master_xfer return code

PATCH Applied Guenter Roeck 3 participants 6 messages Aug 9, 2012 – Aug 18, 2012 4 messages not in the archive

Subsystems (2)

I2C subsystem host drivers Diolan U2C-12 I2C driver

lore

Share & tools

lore

Guenter Roeck

author

2012-08-09 15:47 UTC #0 lore raw

PATCH 1/1 i2c: (i2c-diolan-u2c) Fix master_xfer return code

The master_xfer function returns 0 on success. It should return the number of successful transactions.

Signed-off-by Guenter Roeck <[email protected]>

---
 drivers/i2c/busses/i2c-diolan-u2c.c |    1 +
 1 file changed, 1 insertion(+)

drivers/i2c/busses/i2c-diolan-u2c.c +1 -0

index aedb94f..dae3ddf 100644
@@ -405,6 +405,7 @@ static int diolan_usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 			}
 		}
 	}
+	ret = num;
 abort:
 	sret = diolan_i2c_stop(dev);
 	if (sret < 0 && ret >= 0)

Signature · 1 line

1.7.9.7

A message that is not in the archive (sent off-list or not yet received): 1344527240-18266-1-git-send-email-linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org

Shubhrajyoti Datta

2012-08-10 05:58 UTC #2 lore raw

Hi Gunter,
A minor suggestion below.

Quoted 13 lines · Guenter Roeck

The master_xfer function returns 0 on success. It should return the number of successful transactions.
Signed-off-by Guenter Roeck <[email protected]>
---
 drivers/i2c/busses/i2c-diolan-u2c.c |    1 +
 1 file changed, 1 insertion(+)
drivers/i2c/busses/i2c-diolan-u2c.c +0 -0
index aedb94f..dae3ddf 100644

drivers/i2c/busses/i2c-diolan-u2c.c static int diolan_usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, · line 408

Quoted 4 lines

drivers/i2c/busses/i2c-diolan-u2c.c +1 -0

@@ -405,6 +405,7 @@ static int diolan_usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
                        }
                }
        }
+       ret = num;

So if all the messages get transferred this is right.

However if only a few go through should we return the number of successful transactions
instead of error?

Quoted 10 lines

drivers/i2c/busses/i2c-diolan-u2c.c +0 -1
 abort:
        sret = diolan_i2c_stop(dev);
        if (sret < 0 && ret >= 0)
--
1.7.9.7
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAM=Q2ctnrO6hJkPDa_daDRFNyig3+GVDkgW_MGMjV24jEM8T5Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Guenter Roeck

author

2012-08-10 17:06 UTC #4 lore raw

Quoted 23 lines · Shubhrajyoti Datta

Hi Gunter,
A minor suggestion below.
Quoted 13 lines · Guenter Roeck
The master_xfer function returns 0 on success. It should return the number of successful transactions.
Signed-off-by Guenter Roeck <[email protected]>
---
 drivers/i2c/busses/i2c-diolan-u2c.c |    1 +
 1 file changed, 1 insertion(+)
drivers/i2c/busses/i2c-diolan-u2c.c +0 -0
index aedb94f..dae3ddf 100644
drivers/i2c/busses/i2c-diolan-u2c.c static int diolan_usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, · line 408
Quoted 4 lines
drivers/i2c/busses/i2c-diolan-u2c.c +1 -0
@@ -405,6 +405,7 @@ static int diolan_usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
                        }
                }
        }
+       ret = num;
So if all the messages get transferred this is right.

Quoted 4 lines

However if only a few go through should we return the number of successful transactions
instead of error?

Most other I2C bus drivers do the same, so I decided to not make the code more complicated than necessary and do it the same way. Also, I prefer to have the actual error code returned to the caller, not "I transferred x of your y messages, but I won't tell you what went wrong with the rest".

If that prevents it from getting integrated, I'll be happy to change it.

Thanks,
Guenter

A message that is not in the archive (sent off-list or not yet received): [email protected]

Shubhrajyoti Datta

2012-08-13 06:27 UTC #6 lore raw

On Fri, Aug 10, 2012 at 10:36 PM, Guenter Roeck <[email protected]> wrote:
[...]

Quoted 9 lines

Quoted 5 lines
However if only a few go through should we return the number of successful transactions
instead of error?
Most other I2C bus drivers do the same, so I decided to not make the code more complicated than necessary and do it the same way. Also, I prefer to have the actual error code returned to the caller, not "I transferred x of your y messages, but I won't tell you what went wrong with the rest".

depends on what the user decides however if ever his algo is that resend only the remaining it would never work. Anyways thats a different story.

Quoted 2 lines

If that prevents it from getting integrated, I'll be happy to change it.

I have no objections to the patch getting integrated. Afterall anything that helps, helps:-)

Quoted 3 lines

Thanks,
Guenter

A message that is not in the archive (sent off-list or not yet received): CAM=Q2ctzBg+o-3On8-i-T49Dg9H=_2Tv7Z9iZba6iErTQoLVXw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Guenter Roeck

author

2012-08-13 16:05 UTC #8 lore raw

Quoted 7 lines · Shubhrajyoti Datta

On Fri, Aug 10, 2012 at 10:36 PM, Guenter Roeck <[email protected]> wrote:
[...]
Quoted 5 lines
Quoted 5 lines
However if only a few go through should we return the number of successful transactions
instead of error?

Quoted 8 lines

Quoted 4 lines
Most other I2C bus drivers do the same, so I decided to not make the code more complicated than necessary and do it the same way. Also, I prefer to have the actual error code returned to the caller, not "I transferred x of your y messages, but I won't tell you what went wrong with the rest".
depends on what the user decides however if ever his algo is that resend only the remaining it would never work. Anyways thats a different story.

Worse, many callers don't check if the return code matches the number of messages. So errors can get lost if the return code after an error is not negative.

Guenter

Quoted 11 lines

Quoted 2 lines
If that prevents it from getting integrated, I'll be happy to change it.
I have no objections to the patch getting integrated. Afterall anything that helps, helps:-)
Quoted 3 lines
Thanks,
Guenter

Wolfram Sang

Applied

2012-08-18 10:27 UTC #9 lore raw

In reply to a message not in the archive

Quoted 4 lines · Guenter Roeck

The master_xfer function returns 0 on success. It should return the number of successful transactions.
Signed-off-by Guenter Roeck <[email protected]>

Applied to -current, thanks! To add something to the discussion, an excerpt for i2c-core.c:

1357 int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) 1358 {
1359 int ret;
1360
1361 /* REVISIT the fault reporting model here is weak:

1362          *
1363          *  - When we get an error after receiving N bytes from a slave,
1364          *    there is no way to report "N".
1365          *
1366          *  - When we get a NAK after transmitting N bytes to a slave,
1367          *    there is no way to report "N" ... or to let the master
1368          *    continue executing the rest of this combined message, if
1369          *    that's the appropriate response.
1370          *
1371          *  - When for example "num" is two and we successfully complete
1372          *    the first message but get an error part way through the
1373          *    second, it's unclear whether that should be reported as
1374          *    one (discarding status on the second message) or errno
1375          *    (discarding status on the first one).
1376          */

1377

Signature · 2 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |