1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()

Li Qiang posted 1 patch 6 years, 11 months ago
Test asan passed
Test checkpatch passed
Test docker-quick@centos7 passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1542880825-2604-1-git-send-email-liq3ea@gmail.com
hw/arm/musicpal.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[Qemu-devel] [PATCH] hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()
Posted by Li Qiang 6 years, 11 months ago 
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 <liq3ea@gmail.com>
---
 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);
     qdev_init_nofail(dev);
     sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
     sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);
-- 
2.11.0
Re: [Qemu-devel] [PATCH] hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()
Posted by Philippe Mathieu-Daudé 6 years, 11 months ago 
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 <liq3ea@gmail.com>
> ---
>  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, 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.

>      qdev_init_nofail(dev);
>      sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
>      sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);
>
Re: [Qemu-devel] [PATCH] hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()
Posted by Peter Maydell 6 years, 11 months ago 
On 22 November 2018 at 10:35, Philippe Mathieu-Daudé <philmd@redhat.com> 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 <liq3ea@gmail.com>
>> ---
>>  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, 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.

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

thanks
-- PMM
Re: [Qemu-devel] [PATCH] hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()
Posted by Li Qiang 6 years, 11 months ago 
Peter Maydell <peter.maydell@linaro.org> 于2018年11月22日周四 下午6:38写道：

> On 22 November 2018 at 10:35, Philippe Mathieu-Daudé <philmd@redhat.com>
> 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 <liq3ea@gmail.com>
> >> ---
> >>  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.



> > ---
> >
> > 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


> thanks
> -- PMM
>
Re: [Qemu-devel] [PATCH] hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()
Posted by Philippe Mathieu-Daudé 6 years, 11 months ago 
On 22/11/18 11:40, Li Qiang wrote:
> Peter Maydell <peter.maydell@linaro.org
> <mailto:peter.maydell@linaro.org>> 于2018年11月22日周四 下午6:38写道：
> 
>     On 22 November 2018 at 10:35, Philippe Mathieu-Daudé
>     <philmd@redhat.com <mailto:philmd@redhat.com>> 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 <liq3ea@gmail.com <mailto:liq3ea@gmail.com>>
>     >> ---
>     >>  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.
> 
>  
> 
>     > ---
>     >
>     > 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é <philmd@redhat.com>

Regards,

Phil.

> 
> 
> Agree.
> 
> Thanks,
> Li Qiang
>  
> 
>     thanks
>     -- PMM
>
Re: [Qemu-devel] [PATCH] hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link()
Posted by Peter Maydell 6 years, 11 months ago 
On Thu, 22 Nov 2018 at 10:00, Li Qiang <liq3ea@gmail.com> 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.
>
> Signed-off-by: Li Qiang <liq3ea@gmail.com>
> ---
>  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);
>      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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.