Re: [Qemu-devel] [PATCH v3 04/16] i2c: Don't check return value from i2c_recv()

[Corey Minyard <minyard@acm.org>]

On 11/30/18 11:25 AM, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 20:04, <minyard@acm.org> wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> i2c_recv() cannot fail, so there is no need to check the return
>> value.  It also returns unt8_t, so comparing with < 0 is not
>> meaningful.
>>
>> Fix up various I2C controllers to remove the unneeded code.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> diff --git a/hw/i2c/exynos4210_i2c.c b/hw/i2c/exynos4210_i2c.c
>> index c96fa7d7be..43f284eab7 100644
>> --- a/hw/i2c/exynos4210_i2c.c
>> +++ b/hw/i2c/exynos4210_i2c.c
>> @@ -106,12 +106,12 @@ static inline void exynos4210_i2c_raise_interrupt(Exynos4210I2CState *s)
>>   static void exynos4210_i2c_data_receive(void *opaque)
>>   {
>>       Exynos4210I2CState *s = (Exynos4210I2CState *)opaque;
>> -    int ret;
>> +    uint8_t ret;
>>
>>       s->i2cstat &= ~I2CSTAT_LAST_BIT;
>>       s->scl_free = false;
>>       ret = i2c_recv(s->bus);
>> -    if (ret < 0 && (s->i2ccon & I2CCON_ACK_GEN)) {
>> +    if (s->i2ccon & I2CCON_ACK_GEN) {
>>           s->i2cstat |= I2CSTAT_LAST_BIT;  /* Data is not acknowledged */
> Previously the code was doing this only if i2c_recv()
> failed (returned a negative value) and ACK_GEN was set.
> i2c_recv() can never fail, so this if() {} branch should
> be deleted entirely ("false && something" simplifies
> to "false", not "something").

Oops, yes.  Plus you can just remove the ret variable and simplify
this a lot.  Thanks for finding this.

-corey


>
>>       } else {
>>           s->i2cds = ret;
> The rest of the patch looks good.
>
> thanks
> -- PMM