[PATCH v4 12/12] hw/isa/vt82c686: Create rtc-time alias in boards instead

Bernhard Beschow posted 12 patches 3 years, 5 months ago
Maintainers: "Philippe Mathieu-Daudé" <f4bug@amsat.org>, John Snow <jsnow@redhat.com>, Huacai Chen <chenhuacai@kernel.org>, Jiaxun Yang <jiaxun.yang@flygoat.com>, BALATON Zoltan <balaton@eik.bme.hu>, Gerd Hoffmann <kraxel@redhat.com>
There is a newer version of this series
[PATCH v4 12/12] hw/isa/vt82c686: Create rtc-time alias in boards instead
Posted by Bernhard Beschow 3 years, 5 months ago
According to good QOM practice, an object should only deal with objects
of its own sub tree. Having devices create an alias on the machine
object doesn't respect this good practice. To resolve this, create the
alias in the machine's code.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/isa/vt82c686.c   | 2 --
 hw/mips/fuloong2e.c | 4 ++++
 hw/ppc/pegasos2.c   | 4 ++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 48cd4d0036..3f9bd0c04d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -632,8 +632,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
     if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
         return;
     }
-    object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(&s->rtc),
-                              "date");
     isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
 
     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 2d8723ab74..0f4cfe1188 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -203,6 +203,10 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
 
     via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
                                           TYPE_VT82C686B_ISA);
+    object_property_add_alias(qdev_get_machine(), "rtc-time",
+                              object_resolve_path_component(OBJECT(via),
+                                                            "rtc"),
+                              "date");
     qdev_connect_gpio_out(DEVICE(via), 0, intc);
 
     dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 09fdb7557f..f50e1d8b3f 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -161,6 +161,10 @@ static void pegasos2_init(MachineState *machine)
     /* VIA VT8231 South Bridge (multifunction PCI device) */
     via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
                                           TYPE_VT8231_ISA);
+    object_property_add_alias(qdev_get_machine(), "rtc-time",
+                              object_resolve_path_component(OBJECT(via),
+                                                            "rtc"),
+                              "date");
     qdev_connect_gpio_out(DEVICE(via), 0,
                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
 
-- 
2.37.3
Re: [PATCH v4 12/12] hw/isa/vt82c686: Create rtc-time alias in boards instead
Posted by BALATON Zoltan 3 years, 5 months ago
On Wed, 31 Aug 2022, Bernhard Beschow wrote:
> According to good QOM practice, an object should only deal with objects
> of its own sub tree. Having devices create an alias on the machine
> object doesn't respect this good practice. To resolve this, create the
> alias in the machine's code.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/vt82c686.c   | 2 --
> hw/mips/fuloong2e.c | 4 ++++
> hw/ppc/pegasos2.c   | 4 ++++
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 48cd4d0036..3f9bd0c04d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -632,8 +632,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>     if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>         return;
>     }
> -    object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(&s->rtc),
> -                              "date");
>     isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>
>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 2d8723ab74..0f4cfe1188 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -203,6 +203,10 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>
>     via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
>                                           TYPE_VT82C686B_ISA);
> +    object_property_add_alias(qdev_get_machine(), "rtc-time",
> +                              object_resolve_path_component(OBJECT(via),
> +                                                            "rtc"),
> +                              "date");
>     qdev_connect_gpio_out(DEVICE(via), 0, intc);
>
>     dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> index 09fdb7557f..f50e1d8b3f 100644
> --- a/hw/ppc/pegasos2.c
> +++ b/hw/ppc/pegasos2.c
> @@ -161,6 +161,10 @@ static void pegasos2_init(MachineState *machine)
>     /* VIA VT8231 South Bridge (multifunction PCI device) */
>     via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
>                                           TYPE_VT8231_ISA);
> +    object_property_add_alias(qdev_get_machine(), "rtc-time",

I did not check it in previous version but Phillppe noted this 
qdev_get_machine() should be machine (the parameter to pegasos2_init) 
instead and I agree with that.

Also if you get rid of the now very much cut down 
vt82c686b_southbridge_init func in fuloong2e and just inline what's left 
of it at the only call site then the same machine pointer could be used 
there too and would be simpler then going through the function now that 
it's moved to via-isa mostly.

Sorry that this needs another respin but that's the last, I won't look at 
it again :-) You can also add to the whole series:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Regards,
BALATON Zoltan

> +                              object_resolve_path_component(OBJECT(via),
> +                                                            "rtc"),
> +                              "date");
>     qdev_connect_gpio_out(DEVICE(via), 0,
>                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>
>
Re: [PATCH v4 12/12] hw/isa/vt82c686: Create rtc-time alias in boards instead
Posted by Bernhard Beschow 3 years, 5 months ago
Am 31. August 2022 16:30:10 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 31 Aug 2022, Bernhard Beschow wrote:
>> According to good QOM practice, an object should only deal with objects
>> of its own sub tree. Having devices create an alias on the machine
>> object doesn't respect this good practice. To resolve this, create the
>> alias in the machine's code.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/isa/vt82c686.c   | 2 --
>> hw/mips/fuloong2e.c | 4 ++++
>> hw/ppc/pegasos2.c   | 4 ++++
>> 3 files changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 48cd4d0036..3f9bd0c04d 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -632,8 +632,6 @@ static void via_isa_realize(PCIDevice *d, Error **errp)
>>     if (!qdev_realize(DEVICE(&s->rtc), BUS(isa_bus), errp)) {
>>         return;
>>     }
>> -    object_property_add_alias(qdev_get_machine(), "rtc-time", OBJECT(&s->rtc),
>> -                              "date");
>>     isa_connect_gpio_out(ISA_DEVICE(&s->rtc), 0, s->rtc.isairq);
>> 
>>     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 2d8723ab74..0f4cfe1188 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -203,6 +203,10 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>> 
>>     via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
>>                                           TYPE_VT82C686B_ISA);
>> +    object_property_add_alias(qdev_get_machine(), "rtc-time",
>> +                              object_resolve_path_component(OBJECT(via),
>> +                                                            "rtc"),
>> +                              "date");
>>     qdev_connect_gpio_out(DEVICE(via), 0, intc);
>> 
>>     dev = PCI_DEVICE(object_resolve_path_component(OBJECT(via), "ide"));
>> diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
>> index 09fdb7557f..f50e1d8b3f 100644
>> --- a/hw/ppc/pegasos2.c
>> +++ b/hw/ppc/pegasos2.c
>> @@ -161,6 +161,10 @@ static void pegasos2_init(MachineState *machine)
>>     /* VIA VT8231 South Bridge (multifunction PCI device) */
>>     via = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(12, 0), true,
>>                                           TYPE_VT8231_ISA);
>> +    object_property_add_alias(qdev_get_machine(), "rtc-time",
>
>I did not check it in previous version but Phillppe noted this qdev_get_machine() should be machine (the parameter to pegasos2_init) instead and I agree with that.

Sounds good! I'm all about removing access to globals.

>Also if you get rid of the now very much cut down vt82c686b_southbridge_init func in fuloong2e and just inline what's left of it at the only call site then the same machine pointer could be used there too and would be simpler then going through the function now that it's moved to via-isa mostly.

Sure, I'll add another patch on top.

>Sorry that this needs another respin but that's the last, I won't look at it again :-)

No worries. It's very convenient with git-publish.

>You can also add to the whole series:
>
>Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Will do. Thanks for your quick replies!

Regards,
Bernhard

>Regards,
>BALATON Zoltan
>
>> +                              object_resolve_path_component(OBJECT(via),
>> +                                                            "rtc"),
>> +                              "date");
>>     qdev_connect_gpio_out(DEVICE(via), 0,
>>                           qdev_get_gpio_in_named(pm->mv, "gpp", 31));
>> 
>>