| Message ID | 1542880825-2604-1-git-send-email-liq3ea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw: arm: musicpal: drop TYPE_WM8750 in object_property_set_link() | expand |
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]); >
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
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 >
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 >
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
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]);
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(-)