On Tue, Jun 23, 2020 at 7:15 AM Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > 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);
Bah there is also: $ git grep -A1 -F _send_recv\( include include/hw/i2c/i2c.h:78:int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send); include/hw/i2c/i2c.h-79-int i2c_send(I2CBus *bus, uint8_t data); -- include/qemu-common.h:100:ssize_t qemu_co_send_recv(int sockfd, void *buf, size_t bytes, bool do_send); include/qemu-common.h-101-#define qemu_co_recv(sockfd, buf, bytes) \ include/qemu-common.h:102: qemu_co_send_recv(sockfd, buf, bytes, false) include/qemu-common.h-103-#define qemu_co_send(sockfd, buf, bytes) \ include/qemu-common.h:104: qemu_co_send_recv(sockfd, buf, bytes, true) include/qemu-common.h-105- -- include/qemu/iov.h:84: * r = iov_send_recv(sockfd, iov, iovcnt, offset, bytes, true); include/qemu/iov.h-85- * -- include/qemu/iov.h:93: * For iov_send_recv() _whole_ area being sent or received include/qemu/iov.h-94- * should be within the iovec, not only beginning of it. -- include/qemu/iov.h:96:ssize_t iov_send_recv(int sockfd, const struct iovec *iov, unsigned iov_cnt, include/qemu/iov.h-97- size_t offset, size_t bytes, bool do_send); -- include/qemu/iov.h:99: iov_send_recv(sockfd, iov, iov_cnt, offset, bytes, false) include/qemu/iov.h-100-#define iov_send(sockfd, iov, iov_cnt, offset, bytes) \ include/qemu/iov.h:101: iov_send_recv(sockfd, iov, iov_cnt, offset, bytes, true) include/qemu/iov.h-102- Maybe i2c_transact(I2CBus *bus, uint8_t *data, bool is_send)? > 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 > >>> > > >