Patchwork [v3,2/8] usb/ehci: Use class_data to init PCI variations

login
register
mail settings
Submitter Peter Crosthwaite
Date Oct. 29, 2012, 1:34 a.m.
Message ID <e0b8c06e00ab35dad06f672fb266088bff5028ac.1351473902.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/194778/
State New
Headers show

Comments

Peter Crosthwaite - Oct. 29, 2012, 1:34 a.m.
Got rid of the duplication of the class init functions for the two PCI EHCI
variants. The PCI specifics are passed in as as class_data and set by a common
class_init function.

Premeptively defined a new Class "EHCICLass" for the upcomming addition of new
fields. The class_data is an instance of EHCICLass that forms a template for the
class to generate.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Got rid of union for sharing EHCIClassDefinition - made PCI specific
Simplified literal class_data arrays in ehci_info accordingly
removed null sentinel from ehci_info and used ARRAY_SIZE for type_regsiter loop
	bound instead

 hw/usb/hcd-ehci.c |   76 ++++++++++++++++++++++++++++------------------------
 1 files changed, 41 insertions(+), 35 deletions(-)
Andreas Färber - Oct. 29, 2012, 9:35 a.m.
Am 29.10.2012 02:34, schrieb Peter Crosthwaite:
> Got rid of the duplication of the class init functions for the two PCI EHCI
> variants. The PCI specifics are passed in as as class_data and set by a common
> class_init function.
> 
> Premeptively defined a new Class "EHCICLass" for the upcomming addition of new

"Preemptively", "upcoming"

> fields. The class_data is an instance of EHCICLass that forms a template for the
> class to generate.

Using "EHCI[PCI]Class" to template itself seems a bit awkward, Anthony
do you have any thoughts on this? The usual way would be to have a
dedicated EHCIInfo struct or so.

> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> Got rid of union for sharing EHCIClassDefinition - made PCI specific
> Simplified literal class_data arrays in ehci_info accordingly
> removed null sentinel from ehci_info and used ARRAY_SIZE for type_regsiter loop
> 	bound instead
> 
>  hw/usb/hcd-ehci.c |   76 ++++++++++++++++++++++++++++------------------------
>  1 files changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 6c65a73..274225b 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -2641,46 +2641,49 @@ static Property ehci_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -static void ehci_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -
> -    k->init = usb_ehci_initfn;
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
> -    k->revision = 0x10;
> -    k->class_id = PCI_CLASS_SERIAL_USB;
> -    dc->vmsd = &vmstate_ehci;
> -    dc->props = ehci_properties;
> -}
> +typedef struct EHCIPCIClass {
> +    PCIDeviceClass pci;
> +} EHCIPCIClass;
>  
> -static TypeInfo ehci_info = {
> -    .name          = "usb-ehci",
> -    .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(EHCIState),
> -    .class_init    = ehci_class_init,
> -};
> -
> -static void ich9_ehci_class_init(ObjectClass *klass, void *data)
> +static void ehci_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -
> -    k->init = usb_ehci_initfn;
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
> -    k->revision = 0x03;
> -    k->class_id = PCI_CLASS_SERIAL_USB;
> +    EHCIPCIClass *k = (EHCIPCIClass *)klass;

Please use a proper QOM cast macro: EHCI_PCI_CLASS(klass)

In this case however, please keep using PCIDeviceClass rather than
trying to access it through a named member. If we need to access any
dedicated EHCIPCIClass fields later in the series we can add additional
variables the QOM way.

> +    EHCIPCIClass *template = data;
> +
> +    k->pci.init = usb_ehci_initfn;
> +    k->pci.vendor_id = template->pci.vendor_id;
> +    k->pci.device_id = template->pci.device_id; /* ich4 */
> +    k->pci.revision = template->pci.revision;
> +    k->pci.class_id = PCI_CLASS_SERIAL_USB;
>      dc->vmsd = &vmstate_ehci;
>      dc->props = ehci_properties;
>  }
>  
> -static TypeInfo ich9_ehci_info = {
> -    .name          = "ich9-usb-ehci1",
> -    .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(EHCIState),
> -    .class_init    = ich9_ehci_class_init,
> +static TypeInfo ehci_info[] = {

Can this still be made const despite the embedded class_data?

> +    {
> +        .name          = "usb-ehci",
> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(EHCIState),
> +        .class_init    = ehci_class_init,
> +        .class_size    = sizeof(EHCIPCIClass),
> +        .class_data    = (EHCIPCIClass[]) {{
> +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
> +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
> +            .pci.revision  = 0x10,
> +        } }
> +    }, {
> +        .name          = "ich9-usb-ehci1",

Do we have a suitable header to introduce TYPE_* constants for these
while at it? That would benefit q35.

Andreas

> +        .parent        = TYPE_PCI_DEVICE,
> +        .instance_size = sizeof(EHCIState),
> +        .class_init    = ehci_class_init,
> +        .class_size    = sizeof(EHCIPCIClass),
> +        .class_data    = (EHCIPCIClass[]) {{
> +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
> +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
> +            .pci.revision  = 0x03,
> +        } }
> +    },
>  };
>  
>  static int usb_ehci_initfn(PCIDevice *dev)
> @@ -2769,8 +2772,11 @@ static int usb_ehci_initfn(PCIDevice *dev)
>  
>  static void ehci_register_types(void)
>  {
> -    type_register_static(&ehci_info);
> -    type_register_static(&ich9_ehci_info);
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(ehci_info); i++) {
> +        type_register_static(&ehci_info[i]);
> +    }
>  }
>  
>  type_init(ehci_register_types)
>
Peter Crosthwaite - Oct. 29, 2012, 11:43 a.m.
On Oct 29, 2012 7:35 PM, "Andreas Färber" <afaerber@suse.de> wrote:
>
> Am 29.10.2012 02:34, schrieb Peter Crosthwaite:
> > Got rid of the duplication of the class init functions for the two PCI
EHCI
> > variants. The PCI specifics are passed in as as class_data and set by a
common
> > class_init function.
> >
> > Premeptively defined a new Class "EHCICLass" for the upcomming addition
of new
>
> "Preemptively", "upcoming"
>
> > fields. The class_data is an instance of EHCICLass that forms a
template for the
> > class to generate.
>
> Using "EHCI[PCI]Class" to template itself seems a bit awkward, Anthony
> do you have any thoughts on this? The usual way would be to have a
> dedicated EHCIInfo struct or so.

Why? The class struct defines the exactly all information needed. Seems
redundant and error prone to have to maintain two structs with the same
fields?

> >
> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > ---
> > Got rid of union for sharing EHCIClassDefinition - made PCI specific
> > Simplified literal class_data arrays in ehci_info accordingly
> > removed null sentinel from ehci_info and used ARRAY_SIZE for
type_regsiter loop
> >       bound instead
> >
> >  hw/usb/hcd-ehci.c |   76
++++++++++++++++++++++++++++------------------------
> >  1 files changed, 41 insertions(+), 35 deletions(-)
> >
> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> > index 6c65a73..274225b 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -2641,46 +2641,49 @@ static Property ehci_properties[] = {
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > -static void ehci_class_init(ObjectClass *klass, void *data)
> > -{
> > -    DeviceClass *dc = DEVICE_CLASS(klass);
> > -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > -
> > -    k->init = usb_ehci_initfn;
> > -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> > -    k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
> > -    k->revision = 0x10;
> > -    k->class_id = PCI_CLASS_SERIAL_USB;
> > -    dc->vmsd = &vmstate_ehci;
> > -    dc->props = ehci_properties;
> > -}
> > +typedef struct EHCIPCIClass {
> > +    PCIDeviceClass pci;
> > +} EHCIPCIClass;
> >
> > -static TypeInfo ehci_info = {
> > -    .name          = "usb-ehci",
> > -    .parent        = TYPE_PCI_DEVICE,
> > -    .instance_size = sizeof(EHCIState),
> > -    .class_init    = ehci_class_init,
> > -};
> > -
> > -static void ich9_ehci_class_init(ObjectClass *klass, void *data)
> > +static void ehci_class_init(ObjectClass *klass, void *data)
> >  {
> >      DeviceClass *dc = DEVICE_CLASS(klass);
> > -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > -
> > -    k->init = usb_ehci_initfn;
> > -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> > -    k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
> > -    k->revision = 0x03;
> > -    k->class_id = PCI_CLASS_SERIAL_USB;
> > +    EHCIPCIClass *k = (EHCIPCIClass *)klass;
>
> Please use a proper QOM cast macro: EHCI_PCI_CLASS(klass)
>

How is this possible whe TYPE_EHCI_PCI doesn't exist ? The FOO_CLASS macros
require the name of the class but that does not exist as its a dynamic
class.

> In this case however, please keep using PCIDeviceClass rather than
> trying to access it through a named member. If we need to access any
> dedicated EHCIPCIClass fields later in the series we can add additional
> variables the QOM way.

What do you mean the Qom way? Don't we just add fields to the class and
class_info and class_init copies them across ?

> > +    EHCIPCIClass *template = data;
> > +
> > +    k->pci.init = usb_ehci_initfn;
> > +    k->pci.vendor_id = template->pci.vendor_id;
> > +    k->pci.device_id = template->pci.device_id; /* ich4 */
> > +    k->pci.revision = template->pci.revision;
> > +    k->pci.class_id = PCI_CLASS_SERIAL_USB;
> >      dc->vmsd = &vmstate_ehci;
> >      dc->props = ehci_properties;
> >  }
> >
> > -static TypeInfo ich9_ehci_info = {
> > -    .name          = "ich9-usb-ehci1",
> > -    .parent        = TYPE_PCI_DEVICE,
> > -    .instance_size = sizeof(EHCIState),
> > -    .class_init    = ich9_ehci_class_init,
> > +static TypeInfo ehci_info[] = {
>
> Can this still be made const despite the embedded class_data?
>
> > +    {
> > +        .name          = "usb-ehci",
> > +        .parent        = TYPE_PCI_DEVICE,
> > +        .instance_size = sizeof(EHCIState),
> > +        .class_init    = ehci_class_init,
> > +        .class_size    = sizeof(EHCIPCIClass),
> > +        .class_data    = (EHCIPCIClass[]) {{
> > +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
> > +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
> > +            .pci.revision  = 0x10,
> > +        } }
> > +    }, {
> > +        .name          = "ich9-usb-ehci1",
>
> Do we have a suitable header to introduce TYPE_* constants for these
> while at it? That would benefit q35.
>

No because there are no TYPE_ constants. The classes are private to
hcd-ehci.c.

Regards
Peter

> Andreas
>
> > +        .parent        = TYPE_PCI_DEVICE,
> > +        .instance_size = sizeof(EHCIState),
> > +        .class_init    = ehci_class_init,
> > +        .class_size    = sizeof(EHCIPCIClass),
> > +        .class_data    = (EHCIPCIClass[]) {{
> > +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
> > +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
> > +            .pci.revision  = 0x03,
> > +        } }
> > +    },
> >  };
> >
> >  static int usb_ehci_initfn(PCIDevice *dev)
> > @@ -2769,8 +2772,11 @@ static int usb_ehci_initfn(PCIDevice *dev)
> >
> >  static void ehci_register_types(void)
> >  {
> > -    type_register_static(&ehci_info);
> > -    type_register_static(&ich9_ehci_info);
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(ehci_info); i++) {
> > +        type_register_static(&ehci_info[i]);
> > +    }
> >  }
> >
> >  type_init(ehci_register_types)
> >
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
Andreas Färber - Oct. 29, 2012, 1:21 p.m.
Am 29.10.2012 12:43, schrieb Peter Crosthwaite:
> 
> On Oct 29, 2012 7:35 PM, "Andreas Färber" <afaerber@suse.de
> <mailto:afaerber@suse.de>> wrote:
>>
>> Am 29.10.2012 02:34, schrieb Peter Crosthwaite:
>> > Got rid of the duplication of the class init functions for the two
> PCI EHCI
>> > variants. The PCI specifics are passed in as as class_data and set
> by a common
>> > class_init function.
>> >
>> > Premeptively defined a new Class "EHCICLass" for the upcomming
> addition of new
>>
>> "Preemptively", "upcoming"
>>
>> > fields. The class_data is an instance of EHCICLass that forms a
> template for the
>> > class to generate.
>>
>> Using "EHCI[PCI]Class" to template itself seems a bit awkward, Anthony
>> do you have any thoughts on this? The usual way would be to have a
>> dedicated EHCIInfo struct or so.
> 
> Why? The class struct defines the exactly all information needed. Seems
> redundant and error prone to have to maintain two structs with the same
> fields?

This was a general discussion, involving Anthony and others. One reason
I can think of is that PCIDeviceClass is much larger than the actual
values you are interested in. I once did such a hack involving
XtensaCPUClass and worked around it for applying I believe.

>> >
>> > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com
> <mailto:peter.crosthwaite@xilinx.com>>
>> > ---
>> > Got rid of union for sharing EHCIClassDefinition - made PCI specific
>> > Simplified literal class_data arrays in ehci_info accordingly
>> > removed null sentinel from ehci_info and used ARRAY_SIZE for
> type_regsiter loop
>> >       bound instead
>> >
>> >  hw/usb/hcd-ehci.c |   76
> ++++++++++++++++++++++++++++------------------------
>> >  1 files changed, 41 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
>> > index 6c65a73..274225b 100644
>> > --- a/hw/usb/hcd-ehci.c
>> > +++ b/hw/usb/hcd-ehci.c
>> > @@ -2641,46 +2641,49 @@ static Property ehci_properties[] = {
>> >      DEFINE_PROP_END_OF_LIST(),
>> >  };
>> >
>> > -static void ehci_class_init(ObjectClass *klass, void *data)
>> > -{
>> > -    DeviceClass *dc = DEVICE_CLASS(klass);
>> > -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> > -
>> > -    k->init = usb_ehci_initfn;
>> > -    k->vendor_id = PCI_VENDOR_ID_INTEL;
>> > -    k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
>> > -    k->revision = 0x10;
>> > -    k->class_id = PCI_CLASS_SERIAL_USB;
>> > -    dc->vmsd = &vmstate_ehci;
>> > -    dc->props = ehci_properties;
>> > -}
>> > +typedef struct EHCIPCIClass {
>> > +    PCIDeviceClass pci;
>> > +} EHCIPCIClass;
>> >
>> > -static TypeInfo ehci_info = {
>> > -    .name          = "usb-ehci",
>> > -    .parent        = TYPE_PCI_DEVICE,
>> > -    .instance_size = sizeof(EHCIState),
>> > -    .class_init    = ehci_class_init,
>> > -};
>> > -
>> > -static void ich9_ehci_class_init(ObjectClass *klass, void *data)
>> > +static void ehci_class_init(ObjectClass *klass, void *data)
>> >  {
>> >      DeviceClass *dc = DEVICE_CLASS(klass);
>> > -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> > -
>> > -    k->init = usb_ehci_initfn;
>> > -    k->vendor_id = PCI_VENDOR_ID_INTEL;
>> > -    k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
>> > -    k->revision = 0x03;
>> > -    k->class_id = PCI_CLASS_SERIAL_USB;
>> > +    EHCIPCIClass *k = (EHCIPCIClass *)klass;
>>
>> Please use a proper QOM cast macro: EHCI_PCI_CLASS(klass)
>>
> 
> How is this possible whe TYPE_EHCI_PCI doesn't exist ? The FOO_CLASS
> macros require the name of the class but that does not exist as its a
> dynamic class.

Well, why doesn't it exist? Surely a type exists that uses your
EHCIPCIClass struct and if we don't have an abstract base type for
individual models then adding a TypeInfo alongside the struct surely is
clean and trivial.

>> In this case however, please keep using PCIDeviceClass rather than
>> trying to access it through a named member. If we need to access any
>> dedicated EHCIPCIClass fields later in the series we can add additional
>> variables the QOM way.
> 
> What do you mean the Qom way? Don't we just add fields to the class and
> class_info and class_init copies them across ?

The QOM way refers to accessing fields directly: k->foo, not x->bar.foo.
Your patch adds unnecessary indirection here when all you're actually
changing is the assignment of three values. This was discussed between
Anthony, mst and others in lengths.

>> > +    EHCIPCIClass *template = data;
>> > +
>> > +    k->pci.init = usb_ehci_initfn;
>> > +    k->pci.vendor_id = template->pci.vendor_id;
>> > +    k->pci.device_id = template->pci.device_id; /* ich4 */
>> > +    k->pci.revision = template->pci.revision;
>> > +    k->pci.class_id = PCI_CLASS_SERIAL_USB;
>> >      dc->vmsd = &vmstate_ehci;
>> >      dc->props = ehci_properties;
>> >  }
>> >
>> > -static TypeInfo ich9_ehci_info = {
>> > -    .name          = "ich9-usb-ehci1",
>> > -    .parent        = TYPE_PCI_DEVICE,
>> > -    .instance_size = sizeof(EHCIState),
>> > -    .class_init    = ich9_ehci_class_init,
>> > +static TypeInfo ehci_info[] = {
>>
>> Can this still be made const despite the embedded class_data?
>>
>> > +    {
>> > +        .name          = "usb-ehci",
>> > +        .parent        = TYPE_PCI_DEVICE,
>> > +        .instance_size = sizeof(EHCIState),
>> > +        .class_init    = ehci_class_init,
>> > +        .class_size    = sizeof(EHCIPCIClass),
>> > +        .class_data    = (EHCIPCIClass[]) {{
>> > +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
>> > +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
>> > +            .pci.revision  = 0x10,
>> > +        } }
>> > +    }, {
>> > +        .name          = "ich9-usb-ehci1",
>>
>> Do we have a suitable header to introduce TYPE_* constants for these
>> while at it? That would benefit q35.
>>
> 
> No because there are no TYPE_ constants. The classes are private to
> hcd-ehci.c.

Note that I wrote "introduce TYPE_* constants". What about hw/usb.h at
least for instantiatable types? Question was equally addressed to Gerd.

No doubt this is v1.3 material but we shouldn't rush pulling something
in just because of the Soft Freeze. ;) Adding constants can be done as
follow-up, but the QOM issues are easier fixed from the start.

Regards,
Andreas

>> > +        .parent        = TYPE_PCI_DEVICE,
>> > +        .instance_size = sizeof(EHCIState),
>> > +        .class_init    = ehci_class_init,
>> > +        .class_size    = sizeof(EHCIPCIClass),
>> > +        .class_data    = (EHCIPCIClass[]) {{
>> > +            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
>> > +            .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
>> > +            .pci.revision  = 0x03,
>> > +        } }
>> > +    },
>> >  };
>> >
>> >  static int usb_ehci_initfn(PCIDevice *dev)
>> > @@ -2769,8 +2772,11 @@ static int usb_ehci_initfn(PCIDevice *dev)
>> >
>> >  static void ehci_register_types(void)
>> >  {
>> > -    type_register_static(&ehci_info);
>> > -    type_register_static(&ich9_ehci_info);
>> > +    int i;
>> > +
>> > +    for (i = 0; i < ARRAY_SIZE(ehci_info); i++) {
>> > +        type_register_static(&ehci_info[i]);
>> > +    }
>> >  }
>> >
>> >  type_init(ehci_register_types)

Patch

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 6c65a73..274225b 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2641,46 +2641,49 @@  static Property ehci_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void ehci_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->init = usb_ehci_initfn;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
-    k->revision = 0x10;
-    k->class_id = PCI_CLASS_SERIAL_USB;
-    dc->vmsd = &vmstate_ehci;
-    dc->props = ehci_properties;
-}
+typedef struct EHCIPCIClass {
+    PCIDeviceClass pci;
+} EHCIPCIClass;
 
-static TypeInfo ehci_info = {
-    .name          = "usb-ehci",
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(EHCIState),
-    .class_init    = ehci_class_init,
-};
-
-static void ich9_ehci_class_init(ObjectClass *klass, void *data)
+static void ehci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->init = usb_ehci_initfn;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
-    k->revision = 0x03;
-    k->class_id = PCI_CLASS_SERIAL_USB;
+    EHCIPCIClass *k = (EHCIPCIClass *)klass;
+    EHCIPCIClass *template = data;
+
+    k->pci.init = usb_ehci_initfn;
+    k->pci.vendor_id = template->pci.vendor_id;
+    k->pci.device_id = template->pci.device_id; /* ich4 */
+    k->pci.revision = template->pci.revision;
+    k->pci.class_id = PCI_CLASS_SERIAL_USB;
     dc->vmsd = &vmstate_ehci;
     dc->props = ehci_properties;
 }
 
-static TypeInfo ich9_ehci_info = {
-    .name          = "ich9-usb-ehci1",
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(EHCIState),
-    .class_init    = ich9_ehci_class_init,
+static TypeInfo ehci_info[] = {
+    {
+        .name          = "usb-ehci",
+        .parent        = TYPE_PCI_DEVICE,
+        .instance_size = sizeof(EHCIState),
+        .class_init    = ehci_class_init,
+        .class_size    = sizeof(EHCIPCIClass),
+        .class_data    = (EHCIPCIClass[]) {{
+            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
+            .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
+            .pci.revision  = 0x10,
+        } }
+    }, {
+        .name          = "ich9-usb-ehci1",
+        .parent        = TYPE_PCI_DEVICE,
+        .instance_size = sizeof(EHCIState),
+        .class_init    = ehci_class_init,
+        .class_size    = sizeof(EHCIPCIClass),
+        .class_data    = (EHCIPCIClass[]) {{
+            .pci.vendor_id = PCI_VENDOR_ID_INTEL,
+            .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
+            .pci.revision  = 0x03,
+        } }
+    },
 };
 
 static int usb_ehci_initfn(PCIDevice *dev)
@@ -2769,8 +2772,11 @@  static int usb_ehci_initfn(PCIDevice *dev)
 
 static void ehci_register_types(void)
 {
-    type_register_static(&ehci_info);
-    type_register_static(&ich9_ehci_info);
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(ehci_info); i++) {
+        type_register_static(&ehci_info[i]);
+    }
 }
 
 type_init(ehci_register_types)