Re: [PATCH v2 08/15] hw/pci-bridge: Extract QOM ICH definitions to 'ich9_dmi.h'

[BALATON Zoltan]

On Mon, 26 Feb 2024, Philippe Mathieu-Daudé wrote:
> Expose TYPE_ICH_DMI_PCI_BRIDGE to the new
> "hw/pci-bridge/ich9_dmi.h" header.

Since this is effectively an empty object (that's not even instantiated by 
default) I still think that instead of adding even more files for it all 
this could just be moved to hw/isa/lpc_ich9.c and define there as an 
internal object and drop the OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, 
ICH_DMI_PCI_BRIDGE) and just use the size of the superclass as it's 
instance size. That just adds the realize function and a type definition 
and gets rid of boilerplate scattered around the source tree which just 
adds complexity for no reason. But I don't care too much about it, just 
wanted to say again that if something can be kept simple I'd prefer that 
over making it more complex and for this device it looks already too 
complex for what it does or used for.

Regards,
BALATON Zoltan

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> MAINTAINERS                               |  1 +
> include/hw/pci-bridge/ich9_dmi.h          | 20 ++++++++++++++++++++
> include/hw/southbridge/ich9.h             |  2 --
> hw/pci-bridge/{i82801b11.c => ich9_dmi.c} | 11 ++++-------
> hw/pci-bridge/meson.build                 |  2 +-
> 5 files changed, 26 insertions(+), 10 deletions(-)
> create mode 100644 include/hw/pci-bridge/ich9_dmi.h
> rename hw/pci-bridge/{i82801b11.c => ich9_dmi.c} (95%)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0849283287..52282c680e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2618,6 +2618,7 @@ F: hw/i2c/smbus_ich9.c
> F: hw/isa/lpc_ich9.c
> F: include/hw/acpi/ich9*.h
> F: include/hw/i2c/ich9_smbus.h
> +F: include/hw/pci-bridge/ich9_dmi.h
> F: include/hw/southbridge/ich9.h
>
> PIIX4 South Bridge (i82371AB)
> diff --git a/include/hw/pci-bridge/ich9_dmi.h b/include/hw/pci-bridge/ich9_dmi.h
> new file mode 100644
> index 0000000000..7cf5d9d9b2
> --- /dev/null
> +++ b/include/hw/pci-bridge/ich9_dmi.h
> @@ -0,0 +1,20 @@
> +/*
> + * QEMU ICH4 i82801b11 dmi-to-pci Bridge Emulation
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_PCI_BRIDGE_ICH9_DMI_H
> +#define HW_PCI_BRIDGE_ICH9_DMI_H
> +
> +#include "qom/object.h"
> +#include "hw/pci/pci_bridge.h"
> +
> +#define TYPE_ICH_DMI_PCI_BRIDGE "i82801b11-bridge"
> +OBJECT_DECLARE_SIMPLE_TYPE(I82801b11Bridge, ICH_DMI_PCI_BRIDGE)
> +
> +struct I82801b11Bridge {
> +    PCIBridge parent_obj;
> +};
> +
> +#endif
> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
> index bee522a4cf..b2abf483e0 100644
> --- a/include/hw/southbridge/ich9.h
> +++ b/include/hw/southbridge/ich9.h
> @@ -114,8 +114,6 @@ struct ICH9LPCState {
>
> #define ICH9_D2P_SECONDARY_DEFAULT              (256 - 8)
>
> -#define ICH9_D2P_A2_REVISION                    0x92
> -
> /* D31:F0 LPC Processor Interface */
> #define ICH9_RST_CNT_IOPORT                     0xCF9
>
> diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/ich9_dmi.c
> similarity index 95%
> rename from hw/pci-bridge/i82801b11.c
> rename to hw/pci-bridge/ich9_dmi.c
> index c140919cbc..927e48bf2e 100644
> --- a/hw/pci-bridge/i82801b11.c
> +++ b/hw/pci-bridge/ich9_dmi.c
> @@ -45,7 +45,7 @@
> #include "hw/pci/pci_bridge.h"
> #include "migration/vmstate.h"
> #include "qemu/module.h"
> -#include "hw/southbridge/ich9.h"
> +#include "hw/pci-bridge/ich9_dmi.h"
>
> /*****************************************************************************/
> /* ICH9 DMI-to-PCI bridge */
> @@ -53,11 +53,8 @@
> #define I82801ba_SSVID_SVID     0
> #define I82801ba_SSVID_SSID     0
>
> -typedef struct I82801b11Bridge {
> -    /*< private >*/
> -    PCIBridge parent_obj;
> -    /*< public >*/
> -} I82801b11Bridge;
> +
> +#define ICH9_D2P_A2_REVISION                    0x92
>
> static void i82801b11_bridge_realize(PCIDevice *d, Error **errp)
> {
> @@ -103,7 +100,7 @@ static void i82801b11_bridge_class_init(ObjectClass *klass, 
> void *data)
> }
>
> static const TypeInfo i82801b11_bridge_info = {
> -    .name          = "i82801b11-bridge",
> +    .name          = TYPE_ICH_DMI_PCI_BRIDGE,
>     .parent        = TYPE_PCI_BRIDGE,
>     .instance_size = sizeof(I82801b11Bridge),
>     .class_init    = i82801b11_bridge_class_init,
> diff --git a/hw/pci-bridge/meson.build b/hw/pci-bridge/meson.build
> index f2a60434dd..d746487193 100644
> --- a/hw/pci-bridge/meson.build
> +++ b/hw/pci-bridge/meson.build
> @@ -1,6 +1,6 @@
> pci_ss = ss.source_set()
> pci_ss.add(files('pci_bridge_dev.c'))
> -pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('i82801b11.c'))
> +pci_ss.add(when: 'CONFIG_I82801B11', if_true: files('ich9_dmi.c'))
> pci_ss.add(when: 'CONFIG_IOH3420', if_true: files('ioh3420.c'))
> pci_ss.add(when: 'CONFIG_PCIE_PORT', if_true: files('pcie_root_port.c', 
> 'gen_pcie_root_port.c'))
> pci_ss.add(when: 'CONFIG_PCIE_PCI_BRIDGE', if_true: files('pcie_pci_bridge.c'))