Re: [PATCH] i2c-designware: fixup of wait_for_completion_timeout return handling

2015-02-09 Thread Jarkko Nikula

Hi

On 02/09/2015 06:35 PM, Nicholas Mc Guire wrote:

return type of wait_for_completion_timeout is unsigned long not int.
An appropriate return variable is introduced and assignment fixed up.

Signed-off-by: Nicholas Mc Guire hof...@osadl.org
---

Patch was compile tested with x86_64_defconfig +
CONFIG_X86_AMD_PLATFORM_DEVICE=y, CONFIG_I2C_DESIGNWARE_PLATFORM=m
(implies CONFIG_I2C_DESIGNWARE_CORE=y)

Patch is against 3.19.0-rc7 (localversion-next is -next-20150209)

  drivers/i2c/busses/i2c-designware-core.c |5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 6e25c01..05934e0 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -623,6 +623,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
msgs[], int num)
  {
struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
int ret;
+   unsigned int timeout;


Not unsigned long.


@@ -656,8 +657,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
msgs[], int num)
i2c_dw_xfer_init(dev);

/* wait for tx to complete */
-   ret = wait_for_completion_timeout(dev-cmd_complete, HZ);
-   if (ret == 0) {
+   timeout = wait_for_completion_timeout(dev-cmd_complete, HZ);
+   if (timeout == 0) {
dev_err(dev-dev, controller timed out\n);
/* i2c_dw_init implicitly disables the adapter */
i2c_dw_init(dev);

I'd say better to test directly with if 
(!wait_for_completion_timeout()) since remaining jiffies or potential 
error code from wait_for_completion_timeout() is not used here.


--
Jarkko
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] i2c-designware: fixup of wait_for_completion_timeout return handling

2015-02-09 Thread Nicholas Mc Guire
On Tue, 10 Feb 2015, Jarkko Nikula wrote:

 Hi

 On 02/09/2015 06:35 PM, Nicholas Mc Guire wrote:
 return type of wait_for_completion_timeout is unsigned long not int.
 An appropriate return variable is introduced and assignment fixed up.

 Signed-off-by: Nicholas Mc Guire hof...@osadl.org
 ---

 Patch was compile tested with x86_64_defconfig +
 CONFIG_X86_AMD_PLATFORM_DEVICE=y, CONFIG_I2C_DESIGNWARE_PLATFORM=m
 (implies CONFIG_I2C_DESIGNWARE_CORE=y)

 Patch is against 3.19.0-rc7 (localversion-next is -next-20150209)

   drivers/i2c/busses/i2c-designware-core.c |5 +++--
   1 file changed, 3 insertions(+), 2 deletions(-)

 diff --git a/drivers/i2c/busses/i2c-designware-core.c 
 b/drivers/i2c/busses/i2c-designware-core.c
 index 6e25c01..05934e0 100644
 --- a/drivers/i2c/busses/i2c-designware-core.c
 +++ b/drivers/i2c/busses/i2c-designware-core.c
 @@ -623,6 +623,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
 msgs[], int num)
   {
  struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
  int ret;
 +unsigned int timeout;

 Not unsigned long.

oops thats braindead - yes of course unsigned long
sorry - thats a careless mistake.


 @@ -656,8 +657,8 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
 msgs[], int num)
  i2c_dw_xfer_init(dev);

  /* wait for tx to complete */
 -ret = wait_for_completion_timeout(dev-cmd_complete, HZ);
 -if (ret == 0) {
 +timeout = wait_for_completion_timeout(dev-cmd_complete, HZ);
 +if (timeout == 0) {
  dev_err(dev-dev, controller timed out\n);
  /* i2c_dw_init implicitly disables the adapter */
  i2c_dw_init(dev);

 I'd say better to test directly with if  
 (!wait_for_completion_timeout()) since remaining jiffies or potential  
 error code from wait_for_completion_timeout() is not used here.


Yup - that would eliminate the need for an additional variable
but some maintainers did not like that solution.

Thanks for your comments - Will fix it up and repost.

thx!
hofrat
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html