On Sun, Jun 21, 2020 at 04:43:38PM +0200, BALATON Zoltan wrote: > These functions have a parameter that decides the direction of > transfer but totally confusingly they don't match but inverted sense. > To avoid frequent mistakes when using these functions change > i2c_send_recv to match i2c_start_transfer. Also use bool in > i2c_start_transfer instead of int to match i2c_send_recv.
Hmm, I have to admit that this is a little better. Indeed the hw/misc/auxbus.c looks suspicious. I can't imagine that code has ever been tested. I don't know the policy on changing an API like this with silent semantic changes. You've gotten all the internal ones; I'm wondering if we worry about silently breaking out of tree things. I'll pull this into my tree, but hopefully others will comment on this. -corey > > Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu> > --- > Looks like hw/misc/auxbus.c already got this wrong and calls both > i2c_start_transfer and i2c_send_recv with same is_write parameter. > Although the name of the is_write variable suggest this may need to be > inverted I'm not sure what that value actially means and which usage > was correct so I did not touch it. Someone knowing this device might > want to review and fix it. > > hw/display/sm501.c | 2 +- > hw/i2c/core.c | 34 +++++++++++++++++----------------- > hw/i2c/ppc4xx_i2c.c | 2 +- > include/hw/i2c/i2c.h | 4 ++-- > 4 files changed, 21 insertions(+), 21 deletions(-) > > diff --git a/hw/display/sm501.c b/hw/display/sm501.c > index 2db347dcbc..ccd0a6e376 100644 > --- a/hw/display/sm501.c > +++ b/hw/display/sm501.c > @@ -1034,7 +1034,7 @@ static void sm501_i2c_write(void *opaque, hwaddr addr, > uint64_t value, > int i; > for (i = 0; i <= s->i2c_byte_count; i++) { > res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i], > - !(s->i2c_addr & 1)); > + s->i2c_addr & 1); > if (res) { > s->i2c_status |= SM501_I2C_STATUS_ERROR; > return; > diff --git a/hw/i2c/core.c b/hw/i2c/core.c > index 1aac457a2a..c9d01df427 100644 > --- a/hw/i2c/core.c > +++ b/hw/i2c/core.c > @@ -91,7 +91,7 @@ int i2c_bus_busy(I2CBus *bus) > * without releasing the bus. If that fails, the bus is still > * in a transaction. > */ > -int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv) > +int i2c_start_transfer(I2CBus *bus, uint8_t address, bool recv) > { > BusChild *kid; > I2CSlaveClass *sc; > @@ -175,26 +175,14 @@ void i2c_end_transfer(I2CBus *bus) > bus->broadcast = false; > } > > -int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send) > +int i2c_send_recv(I2CBus *bus, uint8_t *data, bool recv) > { > I2CSlaveClass *sc; > I2CSlave *s; > I2CNode *node; > int ret = 0; > > - if (send) { > - QLIST_FOREACH(node, &bus->current_devs, next) { > - s = node->elt; > - sc = I2C_SLAVE_GET_CLASS(s); > - if (sc->send) { > - trace_i2c_send(s->address, *data); > - ret = ret || sc->send(s, *data); > - } else { > - ret = -1; > - } > - } > - return ret ? -1 : 0; > - } else { > + if (recv) { > ret = 0xff; > if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) { > sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt); > @@ -206,19 +194,31 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send) > } > *data = ret; > return 0; > + } else { > + QLIST_FOREACH(node, &bus->current_devs, next) { > + s = node->elt; > + sc = I2C_SLAVE_GET_CLASS(s); > + if (sc->send) { > + trace_i2c_send(s->address, *data); > + ret = ret || sc->send(s, *data); > + } else { > + ret = -1; > + } > + } > + return ret ? -1 : 0; > } > } > > int i2c_send(I2CBus *bus, uint8_t data) > { > - return i2c_send_recv(bus, &data, true); > + return i2c_send_recv(bus, &data, false); > } > > uint8_t i2c_recv(I2CBus *bus) > { > uint8_t data = 0xff; > > - i2c_send_recv(bus, &data, false); > + i2c_send_recv(bus, &data, true); > return data; > } > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c > index c0a8e04567..d3899203a4 100644 > --- a/hw/i2c/ppc4xx_i2c.c > +++ b/hw/i2c/ppc4xx_i2c.c > @@ -239,7 +239,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, > uint64_t value, > } > } > if (!(i2c->sts & IIC_STS_ERR) && > - i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) { > + i2c_send_recv(i2c->bus, &i2c->mdata[i], recv)) { > i2c->sts |= IIC_STS_ERR; > i2c->extsts |= IIC_EXTSTS_XFRA; > break; > diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h > index 4117211565..a09ab9230b 100644 > --- a/include/hw/i2c/i2c.h > +++ b/include/hw/i2c/i2c.h > @@ -72,10 +72,10 @@ struct I2CBus { > I2CBus *i2c_init_bus(DeviceState *parent, const char *name); > void i2c_set_slave_address(I2CSlave *dev, uint8_t address); > int i2c_bus_busy(I2CBus *bus); > -int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv); > +int i2c_start_transfer(I2CBus *bus, uint8_t address, bool recv); > void i2c_end_transfer(I2CBus *bus); > void i2c_nack(I2CBus *bus); > -int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send); > +int i2c_send_recv(I2CBus *bus, uint8_t *data, bool recv); > int i2c_send(I2CBus *bus, uint8_t data); > uint8_t i2c_recv(I2CBus *bus); > > -- > 2.21.3 >