Re: [PATCH v4 08/22] [media] em28xx: convert i2c wait completion logic to use jiffies

2014-01-06 Thread Frank Schäfer
Am 05.01.2014 14:10, schrieb Mauro Carvalho Chehab:
> Em Sun, 05 Jan 2014 12:03:51 +0100
> Frank Schäfer  escreveu:
>
>> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
>>> The I2C wait completion/timeout logic currently assumes that
>>> msleep(5) will wait exaclty 5 ms. This is not true at all,
>>> as it depends on CONFIG_HZ.
>>>
>>> Convert it to use jiffies, in order to not wait for more time
>>> than needed.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab 
>>> ---
>>>  drivers/media/usb/em28xx/em28xx-i2c.c | 65 
>>> ++-
>>>  1 file changed, 34 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c 
>>> b/drivers/media/usb/em28xx/em28xx-i2c.c
>>> index 9e6a11d01858..9fa7ed51e5b1 100644
>>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>>> @@ -26,6 +26,7 @@
>>>  #include 
>>>  #include 
>>>  #include 
>>> +#include 
>>>  
>>>  #include "em28xx.h"
>>>  #include "tuner-xc2028.h"
>>> @@ -48,8 +49,8 @@ MODULE_PARM_DESC(i2c_debug, "enable debug messages 
>>> [i2c]");
>>>   */
>>>  static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 
>>> len)
>>>  {
>>> +   unsigned long timeout = jiffies + 
>>> msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
>>> int ret;
>>> -   int write_timeout;
>>> u8 b2[6];
>>>  
>>> if (len < 1 || len > 4)
>>> @@ -74,15 +75,15 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 
>>> addr, u8 *buf, u16 len)
>>> return (ret < 0) ? ret : -EIO;
>>> }
>>> /* wait for completion */
>>> -   for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
>>> -write_timeout -= 5) {
>>> +   while (time_is_after_jiffies(timeout)) {
>> AFAIU, it must be time_is_before_jiffies(timeout).
> This is tricky, but it is right. 
>
> See its description at jiffies.h:
>
>   /* time_is_after_jiffies(a) return true if a is after jiffies */
>   #define time_is_after_jiffies(a) time_before(jiffies, a)
Urgh... yes, you are right.
I've read this, but didn't notice that we check the timeout jiffies and
not the current jiffies. :/
Sorry for the noise.

>
>>> ret = dev->em28xx_read_reg(dev, 0x05);
>>> -   if (ret == 0x80 + len - 1) {
>>> +   if (ret == 0x80 + len - 1)
>>> return len;
>>> -   } else if (ret == 0x94 + len - 1) {
>>> +   if (ret == 0x94 + len - 1) {
>>> em28xx_warn("R05 returned 0x%02x: I2C timeout", ret);
>>> return -ENODEV;
>>> -   } else if (ret < 0) {
>>> +   }
>>> +   if (ret < 0) {
>>> em28xx_warn("failed to get i2c transfer status from 
>>> bridge register (error=%i)\n",
>>> ret);
>>> return ret;
>>> @@ -99,9 +100,9 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 
>>> addr, u8 *buf, u16 len)
>>>   */
>>>  static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 
>>> len)
>>>  {
>>> +   unsigned long timeout = jiffies + 
>>> msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
>>> u8 buf2[4];
>>> int ret;
>>> -   int read_timeout;
>>> int i;
>>>  
>>> if (len < 1 || len > 4)
>>> @@ -118,15 +119,15 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, 
>>> u8 addr, u8 *buf, u16 len)
>>> }
>>>  
>>> /* wait for completion */
>>> -   for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
>>> -read_timeout -= 5) {
>>> +   while (time_is_after_jiffies(timeout)) {
>> The same here...
>>
>>> ret = dev->em28xx_read_reg(dev, 0x05);
>>> -   if (ret == 0x84 + len - 1) {
>>> +   if (ret == 0x84 + len - 1)
>>> break;
>>> -   } else if (ret == 0x94 + len - 1) {
>>> +   if (ret == 0x94 + len - 1) {
>>> em28xx_warn("R05 returned 0x%02x: I2C timeout", ret);
>>> return -ENODEV;
>>> -   } else if (ret < 0) {
>>> +   }
>>> +   if (ret < 0) {
>>> em28xx_warn("failed to get i2c transfer status from 
>>> bridge register (error=%i)\n",
>>> ret);
>>> return ret;
>>> @@ -170,7 +171,8 @@ static int em2800_i2c_check_for_device(struct em28xx 
>>> *dev, u8 addr)
>>>  static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>>  u16 len, int stop)
>>>  {
>>> -   int write_timeout, ret;
>>> +   unsigned long timeout = jiffies + 
>>> msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
>>> +   int ret;
>>>  
>>> if (len < 1 || len > 64)
>>> return -EOPNOTSUPP;
>>> @@ -193,17 +195,18 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, 
>>> u16 addr, u8 *buf,
>>> }
>>> }
>>>  
>>> -   /* Check success of the i2c operation */
>>> -   for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
>>> -write_timeout -= 5)

Re: [PATCH v4 08/22] [media] em28xx: convert i2c wait completion logic to use jiffies

2014-01-05 Thread Mauro Carvalho Chehab
Em Sun, 05 Jan 2014 12:03:51 +0100
Frank Schäfer  escreveu:

> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
> > The I2C wait completion/timeout logic currently assumes that
> > msleep(5) will wait exaclty 5 ms. This is not true at all,
> > as it depends on CONFIG_HZ.
> >
> > Convert it to use jiffies, in order to not wait for more time
> > than needed.
> >
> > Signed-off-by: Mauro Carvalho Chehab 
> > ---
> >  drivers/media/usb/em28xx/em28xx-i2c.c | 65 
> > ++-
> >  1 file changed, 34 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c 
> > b/drivers/media/usb/em28xx/em28xx-i2c.c
> > index 9e6a11d01858..9fa7ed51e5b1 100644
> > --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> > +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> > @@ -26,6 +26,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include "em28xx.h"
> >  #include "tuner-xc2028.h"
> > @@ -48,8 +49,8 @@ MODULE_PARM_DESC(i2c_debug, "enable debug messages 
> > [i2c]");
> >   */
> >  static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 
> > len)
> >  {
> > +   unsigned long timeout = jiffies + 
> > msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
> > int ret;
> > -   int write_timeout;
> > u8 b2[6];
> >  
> > if (len < 1 || len > 4)
> > @@ -74,15 +75,15 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 
> > addr, u8 *buf, u16 len)
> > return (ret < 0) ? ret : -EIO;
> > }
> > /* wait for completion */
> > -   for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
> > -write_timeout -= 5) {
> > +   while (time_is_after_jiffies(timeout)) {
> AFAIU, it must be time_is_before_jiffies(timeout).

This is tricky, but it is right. 

See its description at jiffies.h:

/* time_is_after_jiffies(a) return true if a is after jiffies */
#define time_is_after_jiffies(a) time_before(jiffies, a)

> 
> > ret = dev->em28xx_read_reg(dev, 0x05);
> > -   if (ret == 0x80 + len - 1) {
> > +   if (ret == 0x80 + len - 1)
> > return len;
> > -   } else if (ret == 0x94 + len - 1) {
> > +   if (ret == 0x94 + len - 1) {
> > em28xx_warn("R05 returned 0x%02x: I2C timeout", ret);
> > return -ENODEV;
> > -   } else if (ret < 0) {
> > +   }
> > +   if (ret < 0) {
> > em28xx_warn("failed to get i2c transfer status from 
> > bridge register (error=%i)\n",
> > ret);
> > return ret;
> > @@ -99,9 +100,9 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 
> > addr, u8 *buf, u16 len)
> >   */
> >  static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 
> > len)
> >  {
> > +   unsigned long timeout = jiffies + 
> > msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
> > u8 buf2[4];
> > int ret;
> > -   int read_timeout;
> > int i;
> >  
> > if (len < 1 || len > 4)
> > @@ -118,15 +119,15 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, 
> > u8 addr, u8 *buf, u16 len)
> > }
> >  
> > /* wait for completion */
> > -   for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
> > -read_timeout -= 5) {
> > +   while (time_is_after_jiffies(timeout)) {
> The same here...
> 
> > ret = dev->em28xx_read_reg(dev, 0x05);
> > -   if (ret == 0x84 + len - 1) {
> > +   if (ret == 0x84 + len - 1)
> > break;
> > -   } else if (ret == 0x94 + len - 1) {
> > +   if (ret == 0x94 + len - 1) {
> > em28xx_warn("R05 returned 0x%02x: I2C timeout", ret);
> > return -ENODEV;
> > -   } else if (ret < 0) {
> > +   }
> > +   if (ret < 0) {
> > em28xx_warn("failed to get i2c transfer status from 
> > bridge register (error=%i)\n",
> > ret);
> > return ret;
> > @@ -170,7 +171,8 @@ static int em2800_i2c_check_for_device(struct em28xx 
> > *dev, u8 addr)
> >  static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
> >  u16 len, int stop)
> >  {
> > -   int write_timeout, ret;
> > +   unsigned long timeout = jiffies + 
> > msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
> > +   int ret;
> >  
> > if (len < 1 || len > 64)
> > return -EOPNOTSUPP;
> > @@ -193,17 +195,18 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, 
> > u16 addr, u8 *buf,
> > }
> > }
> >  
> > -   /* Check success of the i2c operation */
> > -   for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
> > -write_timeout -= 5) {
> > +   /* wait for completion */
> > +   while (time_is_after_jiffies(timeout)) {
> ... and here.
> 
> > ret = dev->em28xx_read_reg(dev, 0x05);
> > -   if (ret == 0) { /* success */
> > +  

Re: [PATCH v4 08/22] [media] em28xx: convert i2c wait completion logic to use jiffies

2014-01-05 Thread Frank Schäfer
Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab:
> The I2C wait completion/timeout logic currently assumes that
> msleep(5) will wait exaclty 5 ms. This is not true at all,
> as it depends on CONFIG_HZ.
>
> Convert it to use jiffies, in order to not wait for more time
> than needed.
>
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/usb/em28xx/em28xx-i2c.c | 65 
> ++-
>  1 file changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c 
> b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 9e6a11d01858..9fa7ed51e5b1 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "em28xx.h"
>  #include "tuner-xc2028.h"
> @@ -48,8 +49,8 @@ MODULE_PARM_DESC(i2c_debug, "enable debug messages [i2c]");
>   */
>  static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 
> len)
>  {
> + unsigned long timeout = jiffies + 
> msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
>   int ret;
> - int write_timeout;
>   u8 b2[6];
>  
>   if (len < 1 || len > 4)
> @@ -74,15 +75,15 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 
> addr, u8 *buf, u16 len)
>   return (ret < 0) ? ret : -EIO;
>   }
>   /* wait for completion */
> - for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
> -  write_timeout -= 5) {
> + while (time_is_after_jiffies(timeout)) {
AFAIU, it must be time_is_before_jiffies(timeout).

>   ret = dev->em28xx_read_reg(dev, 0x05);
> - if (ret == 0x80 + len - 1) {
> + if (ret == 0x80 + len - 1)
>   return len;
> - } else if (ret == 0x94 + len - 1) {
> + if (ret == 0x94 + len - 1) {
>   em28xx_warn("R05 returned 0x%02x: I2C timeout", ret);
>   return -ENODEV;
> - } else if (ret < 0) {
> + }
> + if (ret < 0) {
>   em28xx_warn("failed to get i2c transfer status from 
> bridge register (error=%i)\n",
>   ret);
>   return ret;
> @@ -99,9 +100,9 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 
> addr, u8 *buf, u16 len)
>   */
>  static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 
> len)
>  {
> + unsigned long timeout = jiffies + 
> msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
>   u8 buf2[4];
>   int ret;
> - int read_timeout;
>   int i;
>  
>   if (len < 1 || len > 4)
> @@ -118,15 +119,15 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 
> addr, u8 *buf, u16 len)
>   }
>  
>   /* wait for completion */
> - for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
> -  read_timeout -= 5) {
> + while (time_is_after_jiffies(timeout)) {
The same here...

>   ret = dev->em28xx_read_reg(dev, 0x05);
> - if (ret == 0x84 + len - 1) {
> + if (ret == 0x84 + len - 1)
>   break;
> - } else if (ret == 0x94 + len - 1) {
> + if (ret == 0x94 + len - 1) {
>   em28xx_warn("R05 returned 0x%02x: I2C timeout", ret);
>   return -ENODEV;
> - } else if (ret < 0) {
> + }
> + if (ret < 0) {
>   em28xx_warn("failed to get i2c transfer status from 
> bridge register (error=%i)\n",
>   ret);
>   return ret;
> @@ -170,7 +171,8 @@ static int em2800_i2c_check_for_device(struct em28xx 
> *dev, u8 addr)
>  static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>u16 len, int stop)
>  {
> - int write_timeout, ret;
> + unsigned long timeout = jiffies + 
> msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
> + int ret;
>  
>   if (len < 1 || len > 64)
>   return -EOPNOTSUPP;
> @@ -193,17 +195,18 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, 
> u16 addr, u8 *buf,
>   }
>   }
>  
> - /* Check success of the i2c operation */
> - for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
> -  write_timeout -= 5) {
> + /* wait for completion */
> + while (time_is_after_jiffies(timeout)) {
... and here.

>   ret = dev->em28xx_read_reg(dev, 0x05);
> - if (ret == 0) { /* success */
> + if (ret == 0) /* success */
>   return len;
> - } else if (ret == 0x10) {
> - em28xx_warn("I2C transfer timeout on writing to addr 
> 0x%02x", addr);
> + if (ret == 0x10) {
> + em28xx_warn("I2C transfer timeout on writing to addr 
> 0x%02x",
> + addr);
>   re

[PATCH v4 08/22] [media] em28xx: convert i2c wait completion logic to use jiffies

2014-01-04 Thread Mauro Carvalho Chehab
The I2C wait completion/timeout logic currently assumes that
msleep(5) will wait exaclty 5 ms. This is not true at all,
as it depends on CONFIG_HZ.

Convert it to use jiffies, in order to not wait for more time
than needed.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/usb/em28xx/em28xx-i2c.c | 65 ++-
 1 file changed, 34 insertions(+), 31 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c 
b/drivers/media/usb/em28xx/em28xx-i2c.c
index 9e6a11d01858..9fa7ed51e5b1 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "em28xx.h"
 #include "tuner-xc2028.h"
@@ -48,8 +49,8 @@ MODULE_PARM_DESC(i2c_debug, "enable debug messages [i2c]");
  */
 static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
+   unsigned long timeout = jiffies + 
msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
int ret;
-   int write_timeout;
u8 b2[6];
 
if (len < 1 || len > 4)
@@ -74,15 +75,15 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 
addr, u8 *buf, u16 len)
return (ret < 0) ? ret : -EIO;
}
/* wait for completion */
-   for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
-write_timeout -= 5) {
+   while (time_is_after_jiffies(timeout)) {
ret = dev->em28xx_read_reg(dev, 0x05);
-   if (ret == 0x80 + len - 1) {
+   if (ret == 0x80 + len - 1)
return len;
-   } else if (ret == 0x94 + len - 1) {
+   if (ret == 0x94 + len - 1) {
em28xx_warn("R05 returned 0x%02x: I2C timeout", ret);
return -ENODEV;
-   } else if (ret < 0) {
+   }
+   if (ret < 0) {
em28xx_warn("failed to get i2c transfer status from 
bridge register (error=%i)\n",
ret);
return ret;
@@ -99,9 +100,9 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 
addr, u8 *buf, u16 len)
  */
 static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
+   unsigned long timeout = jiffies + 
msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
u8 buf2[4];
int ret;
-   int read_timeout;
int i;
 
if (len < 1 || len > 4)
@@ -118,15 +119,15 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 
addr, u8 *buf, u16 len)
}
 
/* wait for completion */
-   for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
-read_timeout -= 5) {
+   while (time_is_after_jiffies(timeout)) {
ret = dev->em28xx_read_reg(dev, 0x05);
-   if (ret == 0x84 + len - 1) {
+   if (ret == 0x84 + len - 1)
break;
-   } else if (ret == 0x94 + len - 1) {
+   if (ret == 0x94 + len - 1) {
em28xx_warn("R05 returned 0x%02x: I2C timeout", ret);
return -ENODEV;
-   } else if (ret < 0) {
+   }
+   if (ret < 0) {
em28xx_warn("failed to get i2c transfer status from 
bridge register (error=%i)\n",
ret);
return ret;
@@ -170,7 +171,8 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, 
u8 addr)
 static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 u16 len, int stop)
 {
-   int write_timeout, ret;
+   unsigned long timeout = jiffies + 
msecs_to_jiffies(EM2800_I2C_XFER_TIMEOUT);
+   int ret;
 
if (len < 1 || len > 64)
return -EOPNOTSUPP;
@@ -193,17 +195,18 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 
addr, u8 *buf,
}
}
 
-   /* Check success of the i2c operation */
-   for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
-write_timeout -= 5) {
+   /* wait for completion */
+   while (time_is_after_jiffies(timeout)) {
ret = dev->em28xx_read_reg(dev, 0x05);
-   if (ret == 0) { /* success */
+   if (ret == 0) /* success */
return len;
-   } else if (ret == 0x10) {
-   em28xx_warn("I2C transfer timeout on writing to addr 
0x%02x", addr);
+   if (ret == 0x10) {
+   em28xx_warn("I2C transfer timeout on writing to addr 
0x%02x",
+   addr);
return -ENODEV;
-   } else if (ret < 0) {
-   em28xx_warn("failed to read i2c transfer status from 
bridge (error=%i)\n",
+   }
+   if (ret < 0) {
+   em28xx_warn("failed to get i2c