[PATCH] i2c: mux: Fix error path in i2c-arb-gpio
There is no reason to use goto and just call return. Better is to call return directly which is done for some if/else parts.
Also make no sense to setup ret to -ETIMEDOUT and then to 0. Return timeout directly.
Signed-off-by: Michal Simek michal.simek@amd.com ---
drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c index a83d7cb0829d..3d2ce0ca705d 100644 --- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c +++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c @@ -54,7 +54,7 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, /* Indicate that we want to claim the bus */ ret = dm_gpio_set_value(&priv->ap_claim, 1); if (ret) - goto err; + return ret; udelay(priv->slew_delay_us);
/* Wait for the EC to release it */ @@ -62,7 +62,7 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, while (get_timer(start_retry) < priv->wait_retry_ms) { ret = dm_gpio_get_value(&priv->ec_claim); if (ret < 0) { - goto err; + return ret; } else if (!ret) { /* We got it, so return */ return 0; @@ -75,17 +75,14 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, /* It didn't release, so give up, wait, and try again */ ret = dm_gpio_set_value(&priv->ap_claim, 0); if (ret) - goto err; + return ret;
mdelay(priv->wait_retry_ms); } while (get_timer(start) < priv->wait_free_ms);
/* Give up, release our claim */ printf("I2C: Could not claim bus, timeout %lu\n", get_timer(start)); - ret = -ETIMEDOUT; - ret = 0; -err: - return ret; + return -ETIMEDOUT; }
static int i2c_arbitrator_probe(struct udevice *dev)
Hello Michal,
On 01.08.24 10:01, Michal Simek wrote:
There is no reason to use goto and just call return. Better is to call return directly which is done for some if/else parts.
Also make no sense to setup ret to -ETIMEDOUT and then to 0. Return timeout directly.
Signed-off-by: Michal Simek michal.simek@amd.com
drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c index a83d7cb0829d..3d2ce0ca705d 100644 --- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c +++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c @@ -54,7 +54,7 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, /* Indicate that we want to claim the bus */ ret = dm_gpio_set_value(&priv->ap_claim, 1); if (ret)
goto err;
return ret;udelay(priv->slew_delay_us);
/* Wait for the EC to release it */
@@ -62,7 +62,7 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, while (get_timer(start_retry) < priv->wait_retry_ms) { ret = dm_gpio_get_value(&priv->ec_claim); if (ret < 0) {
goto err;
return ret; } else if (!ret) { /* We got it, so return */ return 0;@@ -75,17 +75,14 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, /* It didn't release, so give up, wait, and try again */ ret = dm_gpio_set_value(&priv->ap_claim, 0); if (ret)
goto err;
return ret;mdelay(priv->wait_retry_ms); } while (get_timer(start) < priv->wait_free_ms);
/* Give up, release our claim */ printf("I2C: Could not claim bus, timeout %lu\n", get_timer(start));
- ret = -ETIMEDOUT;
- ret = 0;
I wonder here ...
It seems to me current driver returns 0 instead -ETIMEDOUT in this case?
So it is also a bugfix ... ?
-err:
- return ret;
return -ETIMEDOUT; }
static int i2c_arbitrator_probe(struct udevice *dev)
bye, Heiko
On 8/1/24 12:27, Heiko Schocher wrote:
Hello Michal,
On 01.08.24 10:01, Michal Simek wrote:
There is no reason to use goto and just call return. Better is to call return directly which is done for some if/else parts.
Also make no sense to setup ret to -ETIMEDOUT and then to 0. Return timeout directly.
Signed-off-by: Michal Simek michal.simek@amd.com
drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c index a83d7cb0829d..3d2ce0ca705d 100644 --- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c +++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c @@ -54,7 +54,7 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, /* Indicate that we want to claim the bus */ ret = dm_gpio_set_value(&priv->ap_claim, 1); if (ret) - goto err; + return ret; udelay(priv->slew_delay_us); /* Wait for the EC to release it */ @@ -62,7 +62,7 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, while (get_timer(start_retry) < priv->wait_retry_ms) { ret = dm_gpio_get_value(&priv->ec_claim); if (ret < 0) { - goto err; + return ret; } else if (!ret) { /* We got it, so return */ return 0; @@ -75,17 +75,14 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, /* It didn't release, so give up, wait, and try again */ ret = dm_gpio_set_value(&priv->ap_claim, 0); if (ret) - goto err; + return ret; mdelay(priv->wait_retry_ms); } while (get_timer(start) < priv->wait_free_ms); /* Give up, release our claim */ printf("I2C: Could not claim bus, timeout %lu\n", get_timer(start)); - ret = -ETIMEDOUT; - ret = 0;
I wonder here ...
It seems to me current driver returns 0 instead -ETIMEDOUT in this case?
So it is also a bugfix ... ?
Correct but no idea why. I haven't seen any description in commit message. And yes we can also add
Fixes: b725dc458f9d ("i2c: Add a mux for GPIO-based I2C bus arbitration")
Thanks, Michal
Hello Michal,
On 01.08.24 12:32, Michal Simek wrote:
On 8/1/24 12:27, Heiko Schocher wrote:
Hello Michal,
On 01.08.24 10:01, Michal Simek wrote:
There is no reason to use goto and just call return. Better is to call return directly which is done for some if/else parts.
Also make no sense to setup ret to -ETIMEDOUT and then to 0. Return timeout directly.
Signed-off-by: Michal Simek michal.simek@amd.com
drivers/i2c/muxes/i2c-arb-gpio-challenge.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c index a83d7cb0829d..3d2ce0ca705d 100644 --- a/drivers/i2c/muxes/i2c-arb-gpio-challenge.c +++ b/drivers/i2c/muxes/i2c-arb-gpio-challenge.c @@ -54,7 +54,7 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, /* Indicate that we want to claim the bus */ ret = dm_gpio_set_value(&priv->ap_claim, 1); if (ret) - goto err; + return ret; udelay(priv->slew_delay_us); /* Wait for the EC to release it */ @@ -62,7 +62,7 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, while (get_timer(start_retry) < priv->wait_retry_ms) { ret = dm_gpio_get_value(&priv->ec_claim); if (ret < 0) { - goto err; + return ret; } else if (!ret) { /* We got it, so return */ return 0; @@ -75,17 +75,14 @@ int i2c_arbitrator_select(struct udevice *mux, struct udevice *bus, /* It didn't release, so give up, wait, and try again */ ret = dm_gpio_set_value(&priv->ap_claim, 0); if (ret) - goto err; + return ret; mdelay(priv->wait_retry_ms); } while (get_timer(start) < priv->wait_free_ms); /* Give up, release our claim */ printf("I2C: Could not claim bus, timeout %lu\n", get_timer(start)); - ret = -ETIMEDOUT; - ret = 0;
I wonder here ...
It seems to me current driver returns 0 instead -ETIMEDOUT in this case?
So it is also a bugfix ... ?
Correct but no idea why. I haven't seen any description in commit message. And yes we can also add
Fixes: b725dc458f9d ("i2c: Add a mux for GPIO-based I2C bus arbitration")
Okay, I think to, obviously a fix:
Reviewed-by: Heiko Schocher hs@denx.de
Thanks!
bye, Heiko
participants (2)
-
Heiko Schocher -
Michal Simek