Patchwork [2/5] musicpal: qdevify musicpal-misc

login
register
mail settings
Submitter Peter Maydell
Date Feb. 15, 2013, 11:45 a.m.
Message ID <1360928706-13041-3-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/220724/
State New
Headers show

Comments

Peter Maydell - Feb. 15, 2013, 11:45 a.m.
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(-)
Andreas Färber - Feb. 15, 2013, 1:11 p.m.
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
Peter Maydell - Feb. 15, 2013, 1:38 p.m.
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
Andreas Färber - Feb. 15, 2013, 1:45 p.m.
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
Peter Maydell - Feb. 15, 2013, 1:48 p.m.
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
Paolo Bonzini - Feb. 15, 2013, 3:14 p.m.
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
Andreas Färber - Feb. 15, 2013, 3:39 p.m.
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
Peter Maydell - Feb. 15, 2013, 4:04 p.m.
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

Patch

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)