Re: [PATCH 1/2] isa/piix4: Resolve RTCState attribute

[Peter Maydell <peter.maydell@linaro.org>]

On Sat, 5 Feb 2022 at 18:05, Bernhard Beschow <shentey@gmail.com> wrote:
>
> Assuming that mc146818_rtc_init() is "syntactic sugar" for code that could
> be converted into configuration in the future, this patch is a step towards
> this future by freeing piix4 to care about the inner details of mc146818.
>
> Furthermore, by reusing mc146818_rtc_init(), piix4's code becomes more
> homogenious to other code using the mc146818. So piix4 can be refactored in
> the same way as code already using mc146818_rtc_init().
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>  hw/isa/piix4.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 0fe7b69bc4..08b4262467 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -46,7 +46,6 @@ struct PIIX4State {
>      qemu_irq cpu_intr;
>      qemu_irq *isa;
>
> -    RTCState rtc;
>      /* Reset Control Register */
>      MemoryRegion rcr_mem;
>      uint8_t rcr;
> @@ -193,22 +192,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>      i8257_dma_init(isa_bus, 0);
>
>      /* RTC */
> -    qdev_prop_set_int32(DEVICE(&s->rtc), "base_year", 2000);
> -    if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
> -        return;
> -    }
> -    isa_init_irq(ISA_DEVICE(&s->rtc), &s->rtc.irq, RTC_ISA_IRQ);
> +    mc146818_rtc_init(isa_bus, 2000, NULL);
>
>      piix4_dev = dev;
>  }
>
> -static void piix4_init(Object *obj)
> -{
> -    PIIX4State *s = PIIX4_PCI_DEVICE(obj);
> -
> -    object_initialize(&s->rtc, sizeof(s->rtc), TYPE_MC146818_RTC);
> -}
> -
>  static void piix4_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -233,7 +221,6 @@ static const TypeInfo piix4_info = {
>      .name          = TYPE_PIIX4_PCI_DEVICE,
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(PIIX4State),
> -    .instance_init = piix4_init,
>      .class_init    = piix4_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },

This looks like it's going backwards from the way we'd usually
write code for devices that contain other devices these days.
The "we have an init function that does stuff" is the older
style. The newer style has the inner-device as an embedded
struct in the container-device struct, which is initialized,
configured and realized using standard functions like object_initialize
and qdev_realize. (I do wonder whether that ought to be
object_initialize_child() here, incidentally, but haven't checked the
details to be certain.)

The existing uses of mc146818_rtc_init() are mostly in older
code, and also in board-initialization code, which traditionally
didn't have a convenient struct to embed the device-struct into.
hw/isa/vt82c686.c is the only use in another device model
(which could in theory be refactored to the embed-the-device-struct
style, though the benefit of making the change isn't large, which
is one reason we still have the mix of both in the tree).

thanks
-- PMM