| Message ID | e0b8c06e00ab35dad06f672fb266088bff5028ac.1351473902.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
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) >
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 >
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)
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)
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(-)