Am 05.01.2014 14:10, schrieb Mauro Carvalho Chehab:
> Em Sun, 05 Jan 2014 12:03:51 +0100
> Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> 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 <m.chehab@xxxxxxxxxxx>
>>> ---
>>> 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 <linux/kernel.h>
>>> #include <linux/usb.h>
>>> #include <linux/i2c.h>
>>> +#include <linux/jiffies.h>
>>>
>>> #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) {
>>> + /* 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);
>>> 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 transfer status from bridge register (error=%i)\n",
>>> ret);
>>> return ret;
>>> }
>>> @@ -214,6 +217,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
>>> * (even with high payload) ...
>>> */
>>> }
>>> +
>>> em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
>>> return -EIO;
>>> }
>>> @@ -251,21 +255,20 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
>>>
>>> /* Check success of the i2c operation */
>>> ret = dev->em28xx_read_reg(dev, 0x05);
>>> + if (ret == 0) /* success */
>>> + return len;
>>> if (ret < 0) {
>>> - em28xx_warn("failed to read i2c transfer status from bridge (error=%i)\n",
>>> + em28xx_warn("failed to get i2c transfer status from bridge register (error=%i)\n",
>>> ret);
>>> return ret;
>>> }
>>> - if (ret > 0) {
>>> - if (ret == 0x10) {
>>> - em28xx_warn("I2C transfer timeout on read from addr 0x%02x", addr);
>>> - return -ENODEV;
>>> - } else {
>>> - em28xx_warn("unknown i2c error (status=%i)\n", ret);
>>> - return -EIO;
>>> - }
>>> + if (ret == 0x10) {
>>> + em28xx_warn("I2C transfer timeout on read from addr 0x%02x", addr);
>>> + return -ENODEV;
>>> }
>>> - return len;
>>> +
>>> + em28xx_warn("unknown i2c error (status=%i)\n", ret);
>>> + return -EIO;
>>> }
>>>
>>> /*
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
