Re: [PATCH 10/13] ppc4xx_pci: Add define for ppc4xx-host-bridge type name

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 3143 bytes --]

On Tue, 4 Jul 2023, Philippe Mathieu-Daudé wrote:
> On 4/7/23 00:02, BALATON Zoltan wrote:
>> Add a QOM type name define for ppc4xx-host-bridge in the common header
>> and replace direct use of the string name with the constant.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ppc/ppc440_pcix.c    | 3 ++-
>>   hw/ppc/ppc4xx_pci.c     | 4 ++--
>>   include/hw/ppc/ppc4xx.h | 1 +
>>   3 files changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
>> index f10f93c533..dfec25ac83 100644
>> --- a/hw/ppc/ppc440_pcix.c
>> +++ b/hw/ppc/ppc440_pcix.c
>> @@ -495,7 +495,8 @@ static void ppc440_pcix_realize(DeviceState *dev, Error 
>> **errp)
>>                            ppc440_pcix_map_irq, &s->irq, &s->busmem,
>>                            get_system_io(), PCI_DEVFN(0, 0), 1, 
>> TYPE_PCI_BUS);
>>   -    s->dev = pci_create_simple(h->bus, PCI_DEVFN(0, 0), 
>> "ppc4xx-host-bridge");
>> +    s->dev = pci_create_simple(h->bus, PCI_DEVFN(0, 0),
>> +                               TYPE_PPC4xx_HOST_BRIDGE);
>>         memory_region_init(&s->bm, OBJECT(s), "bm-ppc440-pcix", 
>> UINT64_MAX);
>>       memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
>> diff --git a/hw/ppc/ppc4xx_pci.c b/hw/ppc/ppc4xx_pci.c
>> index fbdf8266d8..6652119008 100644
>> --- a/hw/ppc/ppc4xx_pci.c
>> +++ b/hw/ppc/ppc4xx_pci.c
>> @@ -333,7 +333,7 @@ static void ppc4xx_pcihost_realize(DeviceState *dev, 
>> Error **errp)
>>                                 TYPE_PCI_BUS);
>>       h->bus = b;
>>   -    pci_create_simple(b, 0, "ppc4xx-host-bridge");
>> +    pci_create_simple(b, 0, TYPE_PPC4xx_HOST_BRIDGE);
>>         /* XXX split into 2 memory regions, one for config space, one for 
>> regs */
>>       memory_region_init(&s->container, OBJECT(s), "pci-container", 
>> PCI_ALL_SIZE);
>> @@ -367,7 +367,7 @@ static void ppc4xx_host_bridge_class_init(ObjectClass 
>> *klass, void *data)
>>   }
>>     static const TypeInfo ppc4xx_host_bridge_info = {
>> -    .name          = "ppc4xx-host-bridge",
>> +    .name          = TYPE_PPC4xx_HOST_BRIDGE,
>>       .parent        = TYPE_PCI_DEVICE,
>>       .instance_size = sizeof(PCIDevice),
>>       .class_init    = ppc4xx_host_bridge_class_init,
>> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
>> index e053b9751b..766d575e86 100644
>> --- a/include/hw/ppc/ppc4xx.h
>> +++ b/include/hw/ppc/ppc4xx.h
>> @@ -29,6 +29,7 @@
>>   #include "exec/memory.h"
>>   #include "hw/sysbus.h"
>>   +#define TYPE_PPC4xx_HOST_BRIDGE "ppc4xx-host-bridge"
>
> This is the function #0 of the host bridge, maybe:
>
> #define TYPE_PPC4xx_HOST_BRIDGE_FN0 "ppc4xx-pci-host-bridge-fn0"

That's way too long so I'd drop bridge from all of these and maybe name 
this ppc4xx-pci-host-fn0 or ppc4xx-pci-host-func (there are no other 
functions of the bridge so this shows this is the PCI side of it). I'd 
still go for shorted defines and not changing the string types too much. 
Would that be acceptable?

Regards,
BALATON Zoltan

>>   #define TYPE_PPC4xx_PCI_HOST "ppc4xx-pci-host"
>>   #define TYPE_PPC460EX_PCIE_HOST "ppc460ex-pcie-host"
>> 
>
>
>