Am 29.10.2012 12:43, schrieb Peter Crosthwaite: > > On Oct 29, 2012 7:35 PM, "Andreas Färber" <afaer...@suse.de > <mailto: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?
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.crosthwa...@xilinx.com > <mailto: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. 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) -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
