qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Fred Konrad
Subject: Re: [PATCH] i2c: Match parameters of i2c_start_transfer and i2c_send_recv
Date: Fri, 26 Jun 2020 12:20:49 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.9.0 

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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]