[PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Chuansheng Liu]

The return value of wait_for_completion_timeout() is always
>= 0 with unsigned int type.

So the condition "ret < 0" or "ret >= 0" is pointless.

Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 drivers/i2c/busses/i2c-nomadik.c |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 02c3115..8b2ffcf 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -435,13 +435,6 @@ static int read_i2c(struct nmk_i2c_dev *dev, u16 flags)
 	timeout = wait_for_completion_timeout(
 		&dev->xfer_complete, dev->adap.timeout);
 
-	if (timeout < 0) {
-		dev_err(&dev->adev->dev,
-			"wait_for_completion_timeout "
-			"returned %d waiting for event\n", timeout);
-		status = timeout;
-	}
-
 	if (timeout == 0) {
 		/* Controller timed out */
 		dev_err(&dev->adev->dev, "read from slave 0x%x timed out\n",
@@ -523,13 +516,6 @@ static int write_i2c(struct nmk_i2c_dev *dev, u16 flags)
 	timeout = wait_for_completion_timeout(
 		&dev->xfer_complete, dev->adap.timeout);
 
-	if (timeout < 0) {
-		dev_err(&dev->adev->dev,
-			"wait_for_completion_timeout "
-			"returned %d waiting for event\n", timeout);
-		status = timeout;
-	}
-
 	if (timeout == 0) {
 		/* Controller timed out */
 		dev_err(&dev->adev->dev, "write to slave 0x%x timed out\n",
-- 
1.7.0.4

[PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Wolfram Sang]

On Wed, Nov 07, 2012 at 01:18:37AM +0800, Chuansheng Liu wrote:
> 
> The return value of wait_for_completion_timeout() is always
> >= 0 with unsigned int type.
> 
> So the condition "ret < 0" or "ret >= 0" is pointless.
> 
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>

You forgot to add the i2c-list, so I nearly overlooked this patch.
Luckily I haven't, so applied to for-next, thanks! Did you use a script
to find all occurences? Would be nice to know. Also, please take care of
the subject line. "I2c" is misspelled.

@Linus: I am not pushing out today, so if you want to add acks or so...

Thanks,

   Wolfram

> ---
>  drivers/i2c/busses/i2c-nomadik.c |   14 --------------
>  1 files changed, 0 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 02c3115..8b2ffcf 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -435,13 +435,6 @@ static int read_i2c(struct nmk_i2c_dev *dev, u16 flags)
>  	timeout = wait_for_completion_timeout(
>  		&dev->xfer_complete, dev->adap.timeout);
>  
> -	if (timeout < 0) {
> -		dev_err(&dev->adev->dev,
> -			"wait_for_completion_timeout "
> -			"returned %d waiting for event\n", timeout);
> -		status = timeout;
> -	}
> -
>  	if (timeout == 0) {
>  		/* Controller timed out */
>  		dev_err(&dev->adev->dev, "read from slave 0x%x timed out\n",
> @@ -523,13 +516,6 @@ static int write_i2c(struct nmk_i2c_dev *dev, u16 flags)
>  	timeout = wait_for_completion_timeout(
>  		&dev->xfer_complete, dev->adap.timeout);
>  
> -	if (timeout < 0) {
> -		dev_err(&dev->adev->dev,
> -			"wait_for_completion_timeout "
> -			"returned %d waiting for event\n", timeout);
> -		status = timeout;
> -	}
> -
>  	if (timeout == 0) {
>  		/* Controller timed out */
>  		dev_err(&dev->adev->dev, "write to slave 0x%x timed out\n",
> -- 
> 1.7.0.4
> 
> 
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121112/b7c7ac45/attachment.sig>

[PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Liu, Chuansheng]

> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang at pengutronix.de]
> Sent: Tuesday, November 13, 2012 1:27 AM
> To: Liu, Chuansheng
> Cc: linus.walleij at linaro.org; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; linux-i2c at vger.kernel.org
> Subject: Re: [PATCH 1/7] I2c-nomadik: Fix the usage of
> wait_for_completion_timeout
> 
> On Wed, Nov 07, 2012 at 01:18:37AM +0800, Chuansheng Liu wrote:
> >
> > The return value of wait_for_completion_timeout() is always
> > >= 0 with unsigned int type.
> >
> > So the condition "ret < 0" or "ret >= 0" is pointless.
> >
> > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> 
> You forgot to add the i2c-list, so I nearly overlooked this patch.
> Luckily I haven't, so applied to for-next, thanks! Did you use a script
> to find all occurences? Would be nice to know. Also, please take care of
> the subject line. "I2c" is misspelled.
Thanks your pointing out. I just manually reviewed the code when found this point.
Would like to try some auto-script but failed:)

[PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Wolfram Sang]

> Thanks your pointing out. I just manually reviewed the code when found
> this point. Would like to try some auto-script but failed:)

I can suggest 'coccinelle' for that kind of job.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121113/65f95058/attachment.sig>

[PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Liu, Chuansheng]

> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang at pengutronix.de]
> Sent: Tuesday, November 13, 2012 3:58 PM
> To: Liu, Chuansheng
> Cc: linus.walleij at linaro.org; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; linux-i2c at vger.kernel.org
> Subject: Re: [PATCH 1/7] I2c-nomadik: Fix the usage of
> wait_for_completion_timeout
> 
> 
> > Thanks your pointing out. I just manually reviewed the code when found
> > this point. Would like to try some auto-script but failed:)
> 
> I can suggest 'coccinelle' for that kind of job.
Thanks, let me try, if possible anyone can give some help?
> 
> --
> Pengutronix e.K.                           | Wolfram Sang
> |
> Industrial Linux Solutions                 | http://www.pengutronix.de/
> |

Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Srinidhi Kasagar]

[...]

> From: Chuansheng Liu <chuansheng.liu@intel.com>
> Date: Tue, Nov 6, 2012 at 6:18 PM
> Subject: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout
> To: linus.walleij at linaro.org, w.sang at pengutronix.de
> Cc: linux-arm-kernel at lists.infradead.org,
> linux-kernel at vger.kernel.org, chuansheng.liu at intel.com
> 
> 
> 
> The return value of wait_for_completion_timeout() is always
> >= 0 with unsigned int type.
> 
> So the condition "ret < 0" or "ret >= 0" is pointless.
> 
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> ---
>  drivers/i2c/busses/i2c-nomadik.c |   14 --------------
>  1 files changed, 0 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
> index 02c3115..8b2ffcf 100644
> --- a/drivers/i2c/busses/i2c-nomadik.c
> +++ b/drivers/i2c/busses/i2c-nomadik.c
> @@ -435,13 +435,6 @@ static int read_i2c(struct nmk_i2c_dev *dev, u16 flags)
>         timeout = wait_for_completion_timeout(
>                 &dev->xfer_complete, dev->adap.timeout);
> 
> -       if (timeout < 0) {
> -               dev_err(&dev->adev->dev,
> -                       "wait_for_completion_timeout "
> -                       "returned %d waiting for event\n", timeout);
> -               status = timeout;
> -       }
> -
No, it is wrong. You need to update the status variable in the case of timeout.
It is used further in nmk_i2c_xfer_one. You could perhaps use

if (timeout == 0) {
 ...and the rest of the code as is
}

regards/srinidhi

Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Wolfram Sang]

> > -       if (timeout < 0) {
> > -               dev_err(&dev->adev->dev,
> > -                       "wait_for_completion_timeout "
> > -                       "returned %d waiting for event\n", timeout);
> > -               status = timeout;
> > -       }
> > -
> No, it is wrong. You need to update the status variable in the case of timeout.

Looking at the patch context, such code comes later.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121115/f7edc23a/attachment.sig>

Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Srinidhi Kasagar]

On Thu, Nov 15, 2012 at 10:29:53 +0100, Wolfram Sang wrote:
> 
> > > -       if (timeout < 0) {
> > > -               dev_err(&dev->adev->dev,
> > > -                       "wait_for_completion_timeout "
> > > -                       "returned %d waiting for event\n", timeout);
> > > -               status = timeout;
> > > -       }
> > > -
> > No, it is wrong. You need to update the status variable in the case of timeout.
> 
> Looking at the patch context, such code comes later.
But it causes regressions; without looking at the "later" code, we can't afford merging
this code now.

regards/srinidhi

Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Wolfram Sang]

On Thu, Nov 15, 2012 at 03:27:42PM +0530, Srinidhi Kasagar wrote:
> On Thu, Nov 15, 2012 at 10:29:53 +0100, Wolfram Sang wrote:
> > 
> > > > -       if (timeout < 0) {
> > > > -               dev_err(&dev->adev->dev,
> > > > -                       "wait_for_completion_timeout "
> > > > -                       "returned %d waiting for event\n", timeout);
> > > > -               status = timeout;
> > > > -       }
> > > > -
> > > No, it is wrong. You need to update the status variable in the case of timeout.
> > 
> > Looking at the patch context, such code comes later.
> But it causes regressions; without looking at the "later" code, we can't afford merging
> this code now.

Later as in "a few lines later" not "some time later". Or am I missing
something else?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121115/24943c8e/attachment.sig>

Fwd: [PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Srinidhi Kasagar]

On Thu, Nov 15, 2012 at 11:18:20 +0100, Wolfram Sang wrote:
> On Thu, Nov 15, 2012 at 03:27:42PM +0530, Srinidhi Kasagar wrote:
> > On Thu, Nov 15, 2012 at 10:29:53 +0100, Wolfram Sang wrote:
> > > 
> > > > > -       if (timeout < 0) {
> > > > > -               dev_err(&dev->adev->dev,
> > > > > -                       "wait_for_completion_timeout "
> > > > > -                       "returned %d waiting for event\n", timeout);
> > > > > -               status = timeout;
> > > > > -       }
> > > > > -
> > > > No, it is wrong. You need to update the status variable in the case of timeout.
> > > 
> > > Looking at the patch context, such code comes later.
> > But it causes regressions; without looking at the "later" code, we can't afford merging
> > this code now.
> 
> Later as in "a few lines later" not "some time later". Or am I missing
> something else?
I was too fast in reading emails after my short vacation...Sorry.

Acked-by: srinidhi kasagar <srinidhi.kasagar@stericsson.com>

regards/srinidhi

[PATCH 1/7] I2c-nomadik: Fix the usage of wait_for_completion_timeout

[Chuansheng Liu]

timeout is never < 0, so status = timeout is never getting run. This patch delete pointless
code. Could you show the regression? Thanks.

? 2012-11-15?17:57?Srinidhi Kasagar <srinidhi.kasagar@stericsson.com> ???

> On Thu, Nov 15, 2012 at 10:29:53 +0100, Wolfram Sang wrote:
>> 
>>>> -       if (timeout < 0) {
>>>> -               dev_err(&dev->adev->dev,
>>>> -                       "wait_for_completion_timeout "
>>>> -                       "returned %d waiting for event\n", timeout);
>>>> -               status = timeout;
>>>> -       }
>>>> -
>>> No, it is wrong. You need to update the status variable in the case of timeout.
>> 
>> Looking at the patch context, such code comes later.
> But it causes regressions; without looking at the "later" code, we can't afford merging
> this code now.
> 
> regards/srinidhi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121115/0f1d0c0f/attachment-0001.html>