Re: [PATCH v2 1/6] include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names

[Bernhard Beschow]

Am 22. Mai 2022 22:32:06 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 22 May 2022, Bernhard Beschow wrote:
>> TYPE_PIIX3_PCI_DEVICE resides there as already, so add the remaining
>> ones, too.
>>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/isa/piix3.c                | 3 ---
>> include/hw/isa/isa.h          | 2 --
>> include/hw/southbridge/piix.h | 4 ++++
>> 3 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
>> index dab901c9ad..d96ce2b788 100644
>> --- a/hw/isa/piix3.c
>> +++ b/hw/isa/piix3.c
>> @@ -35,9 +35,6 @@
>>
>> #define XEN_PIIX_NUM_PIRQS      128ULL
>>
>> -#define TYPE_PIIX3_DEVICE "PIIX3"
>> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
>> -
>> static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>> {
>>     qemu_set_irq(piix3->pic[pic_irq],
>> diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
>> index 034d706ba1..e9fa2f5cea 100644
>> --- a/include/hw/isa/isa.h
>> +++ b/include/hw/isa/isa.h
>> @@ -144,6 +144,4 @@ static inline ISABus *isa_bus_from_device(ISADevice *d)
>>     return ISA_BUS(qdev_get_parent_bus(DEVICE(d)));
>> }
>>
>> -#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
>> -
>> #endif
>> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
>> index f63f83e5c6..4d17400dfd 100644
>> --- a/include/hw/southbridge/piix.h
>> +++ b/include/hw/southbridge/piix.h
>> @@ -70,6 +70,10 @@ typedef struct PIIXState PIIX3State;
>> DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>>                          TYPE_PIIX3_PCI_DEVICE)
>>
>> +#define TYPE_PIIX3_DEVICE "PIIX3"
>> +#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
>> +#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
>
>This naming seems to go back to 9b74b190d6c so it's not directly related to 
>this series but there seems to be a bit of a confusion here that I wonder 
>could be cleaned up now. We have:
>
>#define TYPE_PIIX3_PCI_DEVICE "pci-piix3"
>and
>#define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
>
>and both of these have mismatched #define and device name. These are not user 
>creatable so so the device names don't appear anywhere (except maybe in info 
>qtree output) but this could still be confusing as normally type defines 
>should match device names. If these are the ISA bridges then maybe piix?-isa 
>is a good name so maybe we should have
>
>#define TYPE_PIIX3_ISA "piix3-isa"
>#defint TYPE_PIIX4_ISA "piix4-isa"
>
>(then also matching e.g. "via-isa") but I'm not sure changing "pci-piix3" to 
>"piix3-isa" could break anything (I don't expect changing the defines 
>themselces could cause any problem).
>
>It's just something I've noticed and brought up for consideration but I have 
>no strong opinion on it so up to you what you do with it.

Hi Zoltan,

yeah, I noticed the naming when moving the defines. I couldn't come up
with better names which were worth the effort because I can imagine
where the names are coming from. I also didn't want to go down the
rabbithole of backwards compatibility (which is a topic I haven't
covered yet). I think that *-isa is too narrow a name for the
container device since it bridges a couple of buses to PCI, but other
than that I don't have strong opinions about the names.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> +
>> PIIX3State *piix3_create(PCIBus *pci_bus, ISABus **isa_bus);
>>
>> DeviceState *piix4_create(PCIBus *pci_bus, ISABus **isa_bus, I2CBus **smbus);
>>