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

[BALATON Zoltan]

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.

Regards,
BALATON Zoltan

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