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