Re: [Qemu-devel] [PATCH] hw/display: QOM'ify g364fb.c

[Peter Maydell]

On 25 December 2016 at 08:25, xiaoqiang zhao <address@hidden> wrote:
> Drop the old Sysbus init and use instance_init and
> DeviceClass::realize instead
>
> Signed-off-by: xiaoqiang zhao <address@hidden>
> ---
>  hw/display/g364fb.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 70ef2c7453..7a0fe48dd5 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -504,18 +504,24 @@ typedef struct {
>      G364State g364;
>  } G364SysBusState;
>
> -static int g364fb_sysbus_init(SysBusDevice *sbd)
> +static void g364fb_sysbus_init(Object *obj)

This isn't a sysbus init function any more, so we should
rename it. Since the existing g364fb_init() is only
called from one place, we should just inline its
contents into the right places, and have one function
364fb_init for the instance_init and g364fb_realize
for the realize function.

>  {
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      DeviceState *dev = DEVICE(sbd);
>      G364SysBusState *sbs = G364(dev);
>      G364State *s = &sbs->g364;
>
> -    g364fb_init(dev, s);
>      sysbus_init_irq(sbd, &s->irq);
>      sysbus_init_mmio(sbd, &s->mem_ctrl);
>      sysbus_init_mmio(sbd, &s->mem_vram);

This means we call sysbus_init_mmio() on the MemoryRegion*s
before we have initialized them. That seems like a bad idea.

> +}
>
> -    return 0;
> +static void g364fb_sysbus_realize(DeviceState *dev, Error **errp)
> +{
> +    G364SysBusState *sbs = G364(dev);
> +    G364State *s = &sbs->g364;
> +
> +    g364fb_init(dev, s);
>  }
>
>  static void g364fb_sysbus_reset(DeviceState *d)
> @@ -534,9 +540,8 @@ static Property g364fb_sysbus_properties[] = {
>  static void g364fb_sysbus_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>
> -    k->init = g364fb_sysbus_init;
> +    dc->realize = g364fb_sysbus_realize;
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>      dc->desc = "G364 framebuffer";
>      dc->reset = g364fb_sysbus_reset;
> @@ -548,6 +553,7 @@ static const TypeInfo g364fb_sysbus_info = {
>      .name          = TYPE_G364,
>      .parent        = TYPE_SYS_BUS_DEVICE,
>      .instance_size = sizeof(G364SysBusState),
> +    .instance_init = g364fb_sysbus_init,
>      .class_init    = g364fb_sysbus_class_init,
>  };

This device is used in the MIPS Jazz machine.
Can you cc the MIPS maintainers and also Hervé Poussineau
(the Jazz maintainer) on the next version of this patch,
please?

thanks
-- PMM