Re: [PATCH] i2c-davinci: Handle signals gracefully
Feel free to adap my patch or comments and commit. Or wait a few weeks for when I have a sponsor to split and update the patch. OK, I'll hope you can make it for 3.17. Thanks! signature.asc Description: Digital signature ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] i2c-davinci: Handle signals gracefully
On 05/21/2014 10:17 AM, Wolfram Sang wrote:
dev_err(dev-dev, controller timed out\n);
davinci_i2c_recover_bus(dev);
i2c_davinci_init(dev);
@@ -384,7 +384,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
if (dev-buf_len) {
/* This should be 0 if all bytes were transferred
* or dev-cmd_err denotes an error.
-* A signal may have aborted the transfer.
*/
if (r = 0) {
dev_err(dev-dev, abnormal termination buf_len=%i\n,
@@ -436,22 +435,24 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
ret = i2c_davinci_wait_bus_not_busy(dev, 1);
if (ret 0) {
dev_warn(dev-dev, timeout waiting for bus ready\n);
- return ret;
+ goto error;
You are fixing the error path here to include the completion? This is a
seperate patch IMO.
Is my remark true? I still prefer the seperate patch, but we may also
simply update the commit message.
Your remarks is correct. All your remarks were. Problem currently is
that I'm not assigned to a project related to the davinci so I cannot
spend time to fix and port it (the actual platform still runs 2.6.37).
Feel free to adap my patch or comments and commit. Or wait a few weeks
for when I have a sponsor to split and update the patch.
--
Mike Looijmans
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] i2c-davinci: Handle signals gracefully
dev_err(dev-dev, controller timed out\n);
davinci_i2c_recover_bus(dev);
i2c_davinci_init(dev);
@@ -384,7 +384,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
if (dev-buf_len) {
/* This should be 0 if all bytes were transferred
* or dev-cmd_err denotes an error.
-* A signal may have aborted the transfer.
*/
if (r = 0) {
dev_err(dev-dev, abnormal termination buf_len=%i\n,
@@ -436,22 +435,24 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct
i2c_msg msgs[], int num)
ret = i2c_davinci_wait_bus_not_busy(dev, 1);
if (ret 0) {
dev_warn(dev-dev, timeout waiting for bus ready\n);
- return ret;
+ goto error;
You are fixing the error path here to include the completion? This is a
seperate patch IMO.
Is my remark true? I still prefer the seperate patch, but we may also
simply update the commit message.
signature.asc
Description: Digital signature
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] i2c-davinci: Handle signals gracefully
diff --git a/drivers/i2c/busses/i2c-davinci.c
b/drivers/i2c/busses/i2c-davinci.c
index af0b583..254d897 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -372,9 +372,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
flag |= DAVINCI_I2C_MDR_STP;
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
- r = wait_for_completion_interruptible_timeout(dev-cmd_complete,
+ r = wait_for_completion_timeout(dev-cmd_complete,
dev-adapter.timeout);
- if (r == 0) {
+ if (unlikely(r == 0)) {
Not really needed, but well yeah...
dev_err(dev-dev, controller timed out\n);
davinci_i2c_recover_bus(dev);
i2c_davinci_init(dev);
@@ -384,7 +384,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct
i2c_msg *msg, int stop)
if (dev-buf_len) {
/* This should be 0 if all bytes were transferred
* or dev-cmd_err denotes an error.
- * A signal may have aborted the transfer.
*/
if (r = 0) {
dev_err(dev-dev, abnormal termination buf_len=%i\n,
@@ -436,22 +435,24 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct
i2c_msg msgs[], int num)
ret = i2c_davinci_wait_bus_not_busy(dev, 1);
if (ret 0) {
dev_warn(dev-dev, timeout waiting for bus ready\n);
- return ret;
+ goto error;
You are fixing the error path here to include the completion? This is a
seperate patch IMO.
}
for (i = 0; i num; i++) {
ret = i2c_davinci_xfer_msg(adap, msgs[i], (i == (num - 1)));
- dev_dbg(dev-dev, %s [%d/%d] ret: %d\n, __func__, i + 1, num,
- ret);
+ dev_dbg(dev-dev, %s [%d/%d] %#x ret: %d\n, __func__, i + 1,
+ num, msgs[i].addr, ret);
No need for this change. We have other debug output/tracing already. But
no need to clean up either.
if (ret 0)
- return ret;
+ goto error;
}
+ ret = num;
+error:
#ifdef CONFIG_CPU_FREQ
complete(dev-xfr_complete);
#endif
- return num;
+ return ret;
Thanks,
Wolfram
signature.asc
Description: Digital signature
___
Davinci-linux-open-source mailing list
Davinci-linux-open-source@linux.davincidsp.com
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] i2c-davinci: Handle signals gracefully
On 03/10/2014 04:24 PM, Wolfram Sang wrote: Even more, you should complete the whole transfer. There are devices where things can really go wrong if you send a half-complete command and then start with the next one. So, not checking signals at all is the way to go for I2C drivers. There is some cruft left, so I am happy about patches fixing that, with testing on real HW. Like yours here. I agree. I know the Zynq (using a cadence controller) also lets signals interrupt I2C transfers, so I'll propose a patch to Xilinx and CC to you and linux-i2c to completely remove signal handling from that driver as well. Cool, thanks! Are you going to update the davinci patch as well? An amended patch is on its way now. I forgot to set the subject to PATCHv2 though. Mike. Met vriendelijke groet / kind regards, Mike Looijmans TOPIC Embedded Systems Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: (+31) (0) 499 33 69 79 Telefax: (+31) (0) 499 33 69 70 E-mail: mike.looijm...@topic.nl Website: www.topic.nl Please consider the environment before printing this e-mail ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] i2c-davinci: Handle signals gracefully
Even more, you should complete the whole transfer. There are devices where things can really go wrong if you send a half-complete command and then start with the next one. So, not checking signals at all is the way to go for I2C drivers. There is some cruft left, so I am happy about patches fixing that, with testing on real HW. Like yours here. I agree. I know the Zynq (using a cadence controller) also lets signals interrupt I2C transfers, so I'll propose a patch to Xilinx and CC to you and linux-i2c to completely remove signal handling from that driver as well. Cool, thanks! signature.asc Description: Digital signature ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] i2c-davinci: Handle signals gracefully
Even more, you should complete the whole transfer. There are devices where things can really go wrong if you send a half-complete command and then start with the next one. So, not checking signals at all is the way to go for I2C drivers. There is some cruft left, so I am happy about patches fixing that, with testing on real HW. Like yours here. I agree. I know the Zynq (using a cadence controller) also lets signals interrupt I2C transfers, so I'll propose a patch to Xilinx and CC to you and linux-i2c to completely remove signal handling from that driver as well. Cool, thanks! Are you going to update the davinci patch as well? signature.asc Description: Digital signature ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH] i2c-davinci: Handle signals gracefully
On Thu, Jan 09, 2014 at 12:11:25PM +0100, Mike Looijmans wrote: When a signal is caught while the i2c-davinci bus driver is transferring, the drive just abandons the transfer and leaves the controller to fend for itself. The next I2C transaction will find the controller in an undefined state and often results in a stream of initiating i2c bus recovery messages until the controller arrives in a defined state. This behaviour also sends out half or possibly even mixed messages to I2C client devices which may put them in an undesired state as well. This patch fixes this issue by always attempting to finish the current transaction, and then check on a pending signal. It either reports success if all data has been transferred, or it returns failure when the transaction was aborted. This keeps the controller in a defined state, and is also much friendlier towards client devices, because it will only send complete messages. Even more, you should complete the whole transfer. There are devices where things can really go wrong if you send a half-complete command and then start with the next one. So, not checking signals at all is the way to go for I2C drivers. There is some cruft left, so I am happy about patches fixing that, with testing on real HW. Like yours here. signature.asc Description: Digital signature ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
