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

[Corey Minyard]

On 11/30/18 11:25 AM, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 20:04, <address@hidden> wrote:
> > From: Corey Minyard <address@hidden>
> >
> > 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 <address@hidden>
> > Suggested-by: Peter Maydell <address@hidden>
> > ---
> > 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