Re: [PATCH 05/12] vt82c686: Make vt82c686b-pm an abstract base class and add vt8231-pm based on it

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 7 Jan 2021 11:38:21 +0100 (CET)
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Thu, 7 Jan 2021, Philippe Mathieu-Daudé wrote:
> > Hi Zoltan,
> >
> > On 1/6/21 10:13 PM, BALATON Zoltan wrote:  
> >> The vt82c686b-pm model can be shared between VT82C686B and VT8231. The
> >> only difference between the two is the device id in what we emulate so
> >> make an abstract via-pm model by renaming appropriately and add types
> >> for vt82c686b-pm and vt8231-pm based on it.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >>  hw/isa/vt82c686.c         | 87 ++++++++++++++++++++++++++-------------
> >>  include/hw/isa/vt82c686.h |  1 +
> >>  2 files changed, 59 insertions(+), 29 deletions(-)  
> > ...
> >  
> >> +typedef struct via_pm_init_info {
> >> +    uint16_t device_id;
> >> +} ViaPMInitInfo;
> >> +
> >>  static void via_pm_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >> +    ViaPMInitInfo *info = data;
> >>
> >> -    k->realize = vt82c686b_pm_realize;
> >> +    k->realize = via_pm_realize;
> >>      k->config_write = pm_write_config;
> >>      k->vendor_id = PCI_VENDOR_ID_VIA;
> >> -    k->device_id = PCI_DEVICE_ID_VIA_ACPI;
> >> +    k->device_id = info->device_id;
> >>      k->class_id = PCI_CLASS_BRIDGE_OTHER;
> >>      k->revision = 0x40;
> >> -    dc->reset = vt82c686b_pm_reset;
> >> -    dc->desc = "PM";
> >> +    dc->reset = via_pm_reset;  
> >  
> >> +    /* Reason: part of VIA south bridge, does not exist stand alone */
> >> +    dc->user_creatable = false;
> >>      dc->vmsd = &vmstate_acpi;
> >> -    set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);  
> >
> > Please do this change in a previous patch.  
> 
> OK, done.
> 
> >>  }
> >>
> >>  static const TypeInfo via_pm_info = {
> >> -    .name          = TYPE_VT82C686B_PM,
> >> +    .name          = TYPE_VIA_PM,
> >>      .parent        = TYPE_PCI_DEVICE,
> >> -    .instance_size = sizeof(VT686PMState),
> >> -    .class_init    = via_pm_class_init,
> >> +    .instance_size = sizeof(ViaPMState),
> >> +    .abstract      = true,
> >>      .interfaces = (InterfaceInfo[]) {
> >>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> >>          { },
> >>      },
> >>  };
> >>
> >> +static const ViaPMInitInfo vt82c686b_pm_init_info = {
> >> +    .device_id = PCI_DEVICE_ID_VIA_ACPI,
> >> +};
> >> +
> >> +static const TypeInfo vt82c686b_pm_info = {
> >> +    .name          = TYPE_VT82C686B_PM,
> >> +    .parent        = TYPE_VIA_PM,
> >> +    .class_init    = via_pm_class_init,
> >> +    .class_data    = (void *)&vt82c686b_pm_init_info,  
> >
> > Igor said new code should avoid using .class_data:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg678305.html
> > Can you convert to "leaf class"? Then this patch is good to go.  
> 
> That says for machines it is not advised (and Igor generally prefers init 
> funcs everywhere) but this is a device model. Is it still not allowed to 
> use class_data here? I think this is shorter this way than with an init 
> function but I may try to convert if absolutely necessary.

For this simple case class_init would be cleaner as it doesn't need casting (void*).
But I'm fine with either approaches here.

> Regards,
> BALATON Zoltan
> 
> > A trivial example of conversion is commit f0eeb4b6154
> > ("hw/arm/raspi: Avoid using TypeInfo::class_data pointer").
> >  
> >> +};
> >> +
> >> +static const ViaPMInitInfo vt8231_pm_init_info = {
> >> +    .device_id = 0x8235,

Is it possible to replace magic number with a human readable macro?

> >> +};
> >> +
> >> +static const TypeInfo vt8231_pm_info = {
> >> +    .name          = TYPE_VT8231_PM,
> >> +    .parent        = TYPE_VIA_PM,
> >> +    .class_init    = via_pm_class_init,
> >> +    .class_data    = (void *)&vt8231_pm_init_info,
> >> +};
> >>
> >>  
> >
> >