diff mbox series

[v5,14/15] hw/i2c: Extract i2c_do_start_transfer() from i2c_start_transfer()

Message ID 20210617115334.2761573-15-f4bug@amsat.org
State New
Headers show
Series hw/i2c: Remove confusing i2c_send_recv() API | expand

Commit Message

Philippe Mathieu-Daudé June 17, 2021, 11:53 a.m. UTC
To allow further simplications, extract i2c_do_start_transfer()
from i2c_start_transfer(). This is mostly the same function,
but the former is static and takes an enum argument.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i2c/core.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

BALATON Zoltan June 17, 2021, 11:57 p.m. UTC | #1
On Thu, 17 Jun 2021, Philippe Mathieu-Daudé wrote:
> To allow further simplications, extract i2c_do_start_transfer()
> from i2c_start_transfer(). This is mostly the same function,
> but the former is static and takes an enum argument.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/i2c/core.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 6639ca8c2e0..5483bf95a3e 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -114,8 +114,11 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
>  * protocol uses a start transfer to switch from write to read mode
>  * without releasing the bus.  If that fails, the bus is still
>  * in a transaction.
> + *
> + * @event must be I2C_START_RECV or I2C_START_SEND.
>  */
> -int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
> +static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
> +                                 enum i2c_event event)
> {
>     I2CSlaveClass *sc;
>     I2CNode *node;
> @@ -157,7 +160,7 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
>
>         if (sc->event) {
>             trace_i2c_event("start", s->address);
> -            rv = sc->event(s, is_recv ? I2C_START_RECV : I2C_START_SEND);
> +            rv = sc->event(s, event);
>             if (rv && !bus->broadcast) {
>                 if (bus_scanned) {
>                     /* First call, terminate the transfer. */
> @@ -170,6 +173,13 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
>     return 0;
> }
>
> +int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
> +{
> +    return i2c_do_start_transfer(bus, address, is_recv
> +                                               ? I2C_START_RECV
> +                                               : I2C_START_SEND);
> +}
> +

Why is this better than keeping the original function and just add 
wrappers calling that for i2c_start_send/start_recv? So you could just 
drop this patch and change the next one to call i2c_start_transfer(bus, 
addr, true) for i2c_start_recv and with false for start_send. This seems 
to add another variant for no good reason. If the enum would only have 
these two values then that could be an improvement (although the function 
is static so nothing else could use it) but it's actually reusing an enum 
with other values so this does not seem to make it much better. Then you 
could just keep the existing function.

Regards,
BALATON Zoltan

> void i2c_end_transfer(I2CBus *bus)
> {
>     I2CSlaveClass *sc;
>
diff mbox series

Patch

diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 6639ca8c2e0..5483bf95a3e 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -114,8 +114,11 @@  bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
  * protocol uses a start transfer to switch from write to read mode
  * without releasing the bus.  If that fails, the bus is still
  * in a transaction.
+ *
+ * @event must be I2C_START_RECV or I2C_START_SEND.
  */
-int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
+static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
+                                 enum i2c_event event)
 {
     I2CSlaveClass *sc;
     I2CNode *node;
@@ -157,7 +160,7 @@  int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
 
         if (sc->event) {
             trace_i2c_event("start", s->address);
-            rv = sc->event(s, is_recv ? I2C_START_RECV : I2C_START_SEND);
+            rv = sc->event(s, event);
             if (rv && !bus->broadcast) {
                 if (bus_scanned) {
                     /* First call, terminate the transfer. */
@@ -170,6 +173,13 @@  int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
     return 0;
 }
 
+int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
+{
+    return i2c_do_start_transfer(bus, address, is_recv
+                                               ? I2C_START_RECV
+                                               : I2C_START_SEND);
+}
+
 void i2c_end_transfer(I2CBus *bus)
 {
     I2CSlaveClass *sc;