* [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv @ 2020-06-21 14:43 BALATON Zoltan 2020-06-22 21:32 ` Corey Minyard 0 siblings, 1 reply; 7+ messages in thread From: BALATON Zoltan @ 2020-06-21 14:43 UTC (permalink / raw) To: qemu-devel; +Cc: Corey Minyard, KONRAD Frederic, David Gibson 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. 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv 2020-06-21 14:43 [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv BALATON Zoltan @ 2020-06-22 21:32 ` Corey Minyard 2020-06-23 2:06 ` Corey Minyard ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Corey Minyard @ 2020-06-22 21:32 UTC (permalink / raw) To: BALATON Zoltan; +Cc: KONRAD Frederic, qemu-devel, David Gibson 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv 2020-06-22 21:32 ` Corey Minyard @ 2020-06-23 2:06 ` Corey Minyard 2020-06-23 5:15 ` Philippe Mathieu-Daudé 2020-06-23 5:29 ` Philippe Mathieu-Daudé 2020-06-26 10:20 ` Fred Konrad 2 siblings, 1 reply; 7+ messages in thread From: Corey Minyard @ 2020-06-23 2:06 UTC (permalink / raw) To: BALATON Zoltan; +Cc: KONRAD Frederic, qemu-devel, David Gibson 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv 2020-06-23 2:06 ` Corey Minyard @ 2020-06-23 5:15 ` Philippe Mathieu-Daudé 2020-06-23 5:23 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-06-23 5:15 UTC (permalink / raw) To: cminyard, BALATON Zoltan; +Cc: KONRAD Frederic, qemu-devel, David Gibson 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 >>> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv 2020-06-23 5:15 ` Philippe Mathieu-Daudé @ 2020-06-23 5:23 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-06-23 5:23 UTC (permalink / raw) To: Corey Minyard, BALATON Zoltan Cc: KONRAD Frederic, qemu-devel@nongnu.org Developers, David Gibson On Tue, Jun 23, 2020 at 7:15 AM Philippe Mathieu-Daudé <f4bug@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 <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 > >>> > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv 2020-06-22 21:32 ` Corey Minyard 2020-06-23 2:06 ` Corey Minyard @ 2020-06-23 5:29 ` Philippe Mathieu-Daudé 2020-06-26 10:20 ` Fred Konrad 2 siblings, 0 replies; 7+ messages in thread From: Philippe Mathieu-Daudé @ 2020-06-23 5:29 UTC (permalink / raw) To: Corey Minyard Cc: KONRAD Frederic, David Gibson, qemu-devel@nongnu.org Developers On Mon, Jun 22, 2020 at 11:34 PM Corey Minyard <cminyard@mvista.com> 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. FYI this patch doesn't apply on current master: https://patchew.org/logs/20200621145235.9E241745712@zero.eik.bme.hu/git/ > > -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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv 2020-06-22 21:32 ` Corey Minyard 2020-06-23 2:06 ` Corey Minyard 2020-06-23 5:29 ` Philippe Mathieu-Daudé @ 2020-06-26 10:20 ` Fred Konrad 2 siblings, 0 replies; 7+ messages in thread From: Fred Konrad @ 2020-06-26 10:20 UTC (permalink / raw) To: cminyard; +Cc: Edgar E. Iglesias, David Gibson, qemu-devel Hi Corey, Le 6/22/20 à 11:32 PM, Corey Minyard a écrit : > 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. Sorry to hear that..., this auxbus stuff is related to the Xilinx Display Port device, so you may want to CC the MAINTAINERS of the Xilinx Display Port. Now about the testing, in theory this code is executed when the driver try to fetch the EDID from the i2c-ddc device which is connected to it, and you won't get any framebuffer with an empty EDID. But this was long ago and I don't have any image anymore to test that today. CC'ed Edgar, he can probably help with that. Regards, Fred > > 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 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-26 10:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-21 14:43 [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv BALATON Zoltan 2020-06-22 21:32 ` Corey Minyard 2020-06-23 2:06 ` Corey Minyard 2020-06-23 5:15 ` Philippe Mathieu-Daudé 2020-06-23 5:23 ` Philippe Mathieu-Daudé 2020-06-23 5:29 ` Philippe Mathieu-Daudé 2020-06-26 10:20 ` Fred Konrad
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.