| Message ID | 20220324153200.8335-1-tharvey@gateworks.com |
|---|---|
| State | Accepted |
| Commit | 051df08fe072c3790c159d7b9966513325eb04ec |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | board: gw_ventana: gsc: fix GSC read/write functions | expand |
Hi Tim, On Thu, Mar 24, 2022 at 12:32 PM Tim Harvey <tharvey@gateworks.com> wrote: > > commit 7c84319af9c7 ("dm: gpio: Correct use of -ENODEV in drivers") > changed the return code for an I2C NAK from -ENODEV to -EREMOTEIO. I think we should be consistent with Linux and return -ENXIO for the NACK case: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/i2c/fault-codes.rst?h=v5.17#n88 "ENXIO Returned by I2C adapters to indicate that the address phase of a transfer didn't get an ACK. While it might just mean an I2C device was temporarily not responding, usually it means there's nothing listening at that address."
On Thu, Mar 24, 2022 at 8:59 AM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Tim, > > On Thu, Mar 24, 2022 at 12:32 PM Tim Harvey <tharvey@gateworks.com> wrote: > > > > commit 7c84319af9c7 ("dm: gpio: Correct use of -ENODEV in drivers") > > changed the return code for an I2C NAK from -ENODEV to -EREMOTEIO. > > I think we should be consistent with Linux and return -ENXIO for the NACK case: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/i2c/fault-codes.rst?h=v5.17#n88 > > "ENXIO > Returned by I2C adapters to indicate that the address phase > of a transfer didn't get an ACK. While it might just mean > an I2C device was temporarily not responding, usually it > means there's nothing listening at that address." Fabio, I share your sentiment however if I go and change this in drivers/i2c/mxc_i2c.c (or others) how do I know I don't cause the same breakage that Simon's patch did? I can't easily tell if anyone is depending on EREMOTEIO (or ENODEV as it was) to mean NAK. For reference there are about 48 i2c drivers in drivers/i2c and only 3 currently return -ENXIO (designware_i2c.c,i2c-microchip.c,sun8i_rsb.c) Honestly I would love to just implement retries on NAK in mxc_i2c.c but that approach was not accepted in the Linux driver when I attempted to do it there. Best Regards, Tim
On Thu, Mar 24, 2022 at 9:04 AM Tim Harvey <tharvey@gateworks.com> wrote: > > On Thu, Mar 24, 2022 at 8:59 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > Hi Tim, > > > > On Thu, Mar 24, 2022 at 12:32 PM Tim Harvey <tharvey@gateworks.com> wrote: > > > > > > commit 7c84319af9c7 ("dm: gpio: Correct use of -ENODEV in drivers") > > > changed the return code for an I2C NAK from -ENODEV to -EREMOTEIO. > > > > I think we should be consistent with Linux and return -ENXIO for the NACK case: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/i2c/fault-codes.rst?h=v5.17#n88 > > > > "ENXIO > > Returned by I2C adapters to indicate that the address phase > > of a transfer didn't get an ACK. While it might just mean > > an I2C device was temporarily not responding, usually it > > means there's nothing listening at that address." > > Fabio, > > I share your sentiment however if I go and change this in > drivers/i2c/mxc_i2c.c (or others) how do I know I don't cause the same > breakage that Simon's patch did? I can't easily tell if anyone is > depending on EREMOTEIO (or ENODEV as it was) to mean NAK. > > For reference there are about 48 i2c drivers in drivers/i2c and only > 3 currently return -ENXIO > (designware_i2c.c,i2c-microchip.c,sun8i_rsb.c) > > Honestly I would love to just implement retries on NAK in mxc_i2c.c > but that approach was not accepted in the Linux driver when I > attempted to do it there. > Any other feedback on this? Regardless of if I2C drivers should return the same error code as Linux on a NAK, I would like to get this patch applied to fix the current regression for the upcoming v2022.04. Best Regards, Tim
Hi Tim, On Mon, Mar 28, 2022 at 4:24 PM Tim Harvey <tharvey@gateworks.com> wrote: > Any other feedback on this? Regardless of if I2C drivers should return > the same error code as Linux on a NAK, I would like to get this patch > applied to fix the current regression for the upcoming v2022.04. Agreed, let's fix the regression for the upcoming release: Reviewed-by: Fabio Estevam <festevam@denx.de> The error code alignment between U-Boot and Linux can be discussed later.
On Mon, Mar 28, 2022 at 12:53 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Tim, > > On Mon, Mar 28, 2022 at 4:24 PM Tim Harvey <tharvey@gateworks.com> wrote: > > > Any other feedback on this? Regardless of if I2C drivers should return > > the same error code as Linux on a NAK, I would like to get this patch > > applied to fix the current regression for the upcoming v2022.04. > > Agreed, let's fix the regression for the upcoming release: > > Reviewed-by: Fabio Estevam <festevam@denx.de> > > The error code alignment between U-Boot and Linux can be discussed later. Tom, Could you pull this for the v2022.04 release? Best Regards, Tim
On Thu, Mar 24, 2022 at 08:32:00AM -0700, Tim Harvey wrote: > commit 7c84319af9c7 ("dm: gpio: Correct use of -ENODEV in drivers") > changed the return code for an I2C NAK from -ENODEV to -EREMOTEIO. > > Update the gsc_i2c_read and gsc_i2c_write functions for this change > to properly retry the transaction on a NAK meaning the GSC is busy. > > Fixes: 7c84319af9c7 ("dm: gpio: Correct use of -ENODEV in drivers") > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > Reviewed-by: Fabio Estevam <festevam@denx.de> Applied to u-boot/master, thanks!
diff --git a/board/gateworks/gw_ventana/gsc.c b/board/gateworks/gw_ventana/gsc.c index 324e5dbed2c8..a5d6de7e0e8e 100644 --- a/board/gateworks/gw_ventana/gsc.c +++ b/board/gateworks/gw_ventana/gsc.c @@ -41,7 +41,7 @@ int gsc_i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len) break; debug("%s: 0x%02x 0x%02x retry%d: %d\n", __func__, chip, addr, n, ret); - if (ret != -ENODEV) + if (ret != -EREMOTEIO) break; mdelay(10); } @@ -60,7 +60,7 @@ int gsc_i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len) break; debug("%s: 0x%02x 0x%02x retry%d: %d\n", __func__, chip, addr, n, ret); - if (ret != -ENODEV) + if (ret != -EREMOTEIO) break; mdelay(10); }
commit 7c84319af9c7 ("dm: gpio: Correct use of -ENODEV in drivers") changed the return code for an I2C NAK from -ENODEV to -EREMOTEIO. Update the gsc_i2c_read and gsc_i2c_write functions for this change to properly retry the transaction on a NAK meaning the GSC is busy. Fixes: 7c84319af9c7 ("dm: gpio: Correct use of -ENODEV in drivers") Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- board/gateworks/gw_ventana/gsc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)