hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()

PATCH Applied Li Qiang 3 participants 6 messages Nov 22, 2018 – Nov 30, 2018

Share & tools

Li Qiang

author

2018-11-22 10:04 UTC #0 lore raw

PATCH 1/1 hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()

The third argument of object_property_set_link() is the name of property, not related with the QOM type name, using the constant string instead.

Signed-off-by Li Qiang <[email protected]>

---
 hw/arm/musicpal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/arm/musicpal.c +1 -1

index 9648b3af44..726ae29394 100644
@@ -1696,7 +1696,7 @@ static void musicpal_init(MachineState *machine)
     dev = qdev_create(NULL, TYPE_MV88W8618_AUDIO);
     s = SYS_BUS_DEVICE(dev);
     object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
-                             TYPE_WM8750, NULL);
+                             "wm8750", NULL);
     qdev_init_nofail(dev);
     sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
     sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);

Signature · 1 line

2.11.0

Philippe Mathieu-Daudé

2018-11-22 10:35 UTC #1 lore raw

Hi Li,

Quoted 3 lines · Li Qiang

The third argument of object_property_set_link() is the name of property, not related with the QOM type name, using the constant string instead.

You are correct.

Quoted 11 lines

Signed-off-by Li Qiang <[email protected]>
---
 hw/arm/musicpal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/arm/musicpal.c +0 -0
index 9648b3af44..726ae29394 100644

hw/arm/musicpal.c static void musicpal_init(MachineState *machine) · line 1699

Quoted 5 lines

hw/arm/musicpal.c +1 -1

@@ -1696,7 +1696,7 @@ static void musicpal_init(MachineState *machine)
     dev = qdev_create(NULL, TYPE_MV88W8618_AUDIO);
     s = SYS_BUS_DEVICE(dev);
     object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
-                             TYPE_WM8750, NULL);
+                             "wm8750", NULL);

Since this property is not related to migration, maybe we can resolve this the other way, using TYPE_WM8750 in mv88w8618_audio_init():

-- >8 --
@@ -255,3 +255,3 @@ static void mv88w8618_audio_init(Object *obj)

- object_property_add_link(OBJECT(dev), "wm8750", TYPE_WM8750,
+ object_property_add_link(OBJECT(dev), TYPE_WM8750, TYPE_WM8750,

                              (Object **) &s->wm,

---

So using the same definition would protect someone to accidentaly rename the property name.

What do you think?

Regards,

Phil.

Quoted 4 lines

hw/arm/musicpal.c +0 -0

     qdev_init_nofail(dev);
     sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
     sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);

Peter Maydell

2018-11-22 10:38 UTC #2 lore raw

Quoted 37 lines · Philippe Mathieu-Daudé

Hi Li,
Quoted 3 lines · Li Qiang
The third argument of object_property_set_link() is the name of property, not related with the QOM type name, using the constant string instead.
You are correct.
Quoted 11 lines
Signed-off-by Li Qiang <[email protected]>
---
 hw/arm/musicpal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/arm/musicpal.c +0 -0
index 9648b3af44..726ae29394 100644
hw/arm/musicpal.c static void musicpal_init(MachineState *machine) · line 1699
Quoted 5 lines
hw/arm/musicpal.c +1 -1
@@ -1696,7 +1696,7 @@ static void musicpal_init(MachineState *machine)
     dev = qdev_create(NULL, TYPE_MV88W8618_AUDIO);
     s = SYS_BUS_DEVICE(dev);
     object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
-                             TYPE_WM8750, NULL);
+                             "wm8750", NULL);
Since this property is not related to migration, maybe we can resolve this the other way, using TYPE_WM8750 in mv88w8618_audio_init():
-- >8 --
(quoted diff) +1 -2
@@ -255,3 +255,3 @@ static void mv88w8618_audio_init(Object *obj)
 
-    object_property_add_link(OBJECT(dev), "wm8750", TYPE_WM8750,
+    object_property_add_link(OBJECT(dev), TYPE_WM8750, TYPE_WM8750,
                              (Object **) &s->wm,
---

Quoted 2 lines

So using the same definition would protect someone to accidentaly rename the property name.

I think I prefer Li's patch -- the property name is just an arbitrary name not related to the type name necessarily.

thanks
-- PMM

Li Qiang

author

2018-11-22 10:41 UTC #3 lore raw

Quoted 17 lines · Peter Maydell <[email protected]> 于2018年11月22日周四下午6:38写道：

Quoted 15 lines · Philippe Mathieu-Daudé
Hi Li,
Quoted 3 lines · Li Qiang
The third argument of object_property_set_link() is the name of property, not related with the QOM type name, using the constant string instead.
You are correct.
Quoted 6 lines
Signed-off-by Li Qiang <[email protected]>
---
 hw/arm/musicpal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

Quoted 4 lines

Quoted 4 lines
hw/arm/musicpal.c +0 -0
index 9648b3af44..726ae29394 100644

Quoted 8 lines

hw/arm/musicpal.c static void musicpal_init(MachineState *machine) · line 1699

Quoted 6 lines

hw/arm/musicpal.c +1 -1

@@ -1696,7 +1696,7 @@ static void musicpal_init(MachineState *machine)
     dev = qdev_create(NULL, TYPE_MV88W8618_AUDIO);
     s = SYS_BUS_DEVICE(dev);
     object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
-                             TYPE_WM8750, NULL);
+                             "wm8750", NULL);

Since this property is not related to migration,

This property is not related with migration.

Quoted 10 lines

maybe we can resolve
Review static void mv88w8618_audio_init(Object *obj) · line 257
Quoted 8 lines
this the other way, using TYPE_WM8750 in mv88w8618_audio_init():
-- >8 --
(quoted diff) +1 -1
@@ -255,3 +255,3 @@ static void mv88w8618_audio_init(Object *obj)
 
-    object_property_add_link(OBJECT(dev), "wm8750", TYPE_WM8750,
+    object_property_add_link(OBJECT(dev), TYPE_WM8750, TYPE_WM8750,
                              (Object **) &s->wm,

No, just as Peter point out, the property can be arbitrary name, here use 'TYPE_WM8750'
makes confusion.

Quoted 9 lines

Quoted 4 lines
---
So using the same definition would protect someone to accidentaly rename the property name.
I think I prefer Li's patch -- the property name is just an arbitrary name not related to the type name necessarily.

Agree.

Thanks,
Li Qiang

Quoted 3 lines

thanks
-- PMM

Philippe Mathieu-Daudé

Reviewed-by

2018-11-22 11:04 UTC #4 lore raw

Quoted 55 lines · Li Qiang

Peter Maydell <[email protected] <mailto:[email protected]>> 于2018年11月22日周四下午6:38写道：

    On 22 November 2018 at 10:35, Philippe Mathieu-Daudé
    <[email protected] <mailto:[email protected]>> wrote:
    > Hi Li,
    >
    > On 22/11/18 11:00, Li Qiang wrote:
    >> The third argument of object_property_set_link() is the name of
    >> property, not related with the QOM type name, using the constant
    >> string instead.
    >
    > You are correct.
    >
    >>
    >> Signed-off-by: Li Qiang <[email protected] <mailto:[email protected]>>
    >> ---
    >>  hw/arm/musicpal.c | 2 +-
    >>  1 file changed, 1 insertion(+), 1 deletion(-)
    >>
    >> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
    >> index 9648b3af44..726ae29394 100644
    >> --- a/hw/arm/musicpal.c
    >> +++ b/hw/arm/musicpal.c
    >> @@ -1696,7 +1696,7 @@ static void musicpal_init(MachineState
    *machine)
    >>      dev = qdev_create(NULL, TYPE_MV88W8618_AUDIO);
    >>      s = SYS_BUS_DEVICE(dev);
    >>      object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
    >> -                             TYPE_WM8750, NULL);
    >> +                             "wm8750", NULL);
    >
    > Since this property is not related to migration,

This property is not related with migration.

    maybe we can resolve
    > this the other way, using TYPE_WM8750 in mv88w8618_audio_init():
    >
    > -- >8 --
    > @@ -255,3 +255,3 @@ static void mv88w8618_audio_init(Object *obj)
    >
    > -    object_property_add_link(OBJECT(dev), "wm8750", TYPE_WM8750,
    > +    object_property_add_link(OBJECT(dev), TYPE_WM8750, TYPE_WM8750,
    >                               (Object **) &s->wm,

No, just as Peter point out, the property can be arbitrary name, here use 'TYPE_WM8750'
makes confusion.

Quoted 8 lines

Quoted 3 lines
---
So using the same definition would protect someone to accidentaly
rename
> the property name.
I think I prefer Li's patch -- the property name is just an arbitrary name not related to the type name necessarily.

OK, fine then!

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Regards,

Phil.

Quoted 11 lines

Agree.
Thanks,
Li Qiang
    thanks
    -- PMM

Peter Maydell

Applied

2018-11-30 12:12 UTC #5 lore raw

In reply to Li Qiang

Quoted 15 lines · Li Qiang

The third argument of object_property_set_link() is the name of property, not related with the QOM type name, using the constant string instead.
Signed-off-by Li Qiang <[email protected]>
---
 hw/arm/musicpal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/arm/musicpal.c +0 -0
index 9648b3af44..726ae29394 100644

hw/arm/musicpal.c static void musicpal_init(MachineState *machine)

Quoted 9 lines

hw/arm/musicpal.c +1 -2

@@ -1696,7 +1696,7 @@ static void musicpal_init(MachineState *machine)
     dev = qdev_create(NULL, TYPE_MV88W8618_AUDIO);
     s = SYS_BUS_DEVICE(dev);
     object_property_set_link(OBJECT(dev), OBJECT(wm8750_dev),
-                             TYPE_WM8750, NULL);
+                             "wm8750", NULL);
     qdev_init_nofail(dev);
     sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
     sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);
--

Applied to target-arm.next for 4.0, thanks.

-- PMM