diff mbox series

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

Message ID c8fa8df147473c3ec5f3284b4a5d37fc9741e824.1609967638.git.balaton@eik.bme.hu
State New
Headers show
Series vt82c686b clean ups and vt8231 emulation | expand

Commit Message

BALATON Zoltan Jan. 6, 2021, 9:13 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Jan. 7, 2021, 8:02 a.m. UTC | #1
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.

>  }
>  
>  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.

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,
> +};
> +
> +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,
> +};
>  
>
BALATON Zoltan Jan. 7, 2021, 10:38 a.m. UTC | #2
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.

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,
>> +};
>> +
>> +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,
>> +};
>>
>>
>
>
Igor Mammedov Jan. 7, 2021, 10:56 a.m. UTC | #3
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,
> >> +};
> >>
> >>  
> >
> >
BALATON Zoltan Jan. 7, 2021, 7:57 p.m. UTC | #4
On Thu, 7 Jan 2021, Igor Mammedov wrote:
> 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*).

Cast is only needed because of this:

../hw/isa/vt82c686.c:240:22: error: initialization discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
      .class_data    = &vt82c686b_pm_init_info,
                       ^
../hw/isa/vt82c686.c:251:22: error: initialization discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
      .class_data    = &vt8231_pm_init_info,
                       ^

Could be avoided by removing const from declaration, i.e.

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 604ab4a55e..14e9a4bf76 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -229,7 +229,7 @@ static const TypeInfo via_pm_info = {
      },
  };

-static const ViaPMInitInfo vt82c686b_pm_init_info = {
+static ViaPMInitInfo vt82c686b_pm_init_info = {
      .device_id = PCI_DEVICE_ID_VIA_ACPI,
  };

@@ -237,10 +237,10 @@ 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,
+    .class_data    = &vt82c686b_pm_init_info,
  };

-static const ViaPMInitInfo vt8231_pm_init_info = {
+static ViaPMInitInfo vt8231_pm_init_info = {
      .device_id = 0x8235,
  };

@@ -248,7 +248,7 @@ 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,
+    .class_data    = &vt8231_pm_init_info,
  };


Is that any better?

> But I'm fine with either approaches here.

In that case I'd just leave it as it is now unless you say otherwise.

>>
>>> 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?

Yes, I'll need to add these IDs where the other constants are defined in 
include/hw/pci/pci_ids.h but I did not want to touch that file until now 
as I tried to localise changes only to hw/isa/vt82c686.c but I can move 
these to there as new constants.

Regards,
BALATON Zoltan

>>>> +};
>>>> +
>>>> +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,
>>>> +};
diff mbox series

Patch

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index fc2a1f4430..a989e29fe5 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -27,9 +27,10 @@ 
 #include "exec/address-spaces.h"
 #include "trace.h"
 
-OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM)
+#define TYPE_VIA_PM "via-pm"
+OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
 
-struct VT686PMState {
+struct ViaPMState {
     PCIDevice dev;
     MemoryRegion io;
     ACPIREGS ar;
@@ -37,7 +38,7 @@  struct VT686PMState {
     PMSMBus smb;
 };
 
-static void pm_io_space_update(VT686PMState *s)
+static void pm_io_space_update(ViaPMState *s)
 {
     uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
 
@@ -47,7 +48,7 @@  static void pm_io_space_update(VT686PMState *s)
     memory_region_transaction_commit();
 }
 
-static void smb_io_space_update(VT686PMState *s)
+static void smb_io_space_update(ViaPMState *s)
 {
     uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
 
@@ -59,7 +60,7 @@  static void smb_io_space_update(VT686PMState *s)
 
 static int vmstate_acpi_post_load(void *opaque, int version_id)
 {
-    VT686PMState *s = opaque;
+    ViaPMState *s = opaque;
 
     pm_io_space_update(s);
     smb_io_space_update(s);
@@ -72,20 +73,20 @@  static const VMStateDescription vmstate_acpi = {
     .minimum_version_id = 1,
     .post_load = vmstate_acpi_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, VT686PMState),
-        VMSTATE_UINT16(ar.pm1.evt.sts, VT686PMState),
-        VMSTATE_UINT16(ar.pm1.evt.en, VT686PMState),
-        VMSTATE_UINT16(ar.pm1.cnt.cnt, VT686PMState),
-        VMSTATE_STRUCT(apm, VT686PMState, 0, vmstate_apm, APMState),
-        VMSTATE_TIMER_PTR(ar.tmr.timer, VT686PMState),
-        VMSTATE_INT64(ar.tmr.overflow_time, VT686PMState),
+        VMSTATE_PCI_DEVICE(dev, ViaPMState),
+        VMSTATE_UINT16(ar.pm1.evt.sts, ViaPMState),
+        VMSTATE_UINT16(ar.pm1.evt.en, ViaPMState),
+        VMSTATE_UINT16(ar.pm1.cnt.cnt, ViaPMState),
+        VMSTATE_STRUCT(apm, ViaPMState, 0, vmstate_apm, APMState),
+        VMSTATE_TIMER_PTR(ar.tmr.timer, ViaPMState),
+        VMSTATE_INT64(ar.tmr.overflow_time, ViaPMState),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
 {
-    VT686PMState *s = VT82C686B_PM(d);
+    ViaPMState *s = VIA_PM(d);
 
     trace_via_pm_write(addr, val, len);
     pci_default_write_config(d, addr, val, len);
@@ -127,7 +128,7 @@  static const MemoryRegionOps pm_io_ops = {
     },
 };
 
-static void pm_update_sci(VT686PMState *s)
+static void pm_update_sci(ViaPMState *s)
 {
     int sci_level, pmsts;
 
@@ -145,13 +146,13 @@  static void pm_update_sci(VT686PMState *s)
 
 static void pm_tmr_timer(ACPIREGS *ar)
 {
-    VT686PMState *s = container_of(ar, VT686PMState, ar);
+    ViaPMState *s = container_of(ar, ViaPMState, ar);
     pm_update_sci(s);
 }
 
-static void vt82c686b_pm_reset(DeviceState *d)
+static void via_pm_reset(DeviceState *d)
 {
-    VT686PMState *s = VT82C686B_PM(d);
+    ViaPMState *s = VIA_PM(d);
 
     memset(s->dev.config + PCI_CONFIG_HEADER_SIZE, 0,
            PCI_CONFIG_SPACE_SIZE - PCI_CONFIG_HEADER_SIZE);
@@ -164,9 +165,9 @@  static void vt82c686b_pm_reset(DeviceState *d)
     smb_io_space_update(s);
 }
 
-static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
+static void via_pm_realize(PCIDevice *dev, Error **errp)
 {
-    VT686PMState *s = VT82C686B_PM(dev);
+    ViaPMState *s = VIA_PM(dev);
 
     pci_set_word(dev->config + PCI_STATUS, PCI_STATUS_FAST_BACK |
                  PCI_STATUS_DEVSEL_MEDIUM);
@@ -177,8 +178,7 @@  static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
 
     apm_init(dev, &s->apm, NULL, s);
 
-    memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s,
-                          "vt82c686-pm", 0x100);
+    memory_region_init_io(&s->io, OBJECT(dev), &pm_io_ops, s, "via-pm", 0x100);
     memory_region_add_subregion(pci_address_space_io(dev), 0, &s->io);
     memory_region_set_enabled(&s->io, false);
 
@@ -187,34 +187,61 @@  static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2);
 }
 
+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);
 }
 
 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,
+};
+
+static const ViaPMInitInfo vt8231_pm_init_info = {
+    .device_id = 0x8235,
+};
+
+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,
+};
+
 
 typedef struct SuperIOConfig {
     uint8_t regs[0x100];
@@ -423,6 +450,8 @@  static const TypeInfo via_superio_info = {
 static void vt82c686b_register_types(void)
 {
     type_register_static(&via_pm_info);
+    type_register_static(&vt82c686b_pm_info);
+    type_register_static(&vt8231_pm_info);
     type_register_static(&via_info);
     type_register_static(&via_superio_info);
 }
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 5b0a1ffe72..9b6d610e83 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -4,6 +4,7 @@ 
 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
 #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
 #define TYPE_VT82C686B_PM "vt82c686b-pm"
+#define TYPE_VT8231_PM "vt8231-pm"
 #define TYPE_VIA_AC97 "via-ac97"
 #define TYPE_VIA_MC97 "via-mc97"