Re: [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv

[Corey Minyard]

On Mon, Jun 22, 2020 at 04:32:37PM -0500, Corey Minyard wrote:
> 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.

The more I think about it, the more I think it's a better idea to rename
the function.  Like i2c_send_or_recv(), which is a little more clear
about what it does.  Does that sound good?

-corey

> 
> -corey
> 
> > 
> > Signed-off-by: BALATON Zoltan <balaton@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
> >