| Message ID | 1388832951-11195-18-git-send-email-m.chehab@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab: > The proper error code for I2C errors are EREMOTEIO. The em28xx driver > is using EIO instead. > > Replace all occurrences of EIO at em28xx-i2c, in order to fix it. > > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> > --- > drivers/media/usb/em28xx/em28xx-i2c.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c > index 9fa7ed51e5b1..8b35aa51b9bb 100644 > --- a/drivers/media/usb/em28xx/em28xx-i2c.c > +++ b/drivers/media/usb/em28xx/em28xx-i2c.c > @@ -72,7 +72,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > if (ret != 2 + len) { > em28xx_warn("failed to trigger write to i2c address 0x%x (error=%i)\n", > addr, ret); > - return (ret < 0) ? ret : -EIO; > + return (ret < 0) ? ret : -EREMOTEIO; > } > /* wait for completion */ > while (time_is_after_jiffies(timeout)) { > @@ -91,7 +91,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > msleep(5); > } > em28xx_warn("write to i2c device at 0x%x timed out\n", addr); > - return -EIO; > + return -EREMOTEIO; > } > > /* > @@ -115,7 +115,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > if (ret != 2) { > em28xx_warn("failed to trigger read from i2c address 0x%x (error=%i)\n", > addr, ret); > - return (ret < 0) ? ret : -EIO; > + return (ret < 0) ? ret : -EREMOTEIO; > } > > /* wait for completion */ > @@ -142,7 +142,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > if (ret != len) { > em28xx_warn("reading from i2c device at 0x%x failed: couldn't get the received message from the bridge (error=%i)\n", > addr, ret); > - return (ret < 0) ? ret : -EIO; > + return (ret < 0) ? ret : -EREMOTEIO; > } > for (i = 0; i < len; i++) > buf[i] = buf2[len - 1 - i]; > @@ -162,7 +162,7 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr) > ret = em2800_i2c_recv_bytes(dev, addr, &buf, 1); > if (ret == 1) > return 0; > - return (ret < 0) ? ret : -EIO; > + return (ret < 0) ? ret : -EREMOTEIO; > } > > /* > @@ -191,7 +191,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > } else { > em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", > len, addr, ret); > - return -EIO; > + return -EREMOTEIO; > } > } > > @@ -219,7 +219,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > } > > em28xx_warn("write to i2c device at 0x%x timed out\n", addr); > - return -EIO; > + return -EREMOTEIO; > } > > /* > @@ -268,7 +268,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len) > } > > em28xx_warn("unknown i2c error (status=%i)\n", ret); > - return -EIO; > + return -EREMOTEIO; > } > > /* > @@ -283,7 +283,7 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) > ret = em28xx_i2c_recv_bytes(dev, addr, &buf, 1); > if (ret == 1) > return 0; > - return (ret < 0) ? ret : -EIO; > + return (ret < 0) ? ret : -EREMOTEIO; > } > > /* > @@ -312,7 +312,7 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > } else { > em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", > len, addr, ret); > - return -EIO; > + return -EREMOTEIO; > } > } > /* Check success */ Why the hell -EREMOTEIO ??? See Documentation/i2c/fault-codes. It's not even listed there ! What are you trying to fix here ? -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sun, 05 Jan 2014 21:40:38 +0100 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab: > > The proper error code for I2C errors are EREMOTEIO. The em28xx driver > > is using EIO instead. > > > > Replace all occurrences of EIO at em28xx-i2c, in order to fix it. > > > > Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> > > --- > > drivers/media/usb/em28xx/em28xx-i2c.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c > > index 9fa7ed51e5b1..8b35aa51b9bb 100644 > > --- a/drivers/media/usb/em28xx/em28xx-i2c.c > > +++ b/drivers/media/usb/em28xx/em28xx-i2c.c > > @@ -72,7 +72,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > > if (ret != 2 + len) { > > em28xx_warn("failed to trigger write to i2c address 0x%x (error=%i)\n", > > addr, ret); > > - return (ret < 0) ? ret : -EIO; > > + return (ret < 0) ? ret : -EREMOTEIO; > > } > > /* wait for completion */ > > while (time_is_after_jiffies(timeout)) { > > @@ -91,7 +91,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > > msleep(5); > > } > > em28xx_warn("write to i2c device at 0x%x timed out\n", addr); > > - return -EIO; > > + return -EREMOTEIO; > > } > > > > /* > > @@ -115,7 +115,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > > if (ret != 2) { > > em28xx_warn("failed to trigger read from i2c address 0x%x (error=%i)\n", > > addr, ret); > > - return (ret < 0) ? ret : -EIO; > > + return (ret < 0) ? ret : -EREMOTEIO; > > } > > > > /* wait for completion */ > > @@ -142,7 +142,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > > if (ret != len) { > > em28xx_warn("reading from i2c device at 0x%x failed: couldn't get the received message from the bridge (error=%i)\n", > > addr, ret); > > - return (ret < 0) ? ret : -EIO; > > + return (ret < 0) ? ret : -EREMOTEIO; > > } > > for (i = 0; i < len; i++) > > buf[i] = buf2[len - 1 - i]; > > @@ -162,7 +162,7 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr) > > ret = em2800_i2c_recv_bytes(dev, addr, &buf, 1); > > if (ret == 1) > > return 0; > > - return (ret < 0) ? ret : -EIO; > > + return (ret < 0) ? ret : -EREMOTEIO; > > } > > > > /* > > @@ -191,7 +191,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > > } else { > > em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", > > len, addr, ret); > > - return -EIO; > > + return -EREMOTEIO; > > } > > } > > > > @@ -219,7 +219,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > > } > > > > em28xx_warn("write to i2c device at 0x%x timed out\n", addr); > > - return -EIO; > > + return -EREMOTEIO; > > } > > > > /* > > @@ -268,7 +268,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len) > > } > > > > em28xx_warn("unknown i2c error (status=%i)\n", ret); > > - return -EIO; > > + return -EREMOTEIO; > > } > > > > /* > > @@ -283,7 +283,7 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) > > ret = em28xx_i2c_recv_bytes(dev, addr, &buf, 1); > > if (ret == 1) > > return 0; > > - return (ret < 0) ? ret : -EIO; > > + return (ret < 0) ? ret : -EREMOTEIO; > > } > > > > /* > > @@ -312,7 +312,7 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > > } else { > > em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", > > len, addr, ret); > > - return -EIO; > > + return -EREMOTEIO; > > } > > } > > /* Check success */ > Why the hell -EREMOTEIO ??? > See Documentation/i2c/fault-codes. > It's not even listed there ! > > What are you trying to fix here ? This is not a fixup patch. The idea of this path is to make em28xx more compliant with the Kernel error codes. As em28xx was the first USB media driver, it doesn't follow the best standards, despite all efforts we've made back in 2005, when the driver was merged, in order to follow the Kernel rules. We eventually fixed several things along the time, but we didn't took much care to fix the I2C error codes so far. Now that we're taking care of it, we should do it right. However, you're actually right here. Before 2011, all I2C drivers under drivers/i2c used to return EREMOTEIO, and the media drivers were moving to this error code since then. But it seems that we missed a series of patches that moved I2C away from EREMOTEIO: http://www.spinics.net/lists/linux-i2c/msg06395.html https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg04819.html I'll rewrite this patch to return the right error codes, according with Documentation/i2c/fault-codes. It seems that the proper return codes are: - ETIMEDOUT - when reg 05 returns 0x10 - ENXIO - when the device is not temporarily not responding (e. g. reg 05 returning something not 0x10 or 0x00) - EBUSY - when reg 05 returns 0x20 on em2874 and upper [1] - EIO - for generic I/O errors that don't fit into the above. [1] According with a post that Devin made on IRC sometime ago, bit 5 indicates that the I2C is busy when the master tries to use it, but it exists only on em2874 (and likely newer chips). Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 06.01.2014 10:55, schrieb Mauro Carvalho Chehab: > Em Sun, 05 Jan 2014 21:40:38 +0100 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > >> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab: >>> The proper error code for I2C errors are EREMOTEIO. The em28xx driver >>> is using EIO instead. >>> >>> Replace all occurrences of EIO at em28xx-i2c, in order to fix it. >>> >>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> >>> --- >>> drivers/media/usb/em28xx/em28xx-i2c.c | 20 ++++++++++---------- >>> 1 file changed, 10 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c >>> index 9fa7ed51e5b1..8b35aa51b9bb 100644 >>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c >>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c >>> @@ -72,7 +72,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) >>> if (ret != 2 + len) { >>> em28xx_warn("failed to trigger write to i2c address 0x%x (error=%i)\n", >>> addr, ret); >>> - return (ret < 0) ? ret : -EIO; >>> + return (ret < 0) ? ret : -EREMOTEIO; >>> } >>> /* wait for completion */ >>> while (time_is_after_jiffies(timeout)) { >>> @@ -91,7 +91,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) >>> msleep(5); >>> } >>> em28xx_warn("write to i2c device at 0x%x timed out\n", addr); >>> - return -EIO; >>> + return -EREMOTEIO; >>> } >>> >>> /* >>> @@ -115,7 +115,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) >>> if (ret != 2) { >>> em28xx_warn("failed to trigger read from i2c address 0x%x (error=%i)\n", >>> addr, ret); >>> - return (ret < 0) ? ret : -EIO; >>> + return (ret < 0) ? ret : -EREMOTEIO; >>> } >>> >>> /* wait for completion */ >>> @@ -142,7 +142,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) >>> if (ret != len) { >>> em28xx_warn("reading from i2c device at 0x%x failed: couldn't get the received message from the bridge (error=%i)\n", >>> addr, ret); >>> - return (ret < 0) ? ret : -EIO; >>> + return (ret < 0) ? ret : -EREMOTEIO; >>> } >>> for (i = 0; i < len; i++) >>> buf[i] = buf2[len - 1 - i]; >>> @@ -162,7 +162,7 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr) >>> ret = em2800_i2c_recv_bytes(dev, addr, &buf, 1); >>> if (ret == 1) >>> return 0; >>> - return (ret < 0) ? ret : -EIO; >>> + return (ret < 0) ? ret : -EREMOTEIO; >>> } >>> >>> /* >>> @@ -191,7 +191,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, >>> } else { >>> em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", >>> len, addr, ret); >>> - return -EIO; >>> + return -EREMOTEIO; >>> } >>> } >>> >>> @@ -219,7 +219,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, >>> } >>> >>> em28xx_warn("write to i2c device at 0x%x timed out\n", addr); >>> - return -EIO; >>> + return -EREMOTEIO; >>> } >>> >>> /* >>> @@ -268,7 +268,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len) >>> } >>> >>> em28xx_warn("unknown i2c error (status=%i)\n", ret); >>> - return -EIO; >>> + return -EREMOTEIO; >>> } >>> >>> /* >>> @@ -283,7 +283,7 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) >>> ret = em28xx_i2c_recv_bytes(dev, addr, &buf, 1); >>> if (ret == 1) >>> return 0; >>> - return (ret < 0) ? ret : -EIO; >>> + return (ret < 0) ? ret : -EREMOTEIO; >>> } >>> >>> /* >>> @@ -312,7 +312,7 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, >>> } else { >>> em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", >>> len, addr, ret); >>> - return -EIO; >>> + return -EREMOTEIO; >>> } >>> } >>> /* Check success */ >> Why the hell -EREMOTEIO ??? >> See Documentation/i2c/fault-codes. >> It's not even listed there ! >> What are you trying to fix here ? > This is not a fixup patch. > > The idea of this path is to make em28xx more compliant with the Kernel > error codes. > > As em28xx was the first USB media driver, it doesn't follow the best > standards, despite all efforts we've made back in 2005, when the driver > was merged, in order to follow the Kernel rules. We eventually fixed > several things along the time, but we didn't took much care to fix the > I2C error codes so far. > > Now that we're taking care of it, we should do it right. > > However, you're actually right here. > > Before 2011, all I2C drivers under drivers/i2c used to return EREMOTEIO, > and the media drivers were moving to this error code since then. Ok, that explains it. > But it seems that we missed a series of patches that moved I2C away from > EREMOTEIO: > http://www.spinics.net/lists/linux-i2c/msg06395.html > https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg04819.html > > I'll rewrite this patch to return the right error codes, according with > Documentation/i2c/fault-codes. > > It seems that the proper return codes are: > > - ETIMEDOUT - when reg 05 returns 0x10 I still don't agree here. AFAICS an I2C timeout can only happen if clock stretching is used. But we also get this when a slave device isn't present or doesn't answer, which means it doesn't ACK the data. So ETIMEDOUT is wrong. Yes, 0x10 _could_ be a more general error. In that case EIO would be the better choice. But that's pure speculation as long as we don't have the datasheets. After reading the i2c fault codes descriptions again, I would agree to change it from ENODEV to ENXIO. ENXIO seems to be the intended error code for NACK errors and it's a bit more unspecific than ENODEV. Would that be acceptable for you ? > - ENXIO - when the device is not temporarily not responding > (e. g. reg 05 returning something not 0x10 or 0x00) For those I would return just EIO. I thought we agree here ? > - EBUSY - when reg 05 returns 0x20 on em2874 and upper [1] Yes, that's ok > - EIO - for generic I/O errors that don't fit into the above. I'm not sure what you mean with "generic", but yeah, that's ok, too. > [1] According with a post that Devin made on IRC sometime ago, bit 5 > indicates that the I2C is busy when the master tries to use it, but it > exists only on em2874 (and likely newer chips). Can give me a pointer to this post ? Regards, Frank > Cheers, > Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Tue, 07 Jan 2014 18:28:45 +0100 Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > Am 06.01.2014 10:55, schrieb Mauro Carvalho Chehab: > > Em Sun, 05 Jan 2014 21:40:38 +0100 > > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: > > > >> Am 04.01.2014 11:55, schrieb Mauro Carvalho Chehab: > >>> The proper error code for I2C errors are EREMOTEIO. The em28xx driver > >>> is using EIO instead. > >>> > >>> Replace all occurrences of EIO at em28xx-i2c, in order to fix it. > >>> > >>> Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> > >>> --- > >>> drivers/media/usb/em28xx/em28xx-i2c.c | 20 ++++++++++---------- > >>> 1 file changed, 10 insertions(+), 10 deletions(-) > >>> > >>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c > >>> index 9fa7ed51e5b1..8b35aa51b9bb 100644 > >>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c > >>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c > >>> @@ -72,7 +72,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > >>> if (ret != 2 + len) { > >>> em28xx_warn("failed to trigger write to i2c address 0x%x (error=%i)\n", > >>> addr, ret); > >>> - return (ret < 0) ? ret : -EIO; > >>> + return (ret < 0) ? ret : -EREMOTEIO; > >>> } > >>> /* wait for completion */ > >>> while (time_is_after_jiffies(timeout)) { > >>> @@ -91,7 +91,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > >>> msleep(5); > >>> } > >>> em28xx_warn("write to i2c device at 0x%x timed out\n", addr); > >>> - return -EIO; > >>> + return -EREMOTEIO; > >>> } > >>> > >>> /* > >>> @@ -115,7 +115,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > >>> if (ret != 2) { > >>> em28xx_warn("failed to trigger read from i2c address 0x%x (error=%i)\n", > >>> addr, ret); > >>> - return (ret < 0) ? ret : -EIO; > >>> + return (ret < 0) ? ret : -EREMOTEIO; > >>> } > >>> > >>> /* wait for completion */ > >>> @@ -142,7 +142,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) > >>> if (ret != len) { > >>> em28xx_warn("reading from i2c device at 0x%x failed: couldn't get the received message from the bridge (error=%i)\n", > >>> addr, ret); > >>> - return (ret < 0) ? ret : -EIO; > >>> + return (ret < 0) ? ret : -EREMOTEIO; > >>> } > >>> for (i = 0; i < len; i++) > >>> buf[i] = buf2[len - 1 - i]; > >>> @@ -162,7 +162,7 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr) > >>> ret = em2800_i2c_recv_bytes(dev, addr, &buf, 1); > >>> if (ret == 1) > >>> return 0; > >>> - return (ret < 0) ? ret : -EIO; > >>> + return (ret < 0) ? ret : -EREMOTEIO; > >>> } > >>> > >>> /* > >>> @@ -191,7 +191,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > >>> } else { > >>> em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", > >>> len, addr, ret); > >>> - return -EIO; > >>> + return -EREMOTEIO; > >>> } > >>> } > >>> > >>> @@ -219,7 +219,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > >>> } > >>> > >>> em28xx_warn("write to i2c device at 0x%x timed out\n", addr); > >>> - return -EIO; > >>> + return -EREMOTEIO; > >>> } > >>> > >>> /* > >>> @@ -268,7 +268,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len) > >>> } > >>> > >>> em28xx_warn("unknown i2c error (status=%i)\n", ret); > >>> - return -EIO; > >>> + return -EREMOTEIO; > >>> } > >>> > >>> /* > >>> @@ -283,7 +283,7 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) > >>> ret = em28xx_i2c_recv_bytes(dev, addr, &buf, 1); > >>> if (ret == 1) > >>> return 0; > >>> - return (ret < 0) ? ret : -EIO; > >>> + return (ret < 0) ? ret : -EREMOTEIO; > >>> } > >>> > >>> /* > >>> @@ -312,7 +312,7 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, > >>> } else { > >>> em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", > >>> len, addr, ret); > >>> - return -EIO; > >>> + return -EREMOTEIO; > >>> } > >>> } > >>> /* Check success */ > >> Why the hell -EREMOTEIO ??? > >> See Documentation/i2c/fault-codes. > >> It's not even listed there ! > >> What are you trying to fix here ? > > This is not a fixup patch. > > > > The idea of this path is to make em28xx more compliant with the Kernel > > error codes. > > > > As em28xx was the first USB media driver, it doesn't follow the best > > standards, despite all efforts we've made back in 2005, when the driver > > was merged, in order to follow the Kernel rules. We eventually fixed > > several things along the time, but we didn't took much care to fix the > > I2C error codes so far. > > > > Now that we're taking care of it, we should do it right. > > > > However, you're actually right here. > > > > Before 2011, all I2C drivers under drivers/i2c used to return EREMOTEIO, > > and the media drivers were moving to this error code since then. > Ok, that explains it. > > > But it seems that we missed a series of patches that moved I2C away from > > EREMOTEIO: > > http://www.spinics.net/lists/linux-i2c/msg06395.html > > https://www.mail-archive.com/linux-i2c@vger.kernel.org/msg04819.html > > > > I'll rewrite this patch to return the right error codes, according with > > Documentation/i2c/fault-codes. > > > > It seems that the proper return codes are: > > > > - ETIMEDOUT - when reg 05 returns 0x10 > I still don't agree here. > AFAICS an I2C timeout can only happen if clock stretching is used. It can also happen if the I2C gate is closed, for example, or when a device is not powered. This is likely the most common error when someone didn't properly mapped the GPIOs for switching to analog mode or to digital mode. > But we also get this when a slave device isn't present or doesn't > answer, which means it doesn't ACK the data. > So ETIMEDOUT is wrong. It is still a timeout: the device didn't answer in time. > Yes, 0x10 _could_ be a more general error. In that case EIO would be the > better choice. > But that's pure speculation as long as we don't have the datasheets. The datasheet will not tell what are the corresponding Linux error codes. ETIMEDOUT is better than the generic unspecific EIO. > After reading the i2c fault codes descriptions again, I would agree to > change it from ENODEV to ENXIO. > ENXIO seems to be the intended error code for NACK errors and it's a bit > more unspecific than ENODEV. > > Would that be acceptable for you ? Yes. > > - ENXIO - when the device is not temporarily not responding > > (e. g. reg 05 returning something not 0x10 or 0x00) > For those I would return just EIO. > I thought we agree here ? No. With em28xx/xc3028/tvp5150, we've got some temporary not responding errors with return code 0x02 or 0x04, before fixing that xc3028 power down bug. What I suspect is that codes 0x02 and 0x04 are related to I2C stretching, and that's why they need to be retried up to a software given timeout. I prefer to use EIO only when we got an error while writing to reg 0x04 or when the read operation didn't return the number of requested bytes. So, the better seems to return ETIMEDOUT for return codes different than 0x00/0x10/0x20. > > > - EBUSY - when reg 05 returns 0x20 on em2874 and upper [1] > Yes, that's ok > > > - EIO - for generic I/O errors that don't fit into the above. > I'm not sure what you mean with "generic", but yeah, that's ok, too. > > > [1] According with a post that Devin made on IRC sometime ago, bit 5 > > indicates that the I2C is busy when the master tries to use it, but it > > exists only on em2874 (and likely newer chips). > Can give me a pointer to this post ? Unfortunately, the IRC logs weren't working on May, 27. If you agree with this proposal, I'll rewrite https://patchwork.linuxtv.org/patch/21481/ accordingly. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 08.01.2014 12:55, schrieb Mauro Carvalho Chehab: > Em Tue, 07 Jan 2014 18:28:45 +0100 > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu: ... > >> But we also get this when a slave device isn't present or doesn't >> answer, which means it doesn't ACK the data. >> So ETIMEDOUT is wrong. > It is still a timeout: the device didn't answer in time. Ok, it's a 1 clock cycle timeout. :D :D :D > >> Yes, 0x10 _could_ be a more general error. In that case EIO would be the >> better choice. >> But that's pure speculation as long as we don't have the datasheets. > The datasheet will not tell what are the corresponding Linux error codes. Sure, we have to map the errors on our own. But the datasheet should tell us for which error condition the bridge returns which i2c state. > ETIMEDOUT is better than the generic unspecific EIO. The bridge should also be able to detect when something is fundamentally wrong with SCL/SDA lines (bus not ready). That's why I wouldn't return ETIMEDOUT for error codes we haven't seen so far. ETIMEDOUT for a clock stretching timeout (0x02 or 0x04) is of course right. > >> After reading the i2c fault codes descriptions again, I would agree to >> change it from ENODEV to ENXIO. >> ENXIO seems to be the intended error code for NACK errors and it's a bit >> more unspecific than ENODEV. >> >> Would that be acceptable for you ? > Yes. Ok. fine, then let's use ENXIO for 0x10. > >>> - ENXIO - when the device is not temporarily not responding >>> (e. g. reg 05 returning something not 0x10 or 0x00) >> For those I would return just EIO. >> I thought we agree here ? > No. With em28xx/xc3028/tvp5150, we've got some temporary not responding > errors with return code 0x02 or 0x04, before fixing that xc3028 power down > bug. > > What I suspect is that codes 0x02 and 0x04 are related to I2C stretching, > and that's why they need to be retried up to a software given timeout. Yeah, that's very likely. ETIMEDOUT is ok for a clock stretching timeout, but I wonder why there should be two of them. Can we narrow that down further ? If you are looping over reg 0x05 only (not the whole procedure) after a write for a very long time, which of these two states are temporary and which never change ? My theory would be, that one of them is temporary and means "busy, client holds down SCL" and the other one is final and means "bridge gave up waiting for the client to continue" (internal timeout). > I prefer to use EIO only when we got an error while writing to reg 0x04 > or when the read operation didn't return the number of requested bytes. The register read/write functions currently return the following error codes: device disconnected from USB port: -ENODEV len > URB_MAX_CTRL_SIZE: -EINVAL (Btw, this one should probably should be changed to a more appropriate error code) or the error codes from the USB transfer, filtered/maopped with usb_translate_errors(): -ENOMEM -ENODEV -EOPNOTSUPP -EIO I'm not sure if it is necessary to filter/map them further. With the exeception of -EINVAL, they could probably be passed to to the i2c client, too. OTOH I doubt than any i2c client implements such a detailed error handling. ;) I have no objections against just returning EIO. Regards, Frank -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c index 9fa7ed51e5b1..8b35aa51b9bb 100644 --- a/drivers/media/usb/em28xx/em28xx-i2c.c +++ b/drivers/media/usb/em28xx/em28xx-i2c.c @@ -72,7 +72,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) if (ret != 2 + len) { em28xx_warn("failed to trigger write to i2c address 0x%x (error=%i)\n", addr, ret); - return (ret < 0) ? ret : -EIO; + return (ret < 0) ? ret : -EREMOTEIO; } /* wait for completion */ while (time_is_after_jiffies(timeout)) { @@ -91,7 +91,7 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) msleep(5); } em28xx_warn("write to i2c device at 0x%x timed out\n", addr); - return -EIO; + return -EREMOTEIO; } /* @@ -115,7 +115,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) if (ret != 2) { em28xx_warn("failed to trigger read from i2c address 0x%x (error=%i)\n", addr, ret); - return (ret < 0) ? ret : -EIO; + return (ret < 0) ? ret : -EREMOTEIO; } /* wait for completion */ @@ -142,7 +142,7 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len) if (ret != len) { em28xx_warn("reading from i2c device at 0x%x failed: couldn't get the received message from the bridge (error=%i)\n", addr, ret); - return (ret < 0) ? ret : -EIO; + return (ret < 0) ? ret : -EREMOTEIO; } for (i = 0; i < len; i++) buf[i] = buf2[len - 1 - i]; @@ -162,7 +162,7 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr) ret = em2800_i2c_recv_bytes(dev, addr, &buf, 1); if (ret == 1) return 0; - return (ret < 0) ? ret : -EIO; + return (ret < 0) ? ret : -EREMOTEIO; } /* @@ -191,7 +191,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, } else { em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", len, addr, ret); - return -EIO; + return -EREMOTEIO; } } @@ -219,7 +219,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, } em28xx_warn("write to i2c device at 0x%x timed out\n", addr); - return -EIO; + return -EREMOTEIO; } /* @@ -268,7 +268,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len) } em28xx_warn("unknown i2c error (status=%i)\n", ret); - return -EIO; + return -EREMOTEIO; } /* @@ -283,7 +283,7 @@ static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr) ret = em28xx_i2c_recv_bytes(dev, addr, &buf, 1); if (ret == 1) return 0; - return (ret < 0) ? ret : -EIO; + return (ret < 0) ? ret : -EREMOTEIO; } /* @@ -312,7 +312,7 @@ static int em25xx_bus_B_send_bytes(struct em28xx *dev, u16 addr, u8 *buf, } else { em28xx_warn("%i bytes write to i2c device at 0x%x requested, but %i bytes written\n", len, addr, ret); - return -EIO; + return -EREMOTEIO; } } /* Check success */
The proper error code for I2C errors are EREMOTEIO. The em28xx driver is using EIO instead. Replace all occurrences of EIO at em28xx-i2c, in order to fix it. Signed-off-by: Mauro Carvalho Chehab <m.chehab@samsung.com> --- drivers/media/usb/em28xx/em28xx-i2c.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)