| Message ID | 1360928706-13041-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
Am 15.02.2013 12:45, schrieb Peter Maydell: > Make musicpal-misc into its own (trivial) qdev device, so we > can get rid of the abuse of sysbus_add_memory(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/musicpal.c | 34 +++++++++++++++++++++++++++++----- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/hw/musicpal.c b/hw/musicpal.c > index 272cb80..819e420 100644 > --- a/hw/musicpal.c > +++ b/hw/musicpal.c > @@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = { > > #define MP_BOARD_REVISION 0x31 > > +typedef struct { Anonymous struct > + SysBusDevice busdev; parent_obj please. :) > + MemoryRegion iomem; > +} MusicPalMiscState; > + > +typedef SysBusDeviceClass MusicPalMiscClass; Please don't. Either define a struct and use it for .class_size or drop the typedef. > + > +#define TYPE_MUSICPAL_MISC "musicpal-misc" > +#define MUSICPAL_MISC(obj) \ > + OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC) > +#define MUSICPAL_MISC_CLASS(klass) \ > + OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC) > +#define MUSICPAL_MISC_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC) If we don't have such a class so you can just drop these two. SYS_BUS_DEVICE_[GET_]CLASS() should be used instead when needed. > + > static uint64_t musicpal_misc_read(void *opaque, hwaddr offset, > unsigned size) > { > @@ -1054,15 +1069,23 @@ static const MemoryRegionOps musicpal_misc_ops = { > .endianness = DEVICE_NATIVE_ENDIAN, > }; > > -static void musicpal_misc_init(SysBusDevice *dev) > +static void musicpal_misc_init(Object *obj) > { > - MemoryRegion *iomem = g_new(MemoryRegion, 1); > + SysBusDevice *sd = SYS_BUS_DEVICE(obj); > + MusicPalMiscState *s = MUSICPAL_MISC(obj); > > - memory_region_init_io(iomem, &musicpal_misc_ops, NULL, > + memory_region_init_io(&s->iomem, &musicpal_misc_ops, NULL, > "musicpal-misc", MP_MISC_SIZE); > - sysbus_add_memory(dev, MP_MISC_BASE, iomem); > + sysbus_init_mmio(sd, &s->iomem); > } > > +static const TypeInfo musicpal_misc_info = { > + .name = TYPE_MUSICPAL_MISC, > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_init = musicpal_misc_init, > + .instance_size = sizeof(MusicPalMiscState), > +}; > + > /* WLAN register offsets */ > #define MP_WLAN_MAGIC1 0x11c > #define MP_WLAN_MAGIC2 0x124 > @@ -1612,7 +1635,7 @@ static void musicpal_init(QEMUMachineInitArgs *args) > > sysbus_create_simple("mv88w8618_wlan", MP_WLAN_BASE, NULL); > > - musicpal_misc_init(SYS_BUS_DEVICE(dev)); > + sysbus_create_simple(TYPE_MUSICPAL_MISC, MP_MISC_BASE, NULL); > > dev = sysbus_create_simple("musicpal_gpio", MP_GPIO_BASE, pic[MP_GPIO_IRQ]); > i2c_dev = sysbus_create_simple("gpio_i2c", -1, NULL); > @@ -1692,6 +1715,7 @@ static void musicpal_register_types(void) > type_register_static(&musicpal_lcd_info); > type_register_static(&musicpal_gpio_info); > type_register_static(&musicpal_key_info); > + type_register_static(&musicpal_misc_info); > } > > type_init(musicpal_register_types) Otherwise looks very good with instance_init! Andreas
On 15 February 2013 13:11, Andreas Färber <afaerber@suse.de> wrote: > Am 15.02.2013 12:45, schrieb Peter Maydell: >> --- a/hw/musicpal.c >> +++ b/hw/musicpal.c >> @@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = { >> >> #define MP_BOARD_REVISION 0x31 >> >> +typedef struct { > > Anonymous struct No, it's a typedef. Why would you want to name the struct particularly when it's an error to then use that name rather than the typedef? Better to let the compiler make uses of 'struct Foo' rather than 'Foo' a compile failure. >> + SysBusDevice busdev; > > parent_obj please. :) > >> + MemoryRegion iomem; >> +} MusicPalMiscState; >> + > >> +typedef SysBusDeviceClass MusicPalMiscClass; > > Please don't. Either define a struct and use it for .class_size or drop > the typedef. So my rationale here was "all classes should have a FooClass". If you have classes which don't have a FooClass then if at some later point you need to introduce a class struct you have to go round and locate all the places you wrote ParentClass and now need to change it to FooClass. If we always have a typedef everywhere then there is never a problem. More generally, I think we should prefer to avoid the kind of shortcut that the C implementation allows us to take. If we define a QOM class then that should mean you get the full range of expected things (a TYPE_FOO, a FOO macro, a FOO_CLASS and a FooClass type). If you prefer we could standardize on typedef struct { ParentClass parent; } FooClass; rather than typedef ParentClass FooClass; >> + >> +#define TYPE_MUSICPAL_MISC "musicpal-misc" >> +#define MUSICPAL_MISC(obj) \ >> + OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC) > >> +#define MUSICPAL_MISC_CLASS(klass) \ >> + OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC) >> +#define MUSICPAL_MISC_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC) > > If we don't have such a class so you can just drop these two. > SYS_BUS_DEVICE_[GET_]CLASS() should be used instead when needed. Again, this will then require rework if we ever actually need to put anything in the currently (conceptually) empty class struct. -- PMM
Am 15.02.2013 14:38, schrieb Peter Maydell: > On 15 February 2013 13:11, Andreas Färber <afaerber@suse.de> wrote: >> Am 15.02.2013 12:45, schrieb Peter Maydell: >>> --- a/hw/musicpal.c >>> +++ b/hw/musicpal.c >>> @@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = { >>> >>> #define MP_BOARD_REVISION 0x31 >>> >>> +typedef struct { >> >> Anonymous struct > > No, it's a typedef. Why would you want to name the struct > particularly when it's an error to then use that name rather > than the typedef? Better to let the compiler make uses of > 'struct Foo' rather than 'Foo' a compile failure. I'm pretty sure it has been requested by Blue and Anthony in the past... Not sure if it makes a difference for gdb or something. > >>> + SysBusDevice busdev; >> >> parent_obj please. :) >> >>> + MemoryRegion iomem; >>> +} MusicPalMiscState; >>> + >> >>> +typedef SysBusDeviceClass MusicPalMiscClass; >> >> Please don't. Either define a struct and use it for .class_size or drop >> the typedef. > > So my rationale here was "all classes should have a FooClass". > If you have classes which don't have a FooClass then if at > some later point you need to introduce a class struct you > have to go round and locate all the places you wrote > ParentClass and now need to change it to FooClass. If > we always have a typedef everywhere then there is never > a problem. > > More generally, I think we should prefer to avoid the kind of > shortcut that the C implementation allows us to take. If we > define a QOM class then that should mean you get the full range > of expected things (a TYPE_FOO, a FOO macro, a FOO_CLASS and > a FooClass type). > > If you prefer we could standardize on > typedef struct { > ParentClass parent; > } FooClass; > > rather than typedef ParentClass FooClass; > >>> + >>> +#define TYPE_MUSICPAL_MISC "musicpal-misc" >>> +#define MUSICPAL_MISC(obj) \ >>> + OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC) >> >>> +#define MUSICPAL_MISC_CLASS(klass) \ >>> + OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC) >>> +#define MUSICPAL_MISC_GET_CLASS(obj) \ >>> + OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC) >> >> If we don't have such a class so you can just drop these two. >> SYS_BUS_DEVICE_[GET_]CLASS() should be used instead when needed. > > Again, this will then require rework if we ever actually need > to put anything in the currently (conceptually) empty class struct. It may need rework either way. Because aliasing via typedef gives FooClass fields it will loose once it is turned into a real QOM class. We had such an issue with i440fx in my PHB series, that's why I'm sensitive to it. ;) Andreas
On 15 February 2013 13:45, Andreas Färber <afaerber@suse.de> wrote: > Am 15.02.2013 14:38, schrieb Peter Maydell: >> On 15 February 2013 13:11, Andreas Färber <afaerber@suse.de> wrote: >>> Am 15.02.2013 12:45, schrieb Peter Maydell: >>>> --- a/hw/musicpal.c >>>> +++ b/hw/musicpal.c >>>> @@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = { >>>> >>>> #define MP_BOARD_REVISION 0x31 >>>> >>>> +typedef struct { >>> >>> Anonymous struct >> >> No, it's a typedef. Why would you want to name the struct >> particularly when it's an error to then use that name rather >> than the typedef? Better to let the compiler make uses of >> 'struct Foo' rather than 'Foo' a compile failure. > > I'm pretty sure it has been requested by Blue and Anthony in the past... > Not sure if it makes a difference for gdb or something. I've cc'd them. But unless somebody has an actual good reason for giving the struct in the typedef a completely unnecessary name I think leaving it unnamed is better. (This is distinct from genuinely anonymous structs with no 'struct foo' name or typedef name, which we do have a few of in the codebase. I agree those are better avoided.) >>>> +typedef SysBusDeviceClass MusicPalMiscClass; >>> >>> Please don't. Either define a struct and use it for .class_size or drop >>> the typedef. >> >> So my rationale here was "all classes should have a FooClass". >> If you have classes which don't have a FooClass then if at >> some later point you need to introduce a class struct you >> have to go round and locate all the places you wrote >> ParentClass and now need to change it to FooClass. If >> we always have a typedef everywhere then there is never >> a problem. >> >> More generally, I think we should prefer to avoid the kind of >> shortcut that the C implementation allows us to take. If we >> define a QOM class then that should mean you get the full range >> of expected things (a TYPE_FOO, a FOO macro, a FOO_CLASS and >> a FooClass type). >> >> If you prefer we could standardize on >> typedef struct { >> ParentClass parent; >> } FooClass; >> >> rather than typedef ParentClass FooClass; > It may need rework either way. Because aliasing via typedef gives > FooClass fields it will loose once it is turned into a real QOM class. > We had such an issue with i440fx in my PHB series, that's why I'm > sensitive to it. ;) OK, so that seems like an argument for always defining the empty-except-for-the-parentclass class struct, or does that run into problems too? -- PMM
Il 15/02/2013 14:48, Peter Maydell ha scritto: >>> >> If you prefer we could standardize on >>> >> typedef struct { >>> >> ParentClass parent; >>> >> } FooClass; >>> >> >>> >> rather than typedef ParentClass FooClass; >> > It may need rework either way. Because aliasing via typedef gives >> > FooClass fields it will loose once it is turned into a real QOM class. >> > We had such an issue with i440fx in my PHB series, that's why I'm >> > sensitive to it. ;) > OK, so that seems like an argument for always defining the > empty-except-for-the-parentclass class struct, or does that > run into problems too? I like the empty-except-for-parentclass. OTOH, if you have no fields you will not use FOO_CLASS. You will only use PARENT_CLASS, and those uses will be fine even after you start having a FooClass. So, having no typedef and no _CLASS macros at all is simple and should be acceptable. But if you have a typedef, you should a) make it a struct, b) add the _CLASS macros. Paolo
Am 15.02.2013 16:14, schrieb Paolo Bonzini: > Il 15/02/2013 14:48, Peter Maydell ha scritto: >>>>>> If you prefer we could standardize on >>>>>> typedef struct { >>>>>> ParentClass parent; >>>>>> } FooClass; >>>>>> >>>>>> rather than typedef ParentClass FooClass; >>>> It may need rework either way. Because aliasing via typedef gives >>>> FooClass fields it will loose once it is turned into a real QOM class. >>>> We had such an issue with i440fx in my PHB series, that's why I'm >>>> sensitive to it. ;) >> OK, so that seems like an argument for always defining the >> empty-except-for-the-parentclass class struct, or does that >> run into problems too? > > I like the empty-except-for-parentclass. OTOH, if you have no fields > you will not use FOO_CLASS. You will only use PARENT_CLASS, and those > uses will be fine even after you start having a FooClass. > > So, having no typedef and no _CLASS macros at all is simple and should > be acceptable. > > But if you have a typedef, you should a) make it a struct, b) add the > _CLASS macros. c) use it in TypeInfo, otherwise it's moot. :) A related topic where having classes prepared may make sense is in converting less-simple-than-SysBusDevice types to QOM realize. Device Foo may need to call its parent's realize function then, so should save it in its class, which it may not have yet. However since this is IMO (compared to what other people have already complained about) unnecessary boilerplate code, I'm more in favor of an as-needed approach. I.e. if we don't need a struct, don't require to define it nor macros to access it. But surely there's nothing wrong with adding more structs/macros on a voluntary basis as long as they are consistent. Andreas
On 15 February 2013 15:14, Paolo Bonzini <pbonzini@redhat.com> wrote: > I like the empty-except-for-parentclass. OTOH, if you have no fields > you will not use FOO_CLASS. You will only use PARENT_CLASS, and those > uses will be fine even after you start having a FooClass. OK, that makes sense I think, except there is one case where you do need a FooClass&c even if you have no class fields, which is when you can yourself be subclassed. You want to provide the subclasses with all the types etc they need so they don't change if you have to add a class field yourself in future. I wrote this up for the wiki page: http://wiki.qemu.org/QOMConventions ===begin=== If your class (a) will be subclassed or (b) has member fields it needs to put in its class struct then you should write all of: * a <code>FOO_CLASS</code> macro * a <code>FOO_GET_CLASS</code> macro * a FooClass structure definition containing at least the parent class field: typedef struct { /*< private >*/ MyParentClass parent_class; /*< public >*/ [any fields you need] } FooClass; * and your <code>TypeInfo</code> for this class should set the <code>.class_size</code> field to <code>sizeof(FooClass)</code>. These ensure that nothing in future should need changing if new fields are added to your class struct, and that any subclasses have the correct typenames available so they won't need to change either even if your implementation changes. If your class meets neither of the above requirements (ie it is a simple leaf class) then: * don't provide <code>FOO_CLASS</code> or <code>FOO_GET_CLASS</code> * don't provide a FooClass structure * leave the <code>TypeInfo</code>'s <code>.class_size</code> field unset. If a change means a class which didn't provide these macros/types now needs to provide them, then your change should add all of them (ie move the class from the latter category to the former). ===endit=== If that has some agreement I'll take the 'caution' label off it :-) and update this patch to match, ie remove the class macros and typedef. -- PMM
diff --git a/hw/musicpal.c b/hw/musicpal.c index 272cb80..819e420 100644 --- a/hw/musicpal.c +++ b/hw/musicpal.c @@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = { #define MP_BOARD_REVISION 0x31 +typedef struct { + SysBusDevice busdev; + MemoryRegion iomem; +} MusicPalMiscState; + +typedef SysBusDeviceClass MusicPalMiscClass; + +#define TYPE_MUSICPAL_MISC "musicpal-misc" +#define MUSICPAL_MISC(obj) \ + OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC) +#define MUSICPAL_MISC_CLASS(klass) \ + OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC) +#define MUSICPAL_MISC_GET_CLASS(obj) \ + OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC) + static uint64_t musicpal_misc_read(void *opaque, hwaddr offset, unsigned size) { @@ -1054,15 +1069,23 @@ static const MemoryRegionOps musicpal_misc_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -static void musicpal_misc_init(SysBusDevice *dev) +static void musicpal_misc_init(Object *obj) { - MemoryRegion *iomem = g_new(MemoryRegion, 1); + SysBusDevice *sd = SYS_BUS_DEVICE(obj); + MusicPalMiscState *s = MUSICPAL_MISC(obj); - memory_region_init_io(iomem, &musicpal_misc_ops, NULL, + memory_region_init_io(&s->iomem, &musicpal_misc_ops, NULL, "musicpal-misc", MP_MISC_SIZE); - sysbus_add_memory(dev, MP_MISC_BASE, iomem); + sysbus_init_mmio(sd, &s->iomem); } +static const TypeInfo musicpal_misc_info = { + .name = TYPE_MUSICPAL_MISC, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_init = musicpal_misc_init, + .instance_size = sizeof(MusicPalMiscState), +}; + /* WLAN register offsets */ #define MP_WLAN_MAGIC1 0x11c #define MP_WLAN_MAGIC2 0x124 @@ -1612,7 +1635,7 @@ static void musicpal_init(QEMUMachineInitArgs *args) sysbus_create_simple("mv88w8618_wlan", MP_WLAN_BASE, NULL); - musicpal_misc_init(SYS_BUS_DEVICE(dev)); + sysbus_create_simple(TYPE_MUSICPAL_MISC, MP_MISC_BASE, NULL); dev = sysbus_create_simple("musicpal_gpio", MP_GPIO_BASE, pic[MP_GPIO_IRQ]); i2c_dev = sysbus_create_simple("gpio_i2c", -1, NULL); @@ -1692,6 +1715,7 @@ static void musicpal_register_types(void) type_register_static(&musicpal_lcd_info); type_register_static(&musicpal_gpio_info); type_register_static(&musicpal_key_info); + type_register_static(&musicpal_misc_info); } type_init(musicpal_register_types)
Make musicpal-misc into its own (trivial) qdev device, so we can get rid of the abuse of sysbus_add_memory(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/musicpal.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-)