| Message ID | 20181126200435.23408-5-minyard@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,01/16] i2c: Split smbus into parts | expand |
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"). > } else { > s->i2cds = ret; The rest of the patch looks good. thanks -- PMM
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
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index a2dfa82760..a085510cfd 100644 --- a/hw/i2c/aspeed_i2c.c +++ b/hw/i2c/aspeed_i2c.c @@ -189,16 +189,11 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus) static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus) { - int ret; + uint8_t ret; aspeed_i2c_set_state(bus, I2CD_MRXD); ret = i2c_recv(bus->bus); - if (ret < 0) { - qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__); - ret = 0xff; - } else { - bus->intr_status |= I2CD_INTR_RX_DONE; - } + bus->intr_status |= I2CD_INTR_RX_DONE; bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT; if (bus->cmd & I2CD_M_S_RX_CMD_LAST) { i2c_nack(bus->bus); 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 */ } else { s->i2cds = ret; diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c index 6c81b98ebd..6da5224e2e 100644 --- a/hw/i2c/imx_i2c.c +++ b/hw/i2c/imx_i2c.c @@ -120,7 +120,7 @@ static uint64_t imx_i2c_read(void *opaque, hwaddr offset, value = s->i2dr_read; if (imx_i2c_is_master(s)) { - int ret = 0xff; + uint8_t ret = 0xff; if (s->address == ADDR_RESET) { /* something is wrong as the address is not set */ @@ -133,15 +133,7 @@ static uint64_t imx_i2c_read(void *opaque, hwaddr offset, } else { /* get the next byte */ ret = i2c_recv(s->bus); - - if (ret >= 0) { - imx_i2c_raise_interrupt(s); - } else { - qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: read failed " - "for device 0x%02x\n", TYPE_IMX_I2C, - __func__, s->address); - ret = 0xff; - } + imx_i2c_raise_interrupt(s); } s->i2dr_read = ret;