Am 05.01.2014 14:10, schrieb Mauro Carvalho Chehab: > Em Sun, 05 Jan 2014 12:03:51 +0100 > Frank Schäfer <fschaefer....@googlemail.com> 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.che...@samsung.com> >>> --- >>> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html