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

[Corey Minyard]

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 <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
>