Re: [Qemu-devel] [PATCH 03/13] libqos: move common i2c code to libqos

[Thomas Huth <thuth@redhat.com>]

On 18/03/2019 18.15, Paolo Bonzini wrote:
> The functions to read/write 8-bit or 16-bit registers are the same
> in tmp105 and pca9552 tests, and in fact they are a special case of
> "read block"/"write block" functionality; read block in turn is used
> in ds1338-test.
> 
> Move everything inside libqos-test, removing the duplication.  Account
> for the small differences by adding to tmp105-test.c the "read register
> after writing" behavior that is specific to it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/ds1338-test.c  |  8 +----
>  tests/libqos/i2c.c   | 47 ++++++++++++++++++++++++++
>  tests/libqos/i2c.h   | 11 ++++++
>  tests/pca9552-test.c | 37 +++++---------------
>  tests/tmp105-test.c  | 80 ++++++++++++--------------------------------
>  5 files changed, 88 insertions(+), 95 deletions(-)
> 
> diff --git a/tests/ds1338-test.c b/tests/ds1338-test.c
> index 742dad9113..88f829f241 100644
> --- a/tests/ds1338-test.c
> +++ b/tests/ds1338-test.c
> @@ -35,17 +35,11 @@ static inline uint8_t bcd2bin(uint8_t x)
>  
>  static void send_and_receive(void)
>  {
> -    uint8_t cmd[1];
>      uint8_t resp[7];
>      time_t now = time(NULL);
>      struct tm *tm_ptr = gmtime(&now);
>  
> -    /* reset the index in the RTC memory */
> -    cmd[0] = 0;
> -    i2c_send(i2c, addr, cmd, 1);
> -
> -    /* retrieve the date */
> -    i2c_recv(i2c, addr, resp, 7);
> +    i2c_read_block(i2c, addr, 0, resp, sizeof(resp));
>  
>      /* check retrieved time againt local time */
>      g_assert_cmpuint(bcd2bin(resp[4]), == , tm_ptr->tm_mday);
> diff --git a/tests/libqos/i2c.c b/tests/libqos/i2c.c
> index 23bc2a3eb2..daf9a96617 100644
> --- a/tests/libqos/i2c.c
> +++ b/tests/libqos/i2c.c
> @@ -21,3 +21,50 @@ void i2c_recv(I2CAdapter *i2c, uint8_t addr,
>  {
>      i2c->recv(i2c, addr, buf, len);
>  }
> +
> +void i2c_read_block(I2CAdapter *i2c, uint8_t addr, uint8_t reg,
> +                       uint8_t *buf, uint16_t len)
> +{
> +    i2c_send(i2c, addr, &reg, 1);
> +    i2c_recv(i2c, addr, buf, len);
> +}
> +
> +void i2c_write_block(I2CAdapter *i2c, uint8_t addr, uint8_t reg,
> +                     const uint8_t *buf, uint16_t len)
> +{
> +    uint8_t *cmd = g_malloc(len + 1);
> +    cmd[0] = reg;
> +    memcpy(&cmd[1], buf, len);
> +    i2c_send(i2c, addr, cmd, len + 1);
> +    g_free(cmd);
> +}

So the i2c_write_block function only uses i2c_send() ...

> +uint8_t i2c_get8(I2CAdapter *i2c, uint8_t addr, uint8_t reg)
> +{
> +    uint8_t resp[1];
> +    i2c_read_block(i2c, addr, reg, resp, sizeof(resp));
> +    return resp[0];
> +}
> +
> +uint16_t i2c_get16(I2CAdapter *i2c, uint8_t addr, uint8_t reg)
> +{
> +    uint8_t resp[2];
> +    i2c_read_block(i2c, addr, reg, resp, sizeof(resp));
> +    return (resp[0] << 8) | resp[1];
> +}
> +
> +void i2c_set8(I2CAdapter *i2c, uint8_t addr, uint8_t reg,
> +              uint8_t value)
> +{
> +    i2c_write_block(i2c, addr, reg, &value, 1);
> +}
> +
> +void i2c_set16(I2CAdapter *i2c, uint8_t addr, uint8_t reg,
> +               uint16_t value)
> +{
> +    uint8_t data[2];
> +
> +    data[0] = value >> 8;
> +    data[1] = value & 255;
> +    i2c_write_block(i2c, addr, reg, data, sizeof(data));
> +}

... i.e. the i2c_set8/16() functions also only use i2c_send()...

> -static void tmp105_set8(I2CAdapter *i2c, uint8_t addr, uint8_t reg,
> -                        uint8_t value)
> -{
> -    uint8_t cmd[2];
> -    uint8_t resp[1];
> -
> -    cmd[0] = reg;
> -    cmd[1] = value;
> -    i2c_send(i2c, addr, cmd, 2);
> -    i2c_recv(i2c, addr, resp, 1);
> -    g_assert_cmphex(resp[0], ==, cmd[1]);
> -}
> -
> -static void tmp105_set16(I2CAdapter *i2c, uint8_t addr, uint8_t reg,
> -                         uint16_t value)
> -{
> -    uint8_t cmd[3];
> -    uint8_t resp[2];
> -
> -    cmd[0] = reg;
> -    cmd[1] = value >> 8;
> -    cmd[2] = value & 255;
> -    i2c_send(i2c, addr, cmd, 3);
> -    i2c_recv(i2c, addr, resp, 2);
> -    g_assert_cmphex(resp[0], ==, cmd[1]);
> -    g_assert_cmphex(resp[1], ==, cmd[2]);
> -}

... but the old set8/16 functions also used i2c_recv() and
g_assert_cmphex() ... shouldn't this be added to the new functions, too?

 Thomas