On Oct 29, 2012 7:35 PM, "Andreas Färber" <afaer...@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.crosthwa...@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
>
