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

[BALATON Zoltan]

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;