Re: [PATCH] hw/rtc: Add RTC PCF8563 module

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Ilya,

On 21/2/25 08:34, Ilya Chichkov wrote:
> Add PCF8563 a real-time clock with calendar and I2C interface.
> This commit adds support for interfacing with it and implements
> functionality of setting timer, alarm, reading and writing time.
> 
> Datasheet: https://www.micros.com.pl/mediaserver/UZPCF8563ts5_0001.pdf
> 
> Signed-off-by: Ilya Chichkov <i.chichkov@yadro.com>
> ---
>   hw/rtc/Kconfig       |   5 +
>   hw/rtc/meson.build   |   1 +
>   hw/rtc/pcf8563_rtc.c | 638 +++++++++++++++++++++++++++++++++++++++++++
>   hw/rtc/trace-events  |  11 +
>   4 files changed, 655 insertions(+)
>   create mode 100644 hw/rtc/pcf8563_rtc.c


> +static void pcf8563_realize(DeviceState *dev, Error **errp)
> +{
> +    I2CSlave *i2c = I2C_SLAVE(dev);
> +    Pcf8563State *s = PCF8563(i2c);
> +
> +    s->alarm_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &alarm_timer_cb, s);
> +    s->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &timer_cb, s);
> +    s->irq_gen_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &irq_gen_timer_cb, s);
> +
> +    pcf8563_reset(i2c);

Calling pcf8563_reset() here, the device will only be reset once
(cold reset), not when a guest VM resets (hot reset).

See below to register a reset handler called when guest resets.

> +}


> +static void pcf8563_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);

        ResettableClass *rc = RESETTABLE_CLASS(klass);

> +
> +    k->event = pcf8563_event;
> +    k->recv = pcf8563_rx;
> +    k->send = pcf8563_tx;
> +    dc->realize = pcf8563_realize;
> +    dc->vmsd = &vmstate_pcf8563;

        rc->phases.hold = pcf8563_reset_hold;

> +
> +    trace_pcf8563_rtc_init();

This trace event is wrong, this isn't the device init but the
registration of its model.

> +}

Your device is missing testing coverage. Our QTest framework seems
well adapted for it.

You can use some examples as template:

$ git grep -l i2c tests/qtest/*.c
tests/qtest/adm1266-test.c
tests/qtest/adm1272-test.c
tests/qtest/bcm2835-i2c-test.c
tests/qtest/ds1338-test.c
tests/qtest/emc141x-test.c
tests/qtest/isl_pmbus_vr-test.c
tests/qtest/lsm303dlhc-mag-test.c
tests/qtest/max34451-test.c
tests/qtest/microbit-test.c
tests/qtest/npcm7xx_smbus-test.c
tests/qtest/pca9552-test.c
tests/qtest/pnv-host-i2c-test.c
tests/qtest/qtest_aspeed.c
tests/qtest/tmp105-test.c
tests/qtest/tpm-tis-i2c-test.c

Regards,

Phil.