Am 05.01.2014 21:57, schrieb Mauro Carvalho Chehab: > Em Sun, 05 Jan 2014 21:38:31 +0100 > Frank Schäfer <fschaefer.oss@xxxxxxxxxxxxxx> escreveu: > >> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab: >>> In the lack of a better spec, let's assume the timeout >>> values compatible with SMBus spec: >>> http://smbus.org/specs/smbus110.pdf >>> >>> at chapter 8 - Electrical Characteristics of SMBus devices >>> >>> Ok, SMBus is a subset of I2C, and not all devices will be >>> following it, but the timeout value before this patch was not >>> even following the spec. >>> >>> So, while we don't have a better guess for it, use 35 + 1 >>> ms as the timeout. >>> >>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@xxxxxxxxxxx> >>> --- >>> drivers/media/usb/em28xx/em28xx.h | 17 +++++++++++++++-- >>> 1 file changed, 15 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h >>> index db47c2236ca4..9af19332b0f1 100644 >>> --- a/drivers/media/usb/em28xx/em28xx.h >>> +++ b/drivers/media/usb/em28xx/em28xx.h >>> @@ -183,8 +183,21 @@ >>> >>> #define EM28XX_INTERLACED_DEFAULT 1 >>> >>> -/* time in msecs to wait for i2c xfers to finish */ >>> -#define EM2800_I2C_XFER_TIMEOUT 20 >>> +/* >>> + * Time in msecs to wait for i2c xfers to finish. >>> + * 35ms is the maximum time a SMBUS device could wait when >>> + * clock stretching is used. As the transfer itself will take >>> + * some time to happen, set it to 35 ms. >>> + * >>> + * Ok, I2C doesn't specify any limit. So, eventually, we may need >>> + * to increase this timeout. >>> + * >>> + * FIXME: this assumes that an I2C message is not longer than 1ms. >>> + * This is actually dependent on the I2C bus speed, although most >>> + * devices use a 100kHz clock. So, this assumtion is true most of >>> + * the time. >>> + */ >>> +#define EM2800_I2C_XFER_TIMEOUT 36 >>> >>> /* time in msecs to wait for AC97 xfers to finish */ >>> #define EM2800_AC97_XFER_TIMEOUT 100 >> Mauro... >> What exactly are you fixing with this patch ? > It fixes some of the timeouts I noticed here with HVR-950. > >> Which devices are not working with the current timeout value ? >> >> You really shouldn't increase the timout to 172% for all devices based >> on such a fragile pure theory. > It is not fragile. It is the SMBUS spec. It should _at_least_ wait up to > the timeout specified there. > > Btw, it is not increasing the timeout. It is actually reducing it. > > See, this is the code before the patch: > > for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0; > read_timeout -= 5) { > ret = dev->em28xx_read_reg(dev, 0x05); > if (ret == 0x84 + len - 1) { > break; > } else if (ret == 0x94 + len - 1) { > return -ENODEV; > } else if (ret < 0) { > em28xx_warn("failed to get i2c transfer status from bridge register (error=%i)\n", > ret); > return ret; > } > msleep(5); > } > > msleep(5) actually sleeps up to 20 ms, as the minimal time is the > schedule() time - being 10 ms a typical value (CONFIG_HZ equal to 100). > > So, the current code has a timeout of up to 100 ms. 20ms / 5ms = 4. 4 * 10ms = 40ms ? > This patch is actually reducing from 100 ms to 36 ms. Ok, it's the same as with AC97 reads/writes. I would accept any value from 20ms to 40ms. > Regards, > Mauro -- 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
