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

[Philippe Mathieu-Daudé]

On 6/23/20 4:06 AM, Corey Minyard wrote:
> 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?

Or to match the common pattern used in QEMU:

  int i2c_rw(I2CBus *bus, uint8_t *data, bool is_write);

Or

  int i2c_bus_rw(I2CBus *bus, uint8_t *data, bool is_write);

See:

$ git grep -A1 -F _rw\( include
include/exec/cpu-common.h:69:void cpu_physical_memory_rw(hwaddr addr,
void *buf,
include/exec/cpu-common.h-70-                            hwaddr len,
bool is_write);
--
include/exec/cpu-common.h:74:    cpu_physical_memory_rw(addr, buf, len,
false);
include/exec/cpu-common.h-75-}
--
include/exec/cpu-common.h:79:    cpu_physical_memory_rw(addr, (void
*)buf, len, true);
include/exec/cpu-common.h-80-}
--
include/exec/memory.h:2059:MemTxResult address_space_rw(AddressSpace
*as, hwaddr addr,
include/exec/memory.h-2060-                             MemTxAttrs
attrs, void *buf,
--
include/hw/pci/pci.h:786:static inline int pci_dma_rw(PCIDevice *dev,
dma_addr_t addr,
include/hw/pci/pci.h-787-                             void *buf,
dma_addr_t len, DMADirection dir)
--
include/hw/pci/pci.h:789:    dma_memory_rw(pci_get_address_space(dev),
addr, buf, len, dir);
include/hw/pci/pci.h-790-    return 0;
--
include/hw/pci/pci.h:796:    return pci_dma_rw(dev, addr, buf, len,
DMA_DIRECTION_TO_DEVICE);
include/hw/pci/pci.h-797-}
--
include/hw/pci/pci.h:802:    return pci_dma_rw(dev, addr, (void *) buf,
len, DMA_DIRECTION_FROM_DEVICE);
include/hw/pci/pci.h-803-}
--
include/hw/ppc/spapr_xive.h:86:uint64_t kvmppc_xive_esb_rw(XiveSource
*xsrc, int srcno, uint32_t offset,
include/hw/ppc/spapr_xive.h-87-                            uint64_t
data, bool write);
--
include/sysemu/dma.h:87:    return (bool)address_space_rw(as, addr,
MEMTXATTRS_UNSPECIFIED,
include/sysemu/dma.h-88-                                  buf, len, dir
== DMA_DIRECTION_FROM_DEVICE);
--
include/sysemu/dma.h:104:static inline int dma_memory_rw(AddressSpace
*as, dma_addr_t addr,
include/sysemu/dma.h-105-                                void *buf,
dma_addr_t len,
--
include/sysemu/dma.h:116:    return dma_memory_rw(as, addr, buf, len,
DMA_DIRECTION_TO_DEVICE);
include/sysemu/dma.h-117-}
--
include/sysemu/dma.h:122:    return dma_memory_rw(as, addr, (void *)buf,
len,
include/sysemu/dma.h-123-
DMA_DIRECTION_FROM_DEVICE);

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