Re: [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start

qemu-devel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_se

From:	BALATON Zoltan
Subject:	Re: [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send()
Date:	Fri, 18 Jun 2021 02:27:43 +0200 (CEST)

On Thu, 17 Jun 2021, Philippe Mathieu-Daudé wrote:

To ease reviewing code using the I2C bus API, introduce the
i2c_start_recv() and i2c_start_send() helpers which don't
take the confusing 'is_recv' boolean argument.

In defence of this is_recv I'd like to mention that I2C has such a bit indevice addresses that decides if it's a receive or send so this isactually a good match for that. The problem was that the send_recvfunction had it reversed. With that fixed it was actually a good match fori2c operations and in some cases allowed to write less code. Like theauxbus shows that became twice as large using these separate functionsinstead of being able to handle send or receive the same way.


Regards,
BALATON Zoltan

Use these new helpers in the SMBus / AUX bus models.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
include/hw/i2c/i2c.h  | 24 ++++++++++++++++++++++++
hw/i2c/core.c         | 10 ++++++++++
hw/i2c/pm_smbus.c     |  4 ++--
hw/i2c/smbus_master.c | 22 +++++++++++-----------
hw/misc/auxbus.c      | 12 ++++++------
5 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 21f2dba1bf7..5ca3b708c0b 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -88,9 +88,33 @@ int i2c_bus_busy(I2CBus *bus);
 * @address: address of the slave
 * @is_recv: indicates the transfer direction
 *
+ * When @is_recv is a known boolean constant, use the
+ * i2c_start_recv() or i2c_start_send() helper instead.
+ *
 * Returns: 0 on success, -1 on error
 */
int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv);
+
+/**
+ * i2c_start_recv: start a 'receive' transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ *
+ * Returns: 0 on success, -1 on error
+ */
+int i2c_start_recv(I2CBus *bus, uint8_t address);
+
+/**
+ * i2c_start_send: start a 'send' transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ *
+ * Returns: 0 on success, -1 on error
+ */
+int i2c_start_send(I2CBus *bus, uint8_t address);
+
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 5483bf95a3e..416372ad00c 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -180,6 +180,16 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool 
is_recv)
                                               : I2C_START_SEND);
}

+int i2c_start_recv(I2CBus *bus, uint8_t address)
+{
+    return i2c_do_start_transfer(bus, address, I2C_START_RECV);
+}
+
+int i2c_start_send(I2CBus *bus, uint8_t address)
+{
+    return i2c_do_start_transfer(bus, address, I2C_START_SEND);
+}
+
void i2c_end_transfer(I2CBus *bus)
{
    I2CSlaveClass *sc;
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 06e1e5321b9..d7eae548cbc 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -128,14 +128,14 @@ static void smb_transaction(PMSMBus *s)
         * So at least Linux may or may not set the read bit here.
         * So just ignore the read bit for this command.
         */
-        if (i2c_start_transfer(bus, addr, 0)) {
+        if (i2c_start_send(bus, addr)) {
            goto error;
        }
        ret = i2c_send(bus, s->smb_data1);
        if (ret) {
            goto error;
        }
-        if (i2c_start_transfer(bus, addr, 1)) {
+        if (i2c_start_recv(bus, addr)) {
            goto error;
        }
        s->in_i2c_block_read = true;
diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
index dc43b8637d1..6a53c34e70b 100644
--- a/hw/i2c/smbus_master.c
+++ b/hw/i2c/smbus_master.c
@@ -29,7 +29,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
{
    uint8_t data;

-    if (i2c_start_transfer(bus, addr, 1)) {
+    if (i2c_start_recv(bus, addr)) {
        return -1;
    }
    data = i2c_recv(bus);
@@ -40,7 +40,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)

int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
{
-    if (i2c_start_transfer(bus, addr, 0)) {
+    if (i2c_start_send(bus, addr)) {
        return -1;
    }
    i2c_send(bus, data);
@@ -51,11 +51,11 @@ int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
{
    uint8_t data;
-    if (i2c_start_transfer(bus, addr, 0)) {
+    if (i2c_start_send(bus, addr)) {
        return -1;
    }
    i2c_send(bus, command);
-    if (i2c_start_transfer(bus, addr, 1)) {
+    if (i2c_start_recv(bus, addr)) {
        i2c_end_transfer(bus);
        return -1;
    }
@@ -67,7 +67,7 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t 
command)

int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data)
{
-    if (i2c_start_transfer(bus, addr, 0)) {
+    if (i2c_start_send(bus, addr)) {
        return -1;
    }
    i2c_send(bus, command);
@@ -79,11 +79,11 @@ int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t 
command, uint8_t data)
int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
{
    uint16_t data;
-    if (i2c_start_transfer(bus, addr, 0)) {
+    if (i2c_start_send(bus, addr)) {
        return -1;
    }
    i2c_send(bus, command);
-    if (i2c_start_transfer(bus, addr, 1)) {
+    if (i2c_start_recv(bus, addr)) {
        i2c_end_transfer(bus);
        return -1;
    }
@@ -96,7 +96,7 @@ int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t 
command)

int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
{
-    if (i2c_start_transfer(bus, addr, 0)) {
+    if (i2c_start_send(bus, addr)) {
        return -1;
    }
    i2c_send(bus, command);
@@ -113,12 +113,12 @@ int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t 
command, uint8_t *data,
    int i;

    if (send_cmd) {
-        if (i2c_start_transfer(bus, addr, 0)) {
+        if (i2c_start_send(bus, addr)) {
            return -1;
        }
        i2c_send(bus, command);
    }
-    if (i2c_start_transfer(bus, addr, 1)) {
+    if (i2c_start_recv(bus, addr)) {
        if (send_cmd) {
            i2c_end_transfer(bus);
        }
@@ -149,7 +149,7 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t 
command, uint8_t *data,
        len = 32;
    }

-    if (i2c_start_transfer(bus, addr, 0)) {
+    if (i2c_start_send(bus, addr)) {
        return -1;
    }
    i2c_send(bus, command);
diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 44aa9730bc9..434ff8d910d 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -135,7 +135,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t 
address,
            i2c_end_transfer(i2c_bus);
        }

-        if (i2c_start_transfer(i2c_bus, address, true)) {
+        if (i2c_start_recv(i2c_bus, address)) {
            ret = AUX_I2C_NACK;
            break;
        }
@@ -151,7 +151,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t 
address,
            i2c_end_transfer(i2c_bus);
        }

-        if (i2c_start_transfer(i2c_bus, address, false)) {
+        if (i2c_start_send(i2c_bus, address)) {
            ret = AUX_I2C_NACK;
            break;
        }
@@ -179,7 +179,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t 
address,
            /*
             * No transactions started..
             */
-            if (i2c_start_transfer(i2c_bus, address, false)) {
+            if (i2c_start_send(i2c_bus, address)) {
                break;
            }
        } else if ((address != bus->last_i2c_address) ||
@@ -188,7 +188,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, false)) {
+            if (i2c_start_send(i2c_bus, address)) {
                break;
            }
        }
@@ -210,7 +210,7 @@ AUXReply aux_request(AUXBus *bus, AUXCommand cmd, uint32_t 
address,
            /*
             * No transactions started..
             */
-            if (i2c_start_transfer(i2c_bus, address, true)) {
+            if (i2c_start_recv(i2c_bus, address)) {
                break;
            }
        } else if ((address != bus->last_i2c_address) ||
@@ -219,7 +219,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, true)) {
+            if (i2c_start_recv(i2c_bus, address)) {
                break;
            }
        }

[Prev in Thread]

Current Thread

[Next in Thread]

[PATCH v5 07/15] hw/misc/auxbus: Fix MOT/classic I2C mode, (continued)
- [PATCH v5 07/15] hw/misc/auxbus: Fix MOT/classic I2C mode, Philippe Mathieu-Daudé, 2021/06/17
- [PATCH v5 08/15] hw/misc/auxbus: Explode READ_I2C / WRITE_I2C_MOT cases, Philippe Mathieu-Daudé, 2021/06/17
- [PATCH v5 09/15] hw/misc/auxbus: Replace 'is_write' boolean by its value, Philippe Mathieu-Daudé, 2021/06/17
- [PATCH v5 10/15] hw/misc/auxbus: Replace i2c_send_recv() by i2c_recv() & i2c_send(), Philippe Mathieu-Daudé, 2021/06/17
- [PATCH v5 11/15] hw/i2c: Remove confusing i2c_send_recv(), Philippe Mathieu-Daudé, 2021/06/17
- [PATCH v5 12/15] hw/i2c: Rename i2c_set_slave_address() -> i2c_slave_set_address(), Philippe Mathieu-Daudé, 2021/06/17
- [PATCH v5 13/15] hw/i2c: Make i2c_start_transfer() direction argument a boolean, Philippe Mathieu-Daudé, 2021/06/17
- [PATCH v5 14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer(), Philippe Mathieu-Daudé, 2021/06/17
  - Re: [PATCH v5 14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer(), BALATON Zoltan, 2021/06/17
- [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send(), Philippe Mathieu-Daudé, 2021/06/17
  - Re: [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send(), BALATON Zoltan <=
- Re: [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API, Corey Minyard, 2021/06/17

Prev by Date: Re: [PATCH v5 14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer()
Next by Date: Re: [PATCH v7 4/7] migration/dirtyrate: introduce struct and adjust DirtyRateStat
Previous by thread: [PATCH v5 15/15] hw/i2c: Introduce i2c_start_recv() and i2c_start_send()
Next by thread: Re: [PATCH v5 00/15] hw/i2c: Remove confusing i2c_send_recv() API
Index(es):
- Date
- Thread