Re: [PATCH v4 08/22] [media] em28xx: convert i2c wait completion logic to use jiffies
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
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
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
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