Re: [Qemu-devel] [PATCH 02/13] qgraph: fix qos_node_contains with options

Re: [Qemu-devel] [PATCH 02/13] qgraph: fix qos_node_contains with options

[Thomas Huth]

On 18/03/2019 18.15, Paolo Bonzini wrote:
> Currently, if qos_node_contains was passed options, it would still
> create an edge without any options.  Instead, in that case
> NULL acts as a terminator.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/libqos/qgraph.c | 12 ++++++++----
>  tests/libqos/qgraph.h | 15 +++++++++------
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/libqos/qgraph.c b/tests/libqos/qgraph.c
> index 122efc1b7b..9f738f1849 100644
> --- a/tests/libqos/qgraph.c
> +++ b/tests/libqos/qgraph.c
> @@ -630,15 +630,19 @@ void qos_node_create_driver(const char *name, QOSCreateDriverFunc function)
>  }
>  
>  void qos_node_contains(const char *container, const char *contained,
> -                       ...)
> +                       QOSGraphEdgeOptions *opts, ...)
>  {
>      va_list va;
> -    va_start(va, contained);
> -    QOSGraphEdgeOptions *opts;
>  
> +    if (opts == NULL) {
> +        add_edge(container, contained, QEDGE_CONTAINS, NULL);
> +        return;
> +    }
> +
> +    va_start(va, contained);

As patchew complained - you've got to either replace "contained" with
"opts" here, or switch the order of the options in the prototype.

 Thomas

Re: [Qemu-devel] [PATCH 02/13] qgraph: fix qos_node_contains with options

[Thomas Huth]

On 18/03/2019 18.15, Paolo Bonzini wrote:
> Currently, if qos_node_contains was passed options, it would still
> create an edge without any options.  Instead, in that case
> NULL acts as a terminator.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/libqos/qgraph.c | 12 ++++++++----
>  tests/libqos/qgraph.h | 15 +++++++++------
>  2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/libqos/qgraph.c b/tests/libqos/qgraph.c
> index 122efc1b7b..9f738f1849 100644
> --- a/tests/libqos/qgraph.c
> +++ b/tests/libqos/qgraph.c
> @@ -630,15 +630,19 @@ void qos_node_create_driver(const char *name, QOSCreateDriverFunc function)
>  }
>  
>  void qos_node_contains(const char *container, const char *contained,
> -                       ...)
> +                       QOSGraphEdgeOptions *opts, ...)
>  {
>      va_list va;
> -    va_start(va, contained);
> -    QOSGraphEdgeOptions *opts;
>  
> +    if (opts == NULL) {
> +        add_edge(container, contained, QEDGE_CONTAINS, NULL);
> +        return;
> +    }
> +
> +    va_start(va, contained);

As patchew complained - you've got to either replace "contained" with
"opts" here, or switch the order of the options in the prototype.

 Thomas

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

[Thomas Huth]

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

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

[Thomas Huth]

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

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

[Paolo Bonzini]

On 09/04/19 11:42, Thomas Huth wrote:
>> -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?

That is dependent on the actual device you are working with.  I probably
should add some code to the tmp105 test that tests this behavior too.

Paolo

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

[Paolo Bonzini]

On 09/04/19 11:42, Thomas Huth wrote:
>> -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?

That is dependent on the actual device you are working with.  I probably
should add some code to the tmp105 test that tests this behavior too.

Paolo

Re: [Qemu-devel] [PATCH 05/13] pca9552-test: do not rely on state across tests

[Thomas Huth]

On 18/03/2019 18.15, Paolo Bonzini wrote:
> receive_autoinc is relying on the LED state that is set by
> send_and_receive.  Stop doing that, because qgraph resets the
> machine between tests.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/pca9552-test.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/pca9552-test.c b/tests/pca9552-test.c
> index 06359b7435..89b4445e29 100644
> --- a/tests/pca9552-test.c
> +++ b/tests/pca9552-test.c
> @@ -18,11 +18,20 @@
>  
>  static I2CAdapter *i2c;
>  
> +static void pca9552_init(I2CAdapter *i2c)
> +{
> +    /* Switch on LEDs 0 and 12 */
> +    i2c_set8(i2c, PCA9552_TEST_ADDR, PCA9552_LS0, 0x54);
> +    i2c_set8(i2c, PCA9552_TEST_ADDR, PCA9552_LS3, 0x54);
> +}
> +
>  static void receive_autoinc(void)
>  {
>      uint8_t resp;
>      uint8_t reg = PCA9552_LS0 | PCA9552_AUTOINC;
>  
> +    pca9552_init(i2cdev);
> +
>      i2c_send(i2c, PCA9552_TEST_ADDR, &reg, 1);
>  
>      /* PCA9552_LS0 */
> @@ -52,16 +61,14 @@ static void send_and_receive(void)
>      value = i2c_get8(i2c, PCA9552_TEST_ADDR, PCA9552_INPUT0);
>      g_assert_cmphex(value, ==, 0x0);
>  
> -    /* Switch on LED 0 */
> -    i2c_set8(i2c, PCA9552_TEST_ADDR, PCA9552_LS0, 0x54);
> +    pca9552_init(i2cdev);
> +
>      value = i2c_get8(i2c, PCA9552_TEST_ADDR, PCA9552_LS0);
>      g_assert_cmphex(value, ==, 0x54);
>  
>      value = i2c_get8(i2c, PCA9552_TEST_ADDR, PCA9552_INPUT0);
>      g_assert_cmphex(value, ==, 0x01);
>  
> -    /* Switch on LED 12 */
> -    i2c_set8(i2c, PCA9552_TEST_ADDR, PCA9552_LS3, 0x54);
>      value = i2c_get8(i2c, PCA9552_TEST_ADDR, PCA9552_LS3);
>      g_assert_cmphex(value, ==, 0x54);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 05/13] pca9552-test: do not rely on state across tests

[Thomas Huth]

On 18/03/2019 18.15, Paolo Bonzini wrote:
> receive_autoinc is relying on the LED state that is set by
> send_and_receive.  Stop doing that, because qgraph resets the
> machine between tests.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/pca9552-test.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/pca9552-test.c b/tests/pca9552-test.c
> index 06359b7435..89b4445e29 100644
> --- a/tests/pca9552-test.c
> +++ b/tests/pca9552-test.c
> @@ -18,11 +18,20 @@
>  
>  static I2CAdapter *i2c;
>  
> +static void pca9552_init(I2CAdapter *i2c)
> +{
> +    /* Switch on LEDs 0 and 12 */
> +    i2c_set8(i2c, PCA9552_TEST_ADDR, PCA9552_LS0, 0x54);
> +    i2c_set8(i2c, PCA9552_TEST_ADDR, PCA9552_LS3, 0x54);
> +}
> +
>  static void receive_autoinc(void)
>  {
>      uint8_t resp;
>      uint8_t reg = PCA9552_LS0 | PCA9552_AUTOINC;
>  
> +    pca9552_init(i2cdev);
> +
>      i2c_send(i2c, PCA9552_TEST_ADDR, &reg, 1);
>  
>      /* PCA9552_LS0 */
> @@ -52,16 +61,14 @@ static void send_and_receive(void)
>      value = i2c_get8(i2c, PCA9552_TEST_ADDR, PCA9552_INPUT0);
>      g_assert_cmphex(value, ==, 0x0);
>  
> -    /* Switch on LED 0 */
> -    i2c_set8(i2c, PCA9552_TEST_ADDR, PCA9552_LS0, 0x54);
> +    pca9552_init(i2cdev);
> +
>      value = i2c_get8(i2c, PCA9552_TEST_ADDR, PCA9552_LS0);
>      g_assert_cmphex(value, ==, 0x54);
>  
>      value = i2c_get8(i2c, PCA9552_TEST_ADDR, PCA9552_INPUT0);
>      g_assert_cmphex(value, ==, 0x01);
>  
> -    /* Switch on LED 12 */
> -    i2c_set8(i2c, PCA9552_TEST_ADDR, PCA9552_LS3, 0x54);
>      value = i2c_get8(i2c, PCA9552_TEST_ADDR, PCA9552_LS3);
>      g_assert_cmphex(value, ==, 0x54);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 06/13] imx25-pdk: create ds1338 for qtest inside the test

[Thomas Huth]

On 18/03/2019 18.15, Paolo Bonzini wrote:
> There is no need to have a test device created by the board.
> Instead, create it in the qtest so that we will be able to run
> it on other boards too.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/arm/imx25_pdk.c  | 9 ---------
>  tests/ds1338-test.c | 2 +-
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
> index 9f3ee14739..65e1768663 100644
> --- a/hw/arm/imx25_pdk.c
> +++ b/hw/arm/imx25_pdk.c
> @@ -132,15 +132,6 @@ static void imx25_pdk_init(MachineState *machine)
>       */
>      if (!qtest_enabled()) {
>          arm_load_kernel(&s->soc.cpu, &imx25_pdk_binfo);
> -    } else {
> -        /*
> -         * This I2C device doesn't exist on the real board.
> -         * We add it here (only on qtest usage) to be able to do a bit
> -         * of simple qtest. See "make check" for details.
> -         */
> -        i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]),
> -                                                      "i2c-bus.0"),
> -                         "ds1338", 0x68);
>      }
>  }
>  
> diff --git a/tests/ds1338-test.c b/tests/ds1338-test.c
> index 88f829f241..bd72a159cb 100644
> --- a/tests/ds1338-test.c
> +++ b/tests/ds1338-test.c
> @@ -54,7 +54,7 @@ int main(int argc, char **argv)
>  
>      g_test_init(&argc, &argv, NULL);
>  
> -    s = qtest_start("-display none -machine imx25-pdk");
> +    s = qtest_start("-display none -machine imx25-pdk -device ds1338,address=0x68");
>      i2c = imx_i2c_create(s, IMX25_I2C_0_BASE);
>      addr = DS1338_ADDR;

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 06/13] imx25-pdk: create ds1338 for qtest inside the test

[Thomas Huth]

On 18/03/2019 18.15, Paolo Bonzini wrote:
> There is no need to have a test device created by the board.
> Instead, create it in the qtest so that we will be able to run
> it on other boards too.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/arm/imx25_pdk.c  | 9 ---------
>  tests/ds1338-test.c | 2 +-
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
> index 9f3ee14739..65e1768663 100644
> --- a/hw/arm/imx25_pdk.c
> +++ b/hw/arm/imx25_pdk.c
> @@ -132,15 +132,6 @@ static void imx25_pdk_init(MachineState *machine)
>       */
>      if (!qtest_enabled()) {
>          arm_load_kernel(&s->soc.cpu, &imx25_pdk_binfo);
> -    } else {
> -        /*
> -         * This I2C device doesn't exist on the real board.
> -         * We add it here (only on qtest usage) to be able to do a bit
> -         * of simple qtest. See "make check" for details.
> -         */
> -        i2c_create_slave((I2CBus *)qdev_get_child_bus(DEVICE(&s->soc.i2c[0]),
> -                                                      "i2c-bus.0"),
> -                         "ds1338", 0x68);
>      }
>  }
>  
> diff --git a/tests/ds1338-test.c b/tests/ds1338-test.c
> index 88f829f241..bd72a159cb 100644
> --- a/tests/ds1338-test.c
> +++ b/tests/ds1338-test.c
> @@ -54,7 +54,7 @@ int main(int argc, char **argv)
>  
>      g_test_init(&argc, &argv, NULL);
>  
> -    s = qtest_start("-display none -machine imx25-pdk");
> +    s = qtest_start("-display none -machine imx25-pdk -device ds1338,address=0x68");
>      i2c = imx_i2c_create(s, IMX25_I2C_0_BASE);
>      addr = DS1338_ADDR;

Reviewed-by: Thomas Huth <thuth@redhat.com>