[PATCH v3 00/13] hw/i2c: Remove confusing i2c_send_recv() API

[PATCH v3 00/13] hw/i2c: Remove confusing i2c_send_recv() API

[Philippe Mathieu-Daudé]

This is a respin of Zoltan's patch:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg714711.html

Since v2, tried to address Corey's review comments resulting
in a i2c_send_recv() removal and code easier to review (to my
taste at least).

Supersedes: <20200623063123.20776-1-f4bug@amsat.org>

BALATON Zoltan (1):
  hw/i2c: Make i2c_start_transfer() direction argument a boolean

Philippe Mathieu-Daudé (12):
  hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h"
  hw/input/lm832x: Define TYPE_LM8323 in public header
  hw/display/sm501: Simplify sm501_i2c_write() logic
  hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send()
  hw/i2c/ppc4xx_i2c: Add reference to datasheet
  hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()
  hw/misc/auxbus: Fix MOT/classic I2C mode
  hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases
  hw/misc/auxbus: Replace 'is_write' boolean by its value
  hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()
  hw/i2c: Remove confusing i2c_send_recv()
  hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address()

 include/hw/i2c/i2c.h      | 21 +++++++++---
 include/hw/input/lm832x.h | 28 ++++++++++++++++
 hw/arm/nseries.c          |  3 +-
 hw/arm/pxa2xx.c           |  2 +-
 hw/arm/spitz.c            |  4 +--
 hw/display/ati.c          |  2 +-
 hw/display/sm501.c        | 16 +++++----
 hw/display/xlnx_dp.c      |  2 +-
 hw/i2c/core.c             | 56 ++++++++++++++-----------------
 hw/i2c/imx_i2c.c          |  2 +-
 hw/i2c/ppc4xx_i2c.c       | 15 ++++++---
 hw/input/lm832x.c         |  2 +-
 hw/misc/auxbus.c          | 69 +++++++++++++++++++++++++++++----------
 MAINTAINERS               |  1 +
 14 files changed, 149 insertions(+), 74 deletions(-)
 create mode 100644 include/hw/input/lm832x.h

-- 
2.31.1

[PATCH v3 01/13] hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h"

[Philippe Mathieu-Daudé]

lm832x_key_event() is specific go LM832x devices, not to the
I2C bus API. Move it out of "i2c.h" to a new header.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/i2c/i2c.h      |  3 ---
 include/hw/input/lm832x.h | 26 ++++++++++++++++++++++++++
 hw/arm/nseries.c          |  1 +
 hw/input/lm832x.c         |  1 +
 MAINTAINERS               |  1 +
 5 files changed, 29 insertions(+), 3 deletions(-)
 create mode 100644 include/hw/input/lm832x.h

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index ff4129ea704..850815e707c 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -142,9 +142,6 @@ I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);
  */
 bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
 
-/* lm832x.c */
-void lm832x_key_event(DeviceState *dev, int key, int state);
-
 extern const VMStateDescription vmstate_i2c_slave;
 
 #define VMSTATE_I2C_SLAVE(_field, _state) {                          \
diff --git a/include/hw/input/lm832x.h b/include/hw/input/lm832x.h
new file mode 100644
index 00000000000..8e21fb76452
--- /dev/null
+++ b/include/hw/input/lm832x.h
@@ -0,0 +1,26 @@
+/*
+ * National Semiconductor LM8322/8323 GPIO keyboard & PWM chips.
+ *
+ * Copyright (C) 2008 Nokia Corporation
+ * Written by Andrzej Zaborowski <andrew@openedhand.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) version 3 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_INPUT_LM832X
+#define HW_INPUT_L
+
+void lm832x_key_event(DeviceState *dev, int key, int state);
+
+#endif
diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 0aefa5d0f3e..7b82b8682e8 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -34,6 +34,7 @@
 #include "hw/boards.h"
 #include "hw/i2c/i2c.h"
 #include "hw/display/blizzard.h"
+#include "hw/input/lm832x.h"
 #include "hw/input/tsc2xxx.h"
 #include "hw/misc/cbus.h"
 #include "hw/misc/tmp105.h"
diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index 4cb1e9de01f..d2b92b457e3 100644
--- a/hw/input/lm832x.c
+++ b/hw/input/lm832x.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/input/lm832x.h"
 #include "hw/i2c/i2c.h"
 #include "hw/irq.h"
 #include "migration/vmstate.h"
diff --git a/MAINTAINERS b/MAINTAINERS
index 636bf2f5365..c56bc112ccf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -792,6 +792,7 @@ F: hw/input/tsc2005.c
 F: hw/misc/cbus.c
 F: hw/rtc/twl92230.c
 F: include/hw/display/blizzard.h
+F: include/hw/input/lm832x.h
 F: include/hw/input/tsc2xxx.h
 F: include/hw/misc/cbus.h
 F: tests/acceptance/machine_arm_n8x0.py
-- 
2.31.1

[PATCH v3 02/13] hw/input/lm832x: Define TYPE_LM8323 in public header

[Philippe Mathieu-Daudé]

Define TYPE_LM8323 in the public "hw/input/lm832x.h"
header and use it in hw/arm/nseries.c.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/input/lm832x.h | 4 +++-
 hw/arm/nseries.c          | 2 +-
 hw/input/lm832x.c         | 1 -
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/hw/input/lm832x.h b/include/hw/input/lm832x.h
index 8e21fb76452..2a58ccf8916 100644
--- a/include/hw/input/lm832x.h
+++ b/include/hw/input/lm832x.h
@@ -19,7 +19,9 @@
  */
 
 #ifndef HW_INPUT_LM832X
-#define HW_INPUT_L
+#define HW_INPUT_LM832X
+
+#define TYPE_LM8323 "lm8323"
 
 void lm832x_key_event(DeviceState *dev, int key, int state);
 
diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 7b82b8682e8..3a51264e3cf 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -417,7 +417,7 @@ static void n810_kbd_setup(struct n800_s *s)
     /* Attach the LM8322 keyboard to the I2C bus,
      * should happen in n8x0_i2c_setup and s->kbd be initialised here.  */
     s->kbd = DEVICE(i2c_slave_create_simple(omap_i2c_bus(s->mpu->i2c[0]),
-                                            "lm8323", N810_LM8323_ADDR));
+                                            TYPE_LM8323, N810_LM8323_ADDR));
     qdev_connect_gpio_out(s->kbd, 0, kbd_irq);
 }
 
diff --git a/hw/input/lm832x.c b/hw/input/lm832x.c
index d2b92b457e3..19a646d9bb4 100644
--- a/hw/input/lm832x.c
+++ b/hw/input/lm832x.c
@@ -28,7 +28,6 @@
 #include "ui/console.h"
 #include "qom/object.h"
 
-#define TYPE_LM8323 "lm8323"
 OBJECT_DECLARE_SIMPLE_TYPE(LM823KbdState, LM8323)
 
 struct LM823KbdState {
-- 
2.31.1

[PATCH v3 03/13] hw/display/sm501: Simplify sm501_i2c_write() logic

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/sm501.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 8789722ef27..f276276f7f1 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1036,8 +1036,9 @@ static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
                 int res = i2c_start_transfer(s->i2c_bus,
                                              s->i2c_addr >> 1,
                                              s->i2c_addr & 1);
-                s->i2c_status |= (res ? SM501_I2C_STATUS_ERROR : 0);
-                if (!res) {
+                if (res) {
+                    s->i2c_status |= SM501_I2C_STATUS_ERROR;
+                } else {
                     int i;
                     for (i = 0; i <= s->i2c_byte_count; i++) {
                         res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
-- 
2.31.1

[PATCH v3 04/13] hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Philippe Mathieu-Daudé]

Instead of using the confuse i2c_send_recv(), rewrite to directly
call i2c_recv() & i2c_send(), resulting in code easire to review.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/sm501.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index f276276f7f1..569661a0746 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1033,17 +1033,18 @@ static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
     case SM501_I2C_CONTROL:
         if (value & SM501_I2C_CONTROL_ENABLE) {
             if (value & SM501_I2C_CONTROL_START) {
+                bool is_recv = s->i2c_addr & 1;
                 int res = i2c_start_transfer(s->i2c_bus,
                                              s->i2c_addr >> 1,
-                                             s->i2c_addr & 1);
+                                             is_recv);
                 if (res) {
                     s->i2c_status |= SM501_I2C_STATUS_ERROR;
                 } else {
                     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));
-                        if (res) {
+                        if (is_recv) {
+                            s->i2c_data[i] = i2c_recv(s->i2c_bus);
+                        } else if (i2c_send(s->i2c_bus, s->i2c_data[i]) < 0) {
                             s->i2c_status |= SM501_I2C_STATUS_ERROR;
                             return;
                         }
-- 
2.31.1

[PATCH v3 05/13] hw/i2c/ppc4xx_i2c: Add reference to datasheet

[Philippe Mathieu-Daudé]

It took me a while to find this model datasheet, since it is
an OCR scan. Add a reference to save other developers time.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i2c/ppc4xx_i2c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index c0a8e045670..f4c5bc12d36 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -1,6 +1,8 @@
 /*
  * PPC4xx I2C controller emulation
  *
+ * Documentation: PPC405GP User's Manual, Chapter 22. IIC Bus Interface
+ *
  * Copyright (c) 2007 Jocelyn Mayer
  * Copyright (c) 2012 François Revol
  * Copyright (c) 2016-2018 BALATON Zoltan
-- 
2.31.1

[PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Philippe Mathieu-Daudé]

Instead of using the confuse i2c_send_recv(), rewrite to directly
call i2c_recv() & i2c_send(), resulting in code easire to review.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index f4c5bc12d36..b3d3da56e38 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
                         i2c->sts &= ~IIC_STS_ERR;
                     }
                 }
-                if (!(i2c->sts & IIC_STS_ERR) &&
-                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
-                    i2c->sts |= IIC_STS_ERR;
-                    i2c->extsts |= IIC_EXTSTS_XFRA;
-                    break;
+                if (!(i2c->sts & IIC_STS_ERR)) {
+                    if (recv) {
+                        i2c->mdata[i] = i2c_recv(i2c->bus);
+                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {
+                        i2c->sts |= IIC_STS_ERR;
+                        i2c->extsts |= IIC_EXTSTS_XFRA;
+                        break;
+                    }
                 }
                 if (value & IIC_CNTL_RPST || !(value & IIC_CNTL_CHT)) {
                     i2c_end_transfer(i2c->bus);
-- 
2.31.1

[PATCH v3 07/13] hw/misc/auxbus: Fix MOT/classic I2C mode

[Philippe Mathieu-Daudé]

Since its introduction in commit 6fc7f77fd2 i2c_start_transfer()
uses incorrectly the direction of the transfer (the last argument
is called 'is_recv'). Fix by inverting the argument, we now have
is_recv = !is_write.

Fixes: 6fc7f77fd2 ("introduce aux-bus")
Reported-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/auxbus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 6c099ae2a2d..148b070ce4a 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -139,7 +139,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
             i2c_end_transfer(i2c_bus);
         }
 
-        if (i2c_start_transfer(i2c_bus, address, is_write)) {
+        if (i2c_start_transfer(i2c_bus, address, !is_write)) {
             ret = AUX_I2C_NACK;
             break;
         }
@@ -170,7 +170,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
             /*
              * No transactions started..
              */
-            if (i2c_start_transfer(i2c_bus, address, is_write)) {
+            if (i2c_start_transfer(i2c_bus, address, !is_write)) {
                 break;
             }
         } else if ((address != bus->last_i2c_address) ||
@@ -179,7 +179,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
              * Transaction started but we need to restart..
              */
             i2c_end_transfer(i2c_bus);
-            if (i2c_start_transfer(i2c_bus, address, is_write)) {
+            if (i2c_start_transfer(i2c_bus, address, !is_write)) {
                 break;
             }
         }
-- 
2.31.1

[PATCH v3 08/13] hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases

[Philippe Mathieu-Daudé]

To allow further simplifications in the following commits,
start copying WRITE_I2C code to the READ_I2C, and READ_I2C_MOT
to WRITE_I2C_MOT. No logical change.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/auxbus.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 148b070ce4a..9cc9cf3be32 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -133,6 +133,26 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
      * Classic I2C transactions..
      */
     case READ_I2C:
+        is_write = cmd == READ_I2C ? false : true;
+        if (i2c_bus_busy(i2c_bus)) {
+            i2c_end_transfer(i2c_bus);
+        }
+
+        if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+            ret = AUX_I2C_NACK;
+            break;
+        }
+
+        ret = AUX_I2C_ACK;
+        while (len > 0) {
+            if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+                ret = AUX_I2C_NACK;
+                break;
+            }
+            len--;
+        }
+        i2c_end_transfer(i2c_bus);
+        break;
     case WRITE_I2C:
         is_write = cmd == READ_I2C ? false : true;
         if (i2c_bus_busy(i2c_bus)) {
@@ -163,6 +183,39 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
      *  - We changed the address.
      */
     case WRITE_I2C_MOT:
+        is_write = cmd == READ_I2C_MOT ? false : true;
+        ret = AUX_I2C_NACK;
+        if (!i2c_bus_busy(i2c_bus)) {
+            /*
+             * No transactions started..
+             */
+            if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+                break;
+            }
+        } else if ((address != bus->last_i2c_address) ||
+                   (bus->last_transaction != cmd)) {
+            /*
+             * Transaction started but we need to restart..
+             */
+            i2c_end_transfer(i2c_bus);
+            if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+                break;
+            }
+        }
+
+        bus->last_transaction = cmd;
+        bus->last_i2c_address = address;
+        while (len > 0) {
+            if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+                i2c_end_transfer(i2c_bus);
+                break;
+            }
+            len--;
+        }
+        if (len == 0) {
+            ret = AUX_I2C_ACK;
+        }
+        break;
     case READ_I2C_MOT:
         is_write = cmd == READ_I2C_MOT ? false : true;
         ret = AUX_I2C_NACK;
-- 
2.31.1

[PATCH v3 09/13] hw/misc/auxbus: Replace 'is_write' boolean by its value

[Philippe Mathieu-Daudé]

Remove the 'is_write' boolean by directly using its value in place.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/auxbus.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 9cc9cf3be32..d96219aef88 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -106,7 +106,6 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
     AUXReply ret = AUX_NACK;
     I2CBus *i2c_bus = aux_get_i2c_bus(bus);
     size_t i;
-    bool is_write = false;
 
     DPRINTF("request at address 0x%" PRIX32 ", command %u, len %u\n", address,
             cmd, len);
@@ -117,11 +116,10 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
      */
     case WRITE_AUX:
     case READ_AUX:
-        is_write = cmd == READ_AUX ? false : true;
         for (i = 0; i < len; i++) {
             if (!address_space_rw(&bus->aux_addr_space, address++,
                                   MEMTXATTRS_UNSPECIFIED, data++, 1,
-                                  is_write)) {
+                                  cmd == WRITE_AUX)) {
                 ret = AUX_I2C_ACK;
             } else {
                 ret = AUX_NACK;
@@ -133,19 +131,18 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
      * Classic I2C transactions..
      */
     case READ_I2C:
-        is_write = cmd == READ_I2C ? false : true;
         if (i2c_bus_busy(i2c_bus)) {
             i2c_end_transfer(i2c_bus);
         }
 
-        if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+        if (i2c_start_transfer(i2c_bus, address, true)) {
             ret = AUX_I2C_NACK;
             break;
         }
 
         ret = AUX_I2C_ACK;
         while (len > 0) {
-            if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+            if (i2c_send_recv(i2c_bus, data++, false) < 0) {
                 ret = AUX_I2C_NACK;
                 break;
             }
@@ -154,19 +151,18 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
         i2c_end_transfer(i2c_bus);
         break;
     case WRITE_I2C:
-        is_write = cmd == READ_I2C ? false : true;
         if (i2c_bus_busy(i2c_bus)) {
             i2c_end_transfer(i2c_bus);
         }
 
-        if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+        if (i2c_start_transfer(i2c_bus, address, false)) {
             ret = AUX_I2C_NACK;
             break;
         }
 
         ret = AUX_I2C_ACK;
         while (len > 0) {
-            if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
                 ret = AUX_I2C_NACK;
                 break;
             }
@@ -183,13 +179,12 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
      *  - We changed the address.
      */
     case WRITE_I2C_MOT:
-        is_write = cmd == READ_I2C_MOT ? false : true;
         ret = AUX_I2C_NACK;
         if (!i2c_bus_busy(i2c_bus)) {
             /*
              * No transactions started..
              */
-            if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+            if (i2c_start_transfer(i2c_bus, address, false)) {
                 break;
             }
         } else if ((address != bus->last_i2c_address) ||
@@ -198,7 +193,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
              * Transaction started but we need to restart..
              */
             i2c_end_transfer(i2c_bus);
-            if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+            if (i2c_start_transfer(i2c_bus, address, false)) {
                 break;
             }
         }
@@ -206,7 +201,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
         bus->last_transaction = cmd;
         bus->last_i2c_address = address;
         while (len > 0) {
-            if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
                 i2c_end_transfer(i2c_bus);
                 break;
             }
@@ -217,13 +212,12 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
         }
         break;
     case READ_I2C_MOT:
-        is_write = cmd == READ_I2C_MOT ? false : true;
         ret = AUX_I2C_NACK;
         if (!i2c_bus_busy(i2c_bus)) {
             /*
              * No transactions started..
              */
-            if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+            if (i2c_start_transfer(i2c_bus, address, true)) {
                 break;
             }
         } else if ((address != bus->last_i2c_address) ||
@@ -232,7 +226,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
              * Transaction started but we need to restart..
              */
             i2c_end_transfer(i2c_bus);
-            if (i2c_start_transfer(i2c_bus, address, !is_write)) {
+            if (i2c_start_transfer(i2c_bus, address, true)) {
                 break;
             }
         }
@@ -240,7 +234,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
         bus->last_transaction = cmd;
         bus->last_i2c_address = address;
         while (len > 0) {
-            if (i2c_send_recv(i2c_bus, data++, is_write) < 0) {
+            if (i2c_send_recv(i2c_bus, data++, false) < 0) {
                 i2c_end_transfer(i2c_bus);
                 break;
             }
-- 
2.31.1

[PATCH v3 10/13] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Philippe Mathieu-Daudé]

Instead of using the confuse i2c_send_recv(), replace
  i2c_send_recv(send = true) by i2c_send() and
  i2c_send_recv(send = false) by i2c_recv().
During the replacement we also change a while() statement by for().
The resulting code is easier to review.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/misc/auxbus.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index d96219aef88..4d270ea9ec3 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -141,12 +141,8 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
         }
 
         ret = AUX_I2C_ACK;
-        while (len > 0) {
-            if (i2c_send_recv(i2c_bus, data++, false) < 0) {
-                ret = AUX_I2C_NACK;
-                break;
-            }
-            len--;
+        for (i = 0; i < len; i++) {
+            data[i] = i2c_recv(i2c_bus);
         }
         i2c_end_transfer(i2c_bus);
         break;
@@ -161,12 +157,11 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
         }
 
         ret = AUX_I2C_ACK;
-        while (len > 0) {
-            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
+        for (i = 0; i < len; i++) {
+            if (i2c_send(i2c_bus, data[i]) < 0) {
                 ret = AUX_I2C_NACK;
                 break;
             }
-            len--;
         }
         i2c_end_transfer(i2c_bus);
         break;
@@ -200,14 +195,13 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
 
         bus->last_transaction = cmd;
         bus->last_i2c_address = address;
-        while (len > 0) {
-            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
+        for (i = 0; i < len; i++) {
+            if (i2c_send(i2c_bus, data[i]) < 0) {
                 i2c_end_transfer(i2c_bus);
                 break;
             }
-            len--;
         }
-        if (len == 0) {
+        if (i == len) {
             ret = AUX_I2C_ACK;
         }
         break;
@@ -233,16 +227,10 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
 
         bus->last_transaction = cmd;
         bus->last_i2c_address = address;
-        while (len > 0) {
-            if (i2c_send_recv(i2c_bus, data++, false) < 0) {
-                i2c_end_transfer(i2c_bus);
-                break;
-            }
-            len--;
-        }
-        if (len == 0) {
-            ret = AUX_I2C_ACK;
+        for (i = 0; i < len; i++) {
+            data[i] = i2c_recv(i2c_bus);
         }
+        ret = AUX_I2C_ACK;
         break;
     default:
         qemu_log_mask(LOG_UNIMP, "AUX cmd=%u not implemented\n", cmd);
-- 
2.31.1

[PATCH v3 11/13] hw/i2c: Remove confusing i2c_send_recv()

[Philippe Mathieu-Daudé]

We replaced all the i2c_send_recv() calls by the clearer i2c_recv()
and i2c_send(), so we can remove this confuse API.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/i2c/i2c.h |  1 -
 hw/i2c/core.c        | 50 +++++++++++++++++++-------------------------
 2 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 850815e707c..99635b837a5 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -84,7 +84,6 @@ int i2c_bus_busy(I2CBus *bus);
 int i2c_start_transfer(I2CBus *bus, uint8_t address, int 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(I2CBus *bus, uint8_t data);
 uint8_t i2c_recv(I2CBus *bus);
 bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 3a7bae311df..27a66df7f34 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -188,50 +188,42 @@ void i2c_end_transfer(I2CBus *bus)
     bus->broadcast = false;
 }
 
-int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
+int i2c_send(I2CBus *bus, uint8_t data)
 {
     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;
-            }
+    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 {
-        ret = 0xff;
-        if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
-            sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
-            if (sc->recv) {
-                s = QLIST_FIRST(&bus->current_devs)->elt;
-                ret = sc->recv(s);
-                trace_i2c_recv(s->address, ret);
-            }
-        }
-        *data = ret;
-        return 0;
     }
-}
 
-int i2c_send(I2CBus *bus, uint8_t data)
-{
-    return i2c_send_recv(bus, &data, true);
+    return ret ? -1 : 0;
 }
 
 uint8_t i2c_recv(I2CBus *bus)
 {
     uint8_t data = 0xff;
+    I2CSlaveClass *sc;
+    I2CSlave *s;
+
+    if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
+        sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
+        if (sc->recv) {
+            s = QLIST_FIRST(&bus->current_devs)->elt;
+            data = sc->recv(s);
+            trace_i2c_recv(s->address, data);
+        }
+    }
 
-    i2c_send_recv(bus, &data, false);
     return data;
 }
 
-- 
2.31.1

[PATCH v3 12/13] hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address()

[Philippe Mathieu-Daudé]

Other functions from I2C slave API are named "i2c_slave_XXX()".
Follow that pattern with set_address(). Add docstring along.
No logical change.

Patch created mechanically using:

  $ sed -i s/i2c_set_slave_address/i2c_slave_set_address/ \
    $(git grep -l i2c_set_slave_address)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/i2c/i2c.h | 8 +++++++-
 hw/arm/pxa2xx.c      | 2 +-
 hw/arm/spitz.c       | 4 ++--
 hw/display/ati.c     | 2 +-
 hw/display/sm501.c   | 2 +-
 hw/display/xlnx_dp.c | 2 +-
 hw/i2c/core.c        | 2 +-
 hw/i2c/imx_i2c.c     | 2 +-
 8 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 99635b837a5..2adf521b271 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -79,7 +79,6 @@ 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);
 void i2c_end_transfer(I2CBus *bus);
@@ -141,6 +140,13 @@ I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);
  */
 bool i2c_slave_realize_and_unref(I2CSlave *dev, I2CBus *bus, Error **errp);
 
+/**
+ * Set the I2C bus address of a slave device
+ * @dev: I2C slave device
+ * @address: I2C address of the slave when put on a bus
+ */
+void i2c_slave_set_address(I2CSlave *dev, uint8_t address);
+
 extern const VMStateDescription vmstate_i2c_slave;
 
 #define VMSTATE_I2C_SLAVE(_field, _state) {                          \
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index fdc4955e95b..15a247efae2 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1437,7 +1437,7 @@ static void pxa2xx_i2c_write(void *opaque, hwaddr addr,
         break;
 
     case ISAR:
-        i2c_set_slave_address(I2C_SLAVE(s->slave), value & 0x7f);
+        i2c_slave_set_address(I2C_SLAVE(s->slave), value & 0x7f);
         break;
 
     case IDBR:
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index b45a929cbd9..c0f0f8193f4 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -769,9 +769,9 @@ static void spitz_wm8750_addr(void *opaque, int line, int level)
 {
     I2CSlave *wm = (I2CSlave *) opaque;
     if (level)
-        i2c_set_slave_address(wm, SPITZ_WM_ADDRH);
+        i2c_slave_set_address(wm, SPITZ_WM_ADDRH);
     else
-        i2c_set_slave_address(wm, SPITZ_WM_ADDRL);
+        i2c_slave_set_address(wm, SPITZ_WM_ADDRL);
 }
 
 static void spitz_i2c_setup(PXA2xxState *cpu)
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 4c3ad8f47b0..31f22754dce 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -968,7 +968,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
     I2CBus *i2cbus = i2c_init_bus(DEVICE(s), "ati-vga.ddc");
     bitbang_i2c_init(&s->bbi2c, i2cbus);
     I2CSlave *i2cddc = I2C_SLAVE(qdev_new(TYPE_I2CDDC));
-    i2c_set_slave_address(i2cddc, 0x50);
+    i2c_slave_set_address(i2cddc, 0x50);
     qdev_realize_and_unref(DEVICE(i2cddc), BUS(i2cbus), &error_abort);
 
     /* mmio register space */
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 569661a0746..663c37e7f28 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1828,7 +1828,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
     s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
     /* ddc */
     I2CDDCState *ddc = I2CDDC(qdev_new(TYPE_I2CDDC));
-    i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
+    i2c_slave_set_address(I2C_SLAVE(ddc), 0x50);
     qdev_realize_and_unref(DEVICE(ddc), BUS(s->i2c_bus), &error_abort);
 
     /* mmio */
diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 4fd6aeb18b5..2bb7a5441ad 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1253,7 +1253,7 @@ static void xlnx_dp_init(Object *obj)
     object_property_add_child(OBJECT(s), "dpcd", OBJECT(s->dpcd));
 
     s->edid = I2CDDC(qdev_new("i2c-ddc"));
-    i2c_set_slave_address(I2C_SLAVE(s->edid), 0x50);
+    i2c_slave_set_address(I2C_SLAVE(s->edid), 0x50);
     object_property_add_child(OBJECT(s), "edid", OBJECT(s->edid));
 
     fifo8_create(&s->rx_fifo, 16);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 27a66df7f34..6af24c9e797 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -66,7 +66,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
     return bus;
 }
 
-void i2c_set_slave_address(I2CSlave *dev, uint8_t address)
+void i2c_slave_set_address(I2CSlave *dev, uint8_t address)
 {
     dev->address = address;
 }
diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
index 2e02e1c4faa..9792583fea7 100644
--- a/hw/i2c/imx_i2c.c
+++ b/hw/i2c/imx_i2c.c
@@ -171,7 +171,7 @@ static void imx_i2c_write(void *opaque, hwaddr offset,
     switch (offset) {
     case IADR_ADDR:
         s->iadr = value & IADR_MASK;
-        /* i2c_set_slave_address(s->bus, (uint8_t)s->iadr); */
+        /* i2c_slave_set_address(s->bus, (uint8_t)s->iadr); */
         break;
     case IFDR_ADDR:
         s->ifdr = value & IFDR_MASK;
-- 
2.31.1

[PATCH v3 13/13] hw/i2c: Make i2c_start_transfer() direction argument a boolean

[Philippe Mathieu-Daudé]

From: BALATON Zoltan <balaton@eik.bme.hu>

Make the argument representing the direction of the transfer a
boolean type.
Rename the boolean argument as 'is_recv' to match i2c_recv_send().
Document the function prototype.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Message-Id: <20200621145235.9E241745712@zero.eik.bme.hu>
[PMD: Split patch, added docstring]
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/i2c/i2c.h | 11 ++++++++++-
 hw/i2c/core.c        |  4 ++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 2adf521b271..5fe94c62c00 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -80,7 +80,16 @@ struct I2CBus {
 
 I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
 int i2c_bus_busy(I2CBus *bus);
-int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv);
+/**
+ * i2c_start_transfer: start a transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ * @is_recv: indicates the transfer direction
+ *
+ * Returns: 0 on success, -1 on error
+ */
+int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv);
 void i2c_end_transfer(I2CBus *bus);
 void i2c_nack(I2CBus *bus);
 int i2c_send(I2CBus *bus, uint8_t data);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 6af24c9e797..6639ca8c2e0 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -115,7 +115,7 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
  * 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 is_recv)
 {
     I2CSlaveClass *sc;
     I2CNode *node;
@@ -157,7 +157,7 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
 
         if (sc->event) {
             trace_i2c_event("start", s->address);
-            rv = sc->event(s, recv ? I2C_START_RECV : I2C_START_SEND);
+            rv = sc->event(s, is_recv ? I2C_START_RECV : I2C_START_SEND);
             if (rv && !bus->broadcast) {
                 if (bus_scanned) {
                     /* First call, terminate the transfer. */
-- 
2.31.1

Re: [PATCH v3 01/13] hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h"

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> lm832x_key_event() is specific go LM832x devices, not to the
> I2C bus API. Move it out of "i2c.h" to a new header.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/i2c/i2c.h      |  3 ---
>   include/hw/input/lm832x.h | 26 ++++++++++++++++++++++++++
>   hw/arm/nseries.c          |  1 +
>   hw/input/lm832x.c         |  1 +
>   MAINTAINERS               |  1 +
>   5 files changed, 29 insertions(+), 3 deletions(-)
>   create mode 100644 include/hw/input/lm832x.h

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


> +#ifndef HW_INPUT_LM832X
> +#define HW_INPUT_L

You fix this error in the next patch, but better here.


r~

Re: [PATCH v3 02/13] hw/input/lm832x: Define TYPE_LM8323 in public header

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> Define TYPE_LM8323 in the public "hw/input/lm832x.h"
> header and use it in hw/arm/nseries.c.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   include/hw/input/lm832x.h | 4 +++-
>   hw/arm/nseries.c          | 2 +-
>   hw/input/lm832x.c         | 1 -
>   3 files changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 03/13] hw/display/sm501: Simplify sm501_i2c_write() logic

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/display/sm501.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 04/13] hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> Instead of using the confuse i2c_send_recv(), rewrite to directly
> call i2c_recv() & i2c_send(), resulting in code easire to review.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/display/sm501.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 05/13] hw/i2c/ppc4xx_i2c: Add reference to datasheet

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> It took me a while to find this model datasheet, since it is
> an OCR scan. Add a reference to save other developers time.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/i2c/ppc4xx_i2c.c | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> Instead of using the confuse i2c_send_recv(), rewrite to directly
> call i2c_recv() & i2c_send(), resulting in code easire to review.

easier.

> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 07/13] hw/misc/auxbus: Fix MOT/classic I2C mode

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> Since its introduction in commit 6fc7f77fd2 i2c_start_transfer()
> uses incorrectly the direction of the transfer (the last argument
> is called 'is_recv'). Fix by inverting the argument, we now have
> is_recv = !is_write.
> 
> Fixes: 6fc7f77fd2 ("introduce aux-bus")
> Reported-by: BALATON Zoltan<balaton@eik.bme.hu>
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/misc/auxbus.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 08/13] hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> To allow further simplifications in the following commits,
> start copying WRITE_I2C code to the READ_I2C, and READ_I2C_MOT
> to WRITE_I2C_MOT. No logical change.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/misc/auxbus.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 09/13] hw/misc/auxbus: Replace 'is_write' boolean by its value

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> Remove the 'is_write' boolean by directly using its value in place.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   hw/misc/auxbus.c | 28 +++++++++++-----------------
>   1 file changed, 11 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 10/13] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> @@ -161,12 +157,11 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
>           }
>   
>           ret = AUX_I2C_ACK;
> -        while (len > 0) {
> -            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
> +        for (i = 0; i < len; i++) {
> +            if (i2c_send(i2c_bus, data[i]) < 0) {
>                   ret = AUX_I2C_NACK;
>                   break;
>               }
> -            len--;
>           }

This form of updating ret is better than...

> @@ -200,14 +195,13 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t address,
>   
>           bus->last_transaction = cmd;
>           bus->last_i2c_address = address;
> -        while (len > 0) {
> -            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
> +        for (i = 0; i < len; i++) {
> +            if (i2c_send(i2c_bus, data[i]) < 0) {
>                   i2c_end_transfer(i2c_bus);
>                   break;
>               }
> -            len--;
>           }
> -        if (len == 0) {
> +        if (i == len) {
>               ret = AUX_I2C_ACK;
>           }

... this one.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 11/13] hw/i2c: Remove confusing i2c_send_recv()

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> We replaced all the i2c_send_recv() calls by the clearer i2c_recv()
> and i2c_send(), so we can remove this confuse API.

confusing.

> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   include/hw/i2c/i2c.h |  1 -
>   hw/i2c/core.c        | 50 +++++++++++++++++++-------------------------
>   2 files changed, 21 insertions(+), 30 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 12/13] hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address()

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> Other functions from I2C slave API are named "i2c_slave_XXX()".
> Follow that pattern with set_address(). Add docstring along.
> No logical change.
> 
> Patch created mechanically using:
> 
>    $ sed -i s/i2c_set_slave_address/i2c_slave_set_address/ \
>      $(git grep -l i2c_set_slave_address)
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 13/13] hw/i2c: Make i2c_start_transfer() direction argument a boolean

[Richard Henderson]

On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
> From: BALATON Zoltan <balaton@eik.bme.hu>
> 
> Make the argument representing the direction of the transfer a
> boolean type.
> Rename the boolean argument as 'is_recv' to match i2c_recv_send().
> Document the function prototype.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Message-Id: <20200621145235.9E241745712@zero.eik.bme.hu>
> [PMD: Split patch, added docstring]
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   include/hw/i2c/i2c.h | 11 ++++++++++-
>   hw/i2c/core.c        |  4 ++--
>   2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 2adf521b271..5fe94c62c00 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -80,7 +80,16 @@ struct I2CBus {
>   
>   I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
>   int i2c_bus_busy(I2CBus *bus);
> -int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv);
> +/**
> + * i2c_start_transfer: start a transfer on an I2C bus.
> + *
> + * @bus: #I2CBus to be used
> + * @address: address of the slave
> + * @is_recv: indicates the transfer direction
> + *
> + * Returns: 0 on success, -1 on error
> + */
> +int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv);
>   void i2c_end_transfer(I2CBus *bus);
>   void i2c_nack(I2CBus *bus);
>   int i2c_send(I2CBus *bus, uint8_t data);
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 6af24c9e797..6639ca8c2e0 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -115,7 +115,7 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
>    * 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 is_recv)
>   {
>       I2CSlaveClass *sc;
>       I2CNode *node;
> @@ -157,7 +157,7 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
>   
>           if (sc->event) {
>               trace_i2c_event("start", s->address);
> -            rv = sc->event(s, recv ? I2C_START_RECV : I2C_START_SEND);
> +            rv = sc->event(s, is_recv ? I2C_START_RECV : I2C_START_SEND);

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I did wonder about passing in the I2C_START_{RECV,SEND} enum directly, as that 
auto-documents the sense of the argument.  But there are quite a few instances remaining 
which would need updating.

Perhaps one more patch would be nice: introduce i2c_start_{send,recv} as inline wrappers?


r~

Re: [PATCH v3 04/13] hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Corey Minyard]

On Wed, Jun 16, 2021 at 06:14:09PM +0200, Philippe Mathieu-Daudé wrote:
> Instead of using the confuse i2c_send_recv(), rewrite to directly
> call i2c_recv() & i2c_send(), resulting in code easire to review.
                                                  ^easier



> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/display/sm501.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index f276276f7f1..569661a0746 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1033,17 +1033,18 @@ static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
>      case SM501_I2C_CONTROL:
>          if (value & SM501_I2C_CONTROL_ENABLE) {
>              if (value & SM501_I2C_CONTROL_START) {
> +                bool is_recv = s->i2c_addr & 1;
>                  int res = i2c_start_transfer(s->i2c_bus,
>                                               s->i2c_addr >> 1,
> -                                             s->i2c_addr & 1);
> +                                             is_recv);
>                  if (res) {
>                      s->i2c_status |= SM501_I2C_STATUS_ERROR;
>                  } else {
>                      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));
> -                        if (res) {
> +                        if (is_recv) {
> +                            s->i2c_data[i] = i2c_recv(s->i2c_bus);
> +                        } else if (i2c_send(s->i2c_bus, s->i2c_data[i]) < 0) {
>                              s->i2c_status |= SM501_I2C_STATUS_ERROR;
>                              return;
>                          }
> -- 
> 2.31.1
> 

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Corey Minyard]

On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
> Instead of using the confuse i2c_send_recv(), rewrite to directly
> call i2c_recv() & i2c_send(), resulting in code easire to review.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index f4c5bc12d36..b3d3da56e38 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>                          i2c->sts &= ~IIC_STS_ERR;
>                      }
>                  }
> -                if (!(i2c->sts & IIC_STS_ERR) &&
> -                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
> -                    i2c->sts |= IIC_STS_ERR;
> -                    i2c->extsts |= IIC_EXTSTS_XFRA;
> -                    break;
> +                if (!(i2c->sts & IIC_STS_ERR)) {
> +                    if (recv) {
> +                        i2c->mdata[i] = i2c_recv(i2c->bus);
> +                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {

In the previous patch you checked < 0, it would be nice to be
consistent.

-corey

> +                        i2c->sts |= IIC_STS_ERR;
> +                        i2c->extsts |= IIC_EXTSTS_XFRA;
> +                        break;
> +                    }
>                  }
>                  if (value & IIC_CNTL_RPST || !(value & IIC_CNTL_CHT)) {
>                      i2c_end_transfer(i2c->bus);
> -- 
> 2.31.1
> 

Re: [PATCH v3 01/13] hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h"

[Philippe Mathieu-Daudé]

On 6/16/21 8:38 PM, Richard Henderson wrote:
> On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
>> lm832x_key_event() is specific go LM832x devices, not to the
>> I2C bus API. Move it out of "i2c.h" to a new header.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/hw/i2c/i2c.h      |  3 ---
>>   include/hw/input/lm832x.h | 26 ++++++++++++++++++++++++++
>>   hw/arm/nseries.c          |  1 +
>>   hw/input/lm832x.c         |  1 +
>>   MAINTAINERS               |  1 +
>>   5 files changed, 29 insertions(+), 3 deletions(-)
>>   create mode 100644 include/hw/input/lm832x.h
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
>> +#ifndef HW_INPUT_LM832X
>> +#define HW_INPUT_L
> 
> You fix this error in the next patch, but better here.

Oops! I didn't noticed... Something got wrong while re-ordering
during rebase, thanks for telling me!

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Philippe Mathieu-Daudé]

On 6/16/21 9:16 PM, Corey Minyard wrote:
> On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
>> Instead of using the confuse i2c_send_recv(), rewrite to directly
>> call i2c_recv() & i2c_send(), resulting in code easire to review.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>> index f4c5bc12d36..b3d3da56e38 100644
>> --- a/hw/i2c/ppc4xx_i2c.c
>> +++ b/hw/i2c/ppc4xx_i2c.c
>> @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>                          i2c->sts &= ~IIC_STS_ERR;
>>                      }
>>                  }
>> -                if (!(i2c->sts & IIC_STS_ERR) &&
>> -                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
>> -                    i2c->sts |= IIC_STS_ERR;
>> -                    i2c->extsts |= IIC_EXTSTS_XFRA;
>> -                    break;
>> +                if (!(i2c->sts & IIC_STS_ERR)) {
>> +                    if (recv) {
>> +                        i2c->mdata[i] = i2c_recv(i2c->bus);
>> +                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {
> 
> In the previous patch you checked < 0, it would be nice to be
> consistent.

I did that first but thought Zoltan wouldn't be happy, then went back :)

I'll fix for the next iteration, thanks.

Re: [PATCH v3 10/13] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Philippe Mathieu-Daudé]

On 6/16/21 8:46 PM, Richard Henderson wrote:
> On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
>> @@ -161,12 +157,11 @@ AUXReply aux_request(AUXBus *bus, AUXCommand
>> cmd, uint32_t address,
>>           }
>>             ret = AUX_I2C_ACK;
>> -        while (len > 0) {
>> -            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
>> +        for (i = 0; i < len; i++) {
>> +            if (i2c_send(i2c_bus, data[i]) < 0) {
>>                   ret = AUX_I2C_NACK;
>>                   break;
>>               }
>> -            len--;
>>           }
> 
> This form of updating ret is better than...
> 
>> @@ -200,14 +195,13 @@ AUXReply aux_request(AUXBus *bus, AUXCommand
>> cmd, uint32_t address,
>>             bus->last_transaction = cmd;
>>           bus->last_i2c_address = address;
>> -        while (len > 0) {
>> -            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
>> +        for (i = 0; i < len; i++) {
>> +            if (i2c_send(i2c_bus, data[i]) < 0) {
>>                   i2c_end_transfer(i2c_bus);
>>                   break;
>>               }
>> -            len--;
>>           }
>> -        if (len == 0) {
>> +        if (i == len) {
>>               ret = AUX_I2C_ACK;
>>           }
> 
> ... this one.

I totally agree :) I was a bit ashamed for posting that, I thought
Zoltan would prefer less changes so used this form.
Will update on respin.

> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks!

Re: [PATCH v3 00/13] hw/i2c: Remove confusing i2c_send_recv() API

[Corey Minyard]

On Wed, Jun 16, 2021 at 06:14:05PM +0200, Philippe Mathieu-Daudé wrote:
> This is a respin of Zoltan's patch:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg714711.html
> 
> Since v2, tried to address Corey's review comments resulting
> in a i2c_send_recv() removal and code easier to review (to my
> taste at least).

Yes, this is much better.  I had a few minor comments, and Richard did,
too, but with those fixed this is a big improvement over the existing
code.

I can give you an:

Acked-by: Corey Minyard <cminyard@mvista.com>

for all of these, or I can take them into my tree.  I'm fine either way.

-corey

> 
> Supersedes: <20200623063123.20776-1-f4bug@amsat.org>
> 
> BALATON Zoltan (1):
>   hw/i2c: Make i2c_start_transfer() direction argument a boolean
> 
> Philippe Mathieu-Daudé (12):
>   hw/input/lm832x: Move lm832x_key_event() declaration to "lm832x.h"
>   hw/input/lm832x: Define TYPE_LM8323 in public header
>   hw/display/sm501: Simplify sm501_i2c_write() logic
>   hw/display/sm501: Replace i2c_send_recv() by i2c_recv() & i2c_send()
>   hw/i2c/ppc4xx_i2c: Add reference to datasheet
>   hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()
>   hw/misc/auxbus: Fix MOT/classic I2C mode
>   hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases
>   hw/misc/auxbus: Replace 'is_write' boolean by its value
>   hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()
>   hw/i2c: Remove confusing i2c_send_recv()
>   hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address()
> 
>  include/hw/i2c/i2c.h      | 21 +++++++++---
>  include/hw/input/lm832x.h | 28 ++++++++++++++++
>  hw/arm/nseries.c          |  3 +-
>  hw/arm/pxa2xx.c           |  2 +-
>  hw/arm/spitz.c            |  4 +--
>  hw/display/ati.c          |  2 +-
>  hw/display/sm501.c        | 16 +++++----
>  hw/display/xlnx_dp.c      |  2 +-
>  hw/i2c/core.c             | 56 ++++++++++++++-----------------
>  hw/i2c/imx_i2c.c          |  2 +-
>  hw/i2c/ppc4xx_i2c.c       | 15 ++++++---
>  hw/input/lm832x.c         |  2 +-
>  hw/misc/auxbus.c          | 69 +++++++++++++++++++++++++++++----------
>  MAINTAINERS               |  1 +
>  14 files changed, 149 insertions(+), 74 deletions(-)
>  create mode 100644 include/hw/input/lm832x.h
> 
> -- 
> 2.31.1
> 

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2484 bytes --]

On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> On 6/16/21 9:16 PM, Corey Minyard wrote:
>> On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
>>> Instead of using the confuse i2c_send_recv(), rewrite to directly
>>> call i2c_recv() & i2c_send(), resulting in code easire to review.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>> index f4c5bc12d36..b3d3da56e38 100644
>>> --- a/hw/i2c/ppc4xx_i2c.c
>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>> @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
>>>                          i2c->sts &= ~IIC_STS_ERR;
>>>                      }
>>>                  }
>>> -                if (!(i2c->sts & IIC_STS_ERR) &&
>>> -                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
>>> -                    i2c->sts |= IIC_STS_ERR;
>>> -                    i2c->extsts |= IIC_EXTSTS_XFRA;
>>> -                    break;
>>> +                if (!(i2c->sts & IIC_STS_ERR)) {
>>> +                    if (recv) {
>>> +                        i2c->mdata[i] = i2c_recv(i2c->bus);
>>> +                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {
>>
>> In the previous patch you checked < 0, it would be nice to be
>> consistent.
>
> I did that first but thought Zoltan wouldn't be happy, then went back :)
>
> I'll fix for the next iteration, thanks.

I generally had no problem with i2c_send_recv only that its argument that 
decides which operation to do was inverted compared to other similar i2c 
functions so my original patch just corrected that for consistency and I 
was happy with that. Having a send_recv in one func allowed to avoid 
if-else in some places like these but if you think it's better without 
this function at all I can work with that too. I'll have to check if these 
changes could break anything. At first sight I'm not sure errors are 
handled as before if recv fails but it was years ago I did the sm501 and 
ati parts and I forgot how they work so I need to check again. I'll wait 
for the final version of the series then and test that. I remember I had 
to tweak these a lot because each guest OS had drivers that did things 
slightly differently so if I've fixed one, another broke until I've found 
a way that worked for all.

Regards,
BALATON Zoltan

Re: [PATCH v3 10/13] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]

On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> On 6/16/21 8:46 PM, Richard Henderson wrote:
>> On 6/16/21 9:14 AM, Philippe Mathieu-Daudé wrote:
>>> @@ -161,12 +157,11 @@ AUXReply aux_request(AUXBus *bus, AUXCommand
>>> cmd, uint32_t address,
>>>           }
>>>             ret = AUX_I2C_ACK;
>>> -        while (len > 0) {
>>> -            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
>>> +        for (i = 0; i < len; i++) {
>>> +            if (i2c_send(i2c_bus, data[i]) < 0) {
>>>                   ret = AUX_I2C_NACK;
>>>                   break;
>>>               }
>>> -            len--;
>>>           }
>>
>> This form of updating ret is better than...
>>
>>> @@ -200,14 +195,13 @@ AUXReply aux_request(AUXBus *bus, AUXCommand
>>> cmd, uint32_t address,
>>>             bus->last_transaction = cmd;
>>>           bus->last_i2c_address = address;
>>> -        while (len > 0) {
>>> -            if (i2c_send_recv(i2c_bus, data++, true) < 0) {
>>> +        for (i = 0; i < len; i++) {
>>> +            if (i2c_send(i2c_bus, data[i]) < 0) {
>>>                   i2c_end_transfer(i2c_bus);
>>>                   break;
>>>               }
>>> -            len--;
>>>           }
>>> -        if (len == 0) {
>>> +        if (i == len) {
>>>               ret = AUX_I2C_ACK;
>>>           }
>>
>> ... this one.
>
> I totally agree :) I was a bit ashamed for posting that, I thought
> Zoltan would prefer less changes so used this form.
> Will update on respin.

It's not the number of changes that matters but if there's any change in 
behaviour. If you can make it clearer that there's no change in behaviour 
by making more changes then that's OK.

Regards,
BALATON Zoltan

>> Otherwise,
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> Thanks!
>
>

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Philippe Mathieu-Daudé]

On 6/16/21 10:01 PM, BALATON Zoltan wrote:
> On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
>> On 6/16/21 9:16 PM, Corey Minyard wrote:
>>> On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
>>>> Instead of using the confuse i2c_send_recv(), rewrite to directly
>>>> call i2c_recv() & i2c_send(), resulting in code easire to review.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
>>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>> index f4c5bc12d36..b3d3da56e38 100644
>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>> @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque,
>>>> hwaddr addr, uint64_t value,
>>>>                          i2c->sts &= ~IIC_STS_ERR;
>>>>                      }
>>>>                  }
>>>> -                if (!(i2c->sts & IIC_STS_ERR) &&
>>>> -                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
>>>> -                    i2c->sts |= IIC_STS_ERR;
>>>> -                    i2c->extsts |= IIC_EXTSTS_XFRA;
>>>> -                    break;
>>>> +                if (!(i2c->sts & IIC_STS_ERR)) {
>>>> +                    if (recv) {
>>>> +                        i2c->mdata[i] = i2c_recv(i2c->bus);
>>>> +                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {
>>>
>>> In the previous patch you checked < 0, it would be nice to be
>>> consistent.
>>
>> I did that first but thought Zoltan wouldn't be happy, then went back :)
>>
>> I'll fix for the next iteration, thanks.
> 
> I generally had no problem with i2c_send_recv only that its argument
> that decides which operation to do was inverted compared to other
> similar i2c functions so my original patch just corrected that for
> consistency and I was happy with that.

But we have to make the maintainer happy too to get patch merged ;)

> Having a send_recv in one func
> allowed to avoid if-else in some places like these but if you think it's
> better without this function at all I can work with that too. I'll have
> to check if these changes could break anything. At first sight I'm not
> sure errors are handled as before if recv fails but it was years ago I
> did the sm501 and ati parts and I forgot how they work so I need to
> check again. I'll wait for the final version of the series then and test
> that.

Thanks, that would be great!

> I remember I had to tweak these a lot because each guest OS had
> drivers that did things slightly differently so if I've fixed one,
> another broke until I've found a way that worked for all.

I wish Avocado (or any other testing framework) could help you with
your testing, and you could contribute your tests even if you can not
contribute your firmware blob due to incompatible license.
That would help us understand how you use your firmware, and save you
time while testing.

Regards,

Phil.

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4835 bytes --]

On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> On 6/16/21 10:01 PM, BALATON Zoltan wrote:
>> On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
>>> On 6/16/21 9:16 PM, Corey Minyard wrote:
>>>> On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> Instead of using the confuse i2c_send_recv(), rewrite to directly
>>>>> call i2c_recv() & i2c_send(), resulting in code easire to review.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>>  hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
>>>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>>> index f4c5bc12d36..b3d3da56e38 100644
>>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>>> @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque,
>>>>> hwaddr addr, uint64_t value,
>>>>>                          i2c->sts &= ~IIC_STS_ERR;
>>>>>                      }
>>>>>                  }
>>>>> -                if (!(i2c->sts & IIC_STS_ERR) &&
>>>>> -                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
>>>>> -                    i2c->sts |= IIC_STS_ERR;
>>>>> -                    i2c->extsts |= IIC_EXTSTS_XFRA;
>>>>> -                    break;
>>>>> +                if (!(i2c->sts & IIC_STS_ERR)) {
>>>>> +                    if (recv) {
>>>>> +                        i2c->mdata[i] = i2c_recv(i2c->bus);
>>>>> +                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {
>>>>
>>>> In the previous patch you checked < 0, it would be nice to be
>>>> consistent.
>>>
>>> I did that first but thought Zoltan wouldn't be happy, then went back :)
>>>
>>> I'll fix for the next iteration, thanks.
>>
>> I generally had no problem with i2c_send_recv only that its argument
>> that decides which operation to do was inverted compared to other
>> similar i2c functions so my original patch just corrected that for
>> consistency and I was happy with that.
>
> But we have to make the maintainer happy too to get patch merged ;)

Getting this series in actually means more work for me as I have to 
rewrite my unfinfshed patch to not use send_recv so just leaving it as it 
is would be less work. I mean this patch:

https://lists.nongnu.org/archive/html/qemu-ppc/2020-06/msg00682.html

Where having send_recv is actually nice as I can write it without too much 
if-else clauses but having send_recv and start_transfer with opposite 
sense recv argument is confusing. This is where the orignal patch comes 
from and the point was to just correct this inconsistency between 
start_transfer and send_recv because I got it wrong first but it went 
overboard now removing the whole function instead of just correcting it so 
I'll have to rewrite this patch and make it longer and also have to 
rethink what can fail and how. I also have to review and test all other 
places I've used send_recv before so at this point I would not mind if it 
was left as it is now, then I could just drop my original patch and 
reverse the last argument of send_recv in the above WIP patch and be done 
with it. Much less work than going through this series so I almost regret 
bringing this up again. But if it makes you happy so be it.

>> Having a send_recv in one func
>> allowed to avoid if-else in some places like these but if you think it's
>> better without this function at all I can work with that too. I'll have
>> to check if these changes could break anything. At first sight I'm not
>> sure errors are handled as before if recv fails but it was years ago I
>> did the sm501 and ati parts and I forgot how they work so I need to
>> check again. I'll wait for the final version of the series then and test
>> that.
>
> Thanks, that would be great!
>
>> I remember I had to tweak these a lot because each guest OS had
>> drivers that did things slightly differently so if I've fixed one,
>> another broke until I've found a way that worked for all.
>
> I wish Avocado (or any other testing framework) could help you with
> your testing, and you could contribute your tests even if you can not
> contribute your firmware blob due to incompatible license.
> That would help us understand how you use your firmware, and save you
> time while testing.

Unfortunately it's not only firmware but there's also AmigaOS which is not 
freely available so that would be hard to automate. Other than that 
testing sam460ex and pegasos2 with Linux and MorphOS could be done but I'm 
more comfortable with running a few commands that I already have than 
setting up and learning a testing framework for this so it won't be me who 
makes these test cases. It's not that many boards and OSes that I care 
about to need it automated but I've shared the commands before, they are 
on my web pages about these boards.

Regards,
BALATON Zoltan

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Corey Minyard]

On Thu, Jun 17, 2021 at 01:09:05AM +0200, BALATON Zoltan wrote:
> On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> > On 6/16/21 10:01 PM, BALATON Zoltan wrote:
> > > On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> > > > On 6/16/21 9:16 PM, Corey Minyard wrote:
> > > > > On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
> > > > > > Instead of using the confuse i2c_send_recv(), rewrite to directly
> > > > > > call i2c_recv() & i2c_send(), resulting in code easire to review.
> > > > > > 
> > > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > > > > ---
> > > > > >  hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
> > > > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > > > > index f4c5bc12d36..b3d3da56e38 100644
> > > > > > --- a/hw/i2c/ppc4xx_i2c.c
> > > > > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > > > > @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque,
> > > > > > hwaddr addr, uint64_t value,
> > > > > >                          i2c->sts &= ~IIC_STS_ERR;
> > > > > >                      }
> > > > > >                  }
> > > > > > -                if (!(i2c->sts & IIC_STS_ERR) &&
> > > > > > -                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
> > > > > > -                    i2c->sts |= IIC_STS_ERR;
> > > > > > -                    i2c->extsts |= IIC_EXTSTS_XFRA;
> > > > > > -                    break;
> > > > > > +                if (!(i2c->sts & IIC_STS_ERR)) {
> > > > > > +                    if (recv) {
> > > > > > +                        i2c->mdata[i] = i2c_recv(i2c->bus);
> > > > > > +                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {
> > > > > 
> > > > > In the previous patch you checked < 0, it would be nice to be
> > > > > consistent.
> > > > 
> > > > I did that first but thought Zoltan wouldn't be happy, then went back :)
> > > > 
> > > > I'll fix for the next iteration, thanks.
> > > 
> > > I generally had no problem with i2c_send_recv only that its argument
> > > that decides which operation to do was inverted compared to other
> > > similar i2c functions so my original patch just corrected that for
> > > consistency and I was happy with that.
> > 
> > But we have to make the maintainer happy too to get patch merged ;)
> 
> Getting this series in actually means more work for me as I have to rewrite
> my unfinfshed patch to not use send_recv so just leaving it as it is would
> be less work. I mean this patch:
> 
> https://lists.nongnu.org/archive/html/qemu-ppc/2020-06/msg00682.html
> 
> Where having send_recv is actually nice as I can write it without too much
> if-else clauses but having send_recv and start_transfer with opposite sense
> recv argument is confusing. This is where the orignal patch comes from and
> the point was to just correct this inconsistency between start_transfer and
> send_recv because I got it wrong first but it went overboard now removing
> the whole function instead of just correcting it so I'll have to rewrite
> this patch and make it longer and also have to rethink what can fail and
> how. I also have to review and test all other places I've used send_recv
> before so at this point I would not mind if it was left as it is now, then I
> could just drop my original patch and reverse the last argument of send_recv
> in the above WIP patch and be done with it. Much less work than going
> through this series so I almost regret bringing this up again. But if it
> makes you happy so be it.

I understand your concern, but these sorts of interfaces are really just
asking for trouble, as you experienced and as seen by the other patch
that fixed an error using the same interface.  It's better to have a
clear interface that takes a little more code than one that is easy to
make a mistake with.  That's my opinion, anyway.

I'm also not a big fan of i2c_start_transfer(); it's confused me more
than once.  But that would be harder to fix :-(.

-corey

> 
> > > Having a send_recv in one func
> > > allowed to avoid if-else in some places like these but if you think it's
> > > better without this function at all I can work with that too. I'll have
> > > to check if these changes could break anything. At first sight I'm not
> > > sure errors are handled as before if recv fails but it was years ago I
> > > did the sm501 and ati parts and I forgot how they work so I need to
> > > check again. I'll wait for the final version of the series then and test
> > > that.
> > 
> > Thanks, that would be great!
> > 
> > > I remember I had to tweak these a lot because each guest OS had
> > > drivers that did things slightly differently so if I've fixed one,
> > > another broke until I've found a way that worked for all.
> > 
> > I wish Avocado (or any other testing framework) could help you with
> > your testing, and you could contribute your tests even if you can not
> > contribute your firmware blob due to incompatible license.
> > That would help us understand how you use your firmware, and save you
> > time while testing.
> 
> Unfortunately it's not only firmware but there's also AmigaOS which is not
> freely available so that would be hard to automate. Other than that testing
> sam460ex and pegasos2 with Linux and MorphOS could be done but I'm more
> comfortable with running a few commands that I already have than setting up
> and learning a testing framework for this so it won't be me who makes these
> test cases. It's not that many boards and OSes that I care about to need it
> automated but I've shared the commands before, they are on my web pages
> about these boards.
> 
> Regards,
> BALATON Zoltan

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3179 bytes --]

On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
> On 6/16/21 10:01 PM, BALATON Zoltan wrote:
>> On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
>>> On 6/16/21 9:16 PM, Corey Minyard wrote:
>>>> On Wed, Jun 16, 2021 at 06:14:11PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> Instead of using the confuse i2c_send_recv(), rewrite to directly
>>>>> call i2c_recv() & i2c_send(), resulting in code easire to review.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>>  hw/i2c/ppc4xx_i2c.c | 13 ++++++++-----
>>>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
>>>>> index f4c5bc12d36..b3d3da56e38 100644
>>>>> --- a/hw/i2c/ppc4xx_i2c.c
>>>>> +++ b/hw/i2c/ppc4xx_i2c.c
>>>>> @@ -240,11 +240,14 @@ static void ppc4xx_i2c_writeb(void *opaque,
>>>>> hwaddr addr, uint64_t value,
>>>>>                          i2c->sts &= ~IIC_STS_ERR;
>>>>>                      }
>>>>>                  }
>>>>> -                if (!(i2c->sts & IIC_STS_ERR) &&
>>>>> -                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
>>>>> -                    i2c->sts |= IIC_STS_ERR;
>>>>> -                    i2c->extsts |= IIC_EXTSTS_XFRA;
>>>>> -                    break;
>>>>> +                if (!(i2c->sts & IIC_STS_ERR)) {
>>>>> +                    if (recv) {
>>>>> +                        i2c->mdata[i] = i2c_recv(i2c->bus);
>>>>> +                    } else if (i2c_send(i2c->bus, i2c->mdata[i])) {
>>>>
>>>> In the previous patch you checked < 0, it would be nice to be
>>>> consistent.
>>>
>>> I did that first but thought Zoltan wouldn't be happy, then went back :)
>>>
>>> I'll fix for the next iteration, thanks.
>>
>> I generally had no problem with i2c_send_recv only that its argument
>> that decides which operation to do was inverted compared to other
>> similar i2c functions so my original patch just corrected that for
>> consistency and I was happy with that.
>
> But we have to make the maintainer happy too to get patch merged ;)
>
>> Having a send_recv in one func
>> allowed to avoid if-else in some places like these but if you think it's
>> better without this function at all I can work with that too. I'll have
>> to check if these changes could break anything. At first sight I'm not
>> sure errors are handled as before if recv fails but it was years ago I
>> did the sm501 and ati parts and I forgot how they work so I need to
>> check again. I'll wait for the final version of the series then and test
>> that.
>
> Thanks, that would be great!

I've tried AmigaOS and MorphOS on sam460ex and mac99 with sm501 and 
ati-vga and these still work so I think the patches are OK but I did not 
test other changes to other machines so I did not give a tested-by for the 
series just some reviewed-by to patches I've verified. (Found a regression 
with AROS but that's not related to these changes.)

Regards,
BALATON Zoltan

Re: [PATCH v3 06/13] hw/i2c/ppc4xx_i2c: Replace i2c_send_recv() by i2c_recv() & i2c_send()

[Philippe Mathieu-Daudé]

On 6/18/21 1:49 AM, BALATON Zoltan wrote:
> On Wed, 16 Jun 2021, Philippe Mathieu-Daudé wrote:
>> On 6/16/21 10:01 PM, BALATON Zoltan wrote:

>>> Having a send_recv in one func
>>> allowed to avoid if-else in some places like these but if you think it's
>>> better without this function at all I can work with that too. I'll have
>>> to check if these changes could break anything. At first sight I'm not
>>> sure errors are handled as before if recv fails but it was years ago I
>>> did the sm501 and ati parts and I forgot how they work so I need to
>>> check again. I'll wait for the final version of the series then and test
>>> that.
>>
>> Thanks, that would be great!
> 
> I've tried AmigaOS and MorphOS on sam460ex and mac99 with sm501 and
> ati-vga and these still work so I think the patches are OK but I did not
> test other changes to other machines so I did not give a tested-by for
> the series just some reviewed-by to patches I've verified. (Found a
> regression with AROS but that's not related to these changes.)

Thank you Zoltan for your testing!

Phil.