[PATCH] i2c: mux: Fix error path in i2c-arb-gpio

[Heiko Schocher]

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 at 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 at denx.de>

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de