[Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
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.mayd...@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 {
+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)
--
1.7.9.5
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
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.mayd...@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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
Am 15.02.2013 14:38, schrieb Peter Maydell:
On 15 February 2013 13:11, Andreas Färber afaer...@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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
On 15 February 2013 13:11, Andreas Färber afaer...@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
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
On 15 February 2013 13:45, Andreas Färber afaer...@suse.de wrote:
Am 15.02.2013 14:38, schrieb Peter Maydell:
On 15 February 2013 13:11, Andreas Färber afaer...@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
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
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
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
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
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
On 15 February 2013 15:14, Paolo Bonzini pbonz...@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 FooClassc 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 codeFOO_CLASS/code macro
* a codeFOO_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 codeTypeInfo/code for this class should set the
code.class_size/code field to codesizeof(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 codeFOO_CLASS/code or codeFOO_GET_CLASS/code
* don't provide a FooClass structure
* leave the codeTypeInfo/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
