Re: [PATCH 2/4] isa: extract FDC37M81X to a separate file

[Bernhard Beschow]

Am 29. Januar 2024 13:37:46 UTC schrieb Paolo Bonzini <pbonzini@redhat.com>:
>isa-superio.c currently defines a SuperIO chip that depends on
>CONFIG_IDE_ISA, but not all users of isa-superio.c depend on that
>symbol.  Extract the chip to a separate file so that there is an
>obvious place to select IDE_ISA.
>
>Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>---
> hw/isa/fdc37m81x-superio.c | 37 +++++++++++++++++++++++++++++++++++++
> hw/isa/isa-superio.c       | 19 -------------------
> hw/isa/Kconfig             |  5 +++++
> hw/isa/meson.build         |  1 +
> hw/mips/Kconfig            |  2 +-
> 5 files changed, 44 insertions(+), 20 deletions(-)
> create mode 100644 hw/isa/fdc37m81x-superio.c
>
>diff --git a/hw/isa/fdc37m81x-superio.c b/hw/isa/fdc37m81x-superio.c
>new file mode 100644
>index 00000000000..5ad769b69e8
>--- /dev/null
>+++ b/hw/isa/fdc37m81x-superio.c
>@@ -0,0 +1,37 @@
>+/*
>+ * Generic ISA Super I/O
>+ *
>+ * Copyright (c) 2018 Philippe Mathieu-Daudé
>+ *
>+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
>+ * See the COPYING file in the top-level directory.
>+ * SPDX-License-Identifier: GPL-2.0-or-later
>+ */
>+
>+#include "qemu/osdep.h"
>+#include "hw/isa/superio.h"
>+
>+/* SMS FDC37M817 Super I/O */
>+static void fdc37m81x_class_init(ObjectClass *klass, void *data)
>+{
>+    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
>+
>+    sc->serial.count = 2; /* NS16C550A */
>+    sc->parallel.count = 1;
>+    sc->floppy.count = 1; /* SMSC 82077AA Compatible */
>+    sc->ide.count = 0;
>+}
>+
>+static const TypeInfo fdc37m81x_type_info = {
>+    .name          = TYPE_FDC37M81X_SUPERIO,
>+    .parent        = TYPE_ISA_SUPERIO,
>+    .instance_size = sizeof(ISASuperIODevice),
>+    .class_init    = fdc37m81x_class_init,
>+};
>+
>+static void fdc37m81x_register_types(void)
>+{
>+    type_register_static(&fdc37m81x_type_info);
>+}
>+
>+type_init(fdc37m81x_register_types)
>diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
>index 7dbfc374da3..80e0c34652e 100644
>--- a/hw/isa/isa-superio.c
>+++ b/hw/isa/isa-superio.c
>@@ -187,28 +187,9 @@ static const TypeInfo isa_superio_type_info = {
>     .class_init = isa_superio_class_init,
> };
> 
>-/* SMS FDC37M817 Super I/O */
>-static void fdc37m81x_class_init(ObjectClass *klass, void *data)
>-{
>-    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
>-
>-    sc->serial.count = 2; /* NS16C550A */
>-    sc->parallel.count = 1;
>-    sc->floppy.count = 1; /* SMSC 82077AA Compatible */

>-    sc->ide.count = 0;

Isn't this assignment redundant?

>-}
>-
>-static const TypeInfo fdc37m81x_type_info = {
>-    .name          = TYPE_FDC37M81X_SUPERIO,
>-    .parent        = TYPE_ISA_SUPERIO,
>-    .instance_size = sizeof(ISASuperIODevice),
>-    .class_init    = fdc37m81x_class_init,
>-};
>-
> static void isa_superio_register_types(void)
> {
>     type_register_static(&isa_superio_type_info);
>-    type_register_static(&fdc37m81x_type_info);
> }
> 
> type_init(isa_superio_register_types)
>diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>index 7884179d08b..af856af052d 100644
>--- a/hw/isa/Kconfig
>+++ b/hw/isa/Kconfig
>@@ -23,6 +23,11 @@ config ISA_SUPERIO
>     # Some users of ISA_SUPERIO do not use it
>     #select IDE_ISA
> 
>+config FDC37M81X
>+    bool

+    select ISA_SUPERIO
(in alphabetical order)

>+    select ISA_BUS

Shouldn't it `depend on ISA_BUS`? If so, the previous patch should be adapted 
as well.

>+    select IDE_ISA

Not needed. This device doesn't have an IDE interface.

Best regards,
Bernhard

>+
> config PC87312
>     bool
>     select ISA_SUPERIO
>diff --git a/hw/isa/meson.build b/hw/isa/meson.build
>index 2ab99ce0c6b..f650b395071 100644
>--- a/hw/isa/meson.build
>+++ b/hw/isa/meson.build
>@@ -4,6 +4,7 @@ system_ss.add(when: 'CONFIG_ISA_BUS', if_true: 
>files('isa-bus.c'))
> system_ss.add(when: 'CONFIG_ISA_SUPERIO', if_true: files('isa-superio.c'))
> system_ss.add(when: 'CONFIG_PC87312', if_true: files('pc87312.c'))
> system_ss.add(when: 'CONFIG_PIIX', if_true: files('piix.c'))
>+system_ss.add(when: 'CONFIG_FDC37M81X', if_true: files('fdc37m81x-superio.c'))
> system_ss.add(when: 'CONFIG_SMC37C669', if_true: files('smc37c669-superio.c'))
> system_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686.c'))
> 
>diff --git a/hw/mips/Kconfig b/hw/mips/Kconfig
>index ab61af209a0..94b969c21db 100644
>--- a/hw/mips/Kconfig
>+++ b/hw/mips/Kconfig
>@@ -1,7 +1,7 @@
> config MALTA
>     bool
>+    select FDC37M81X
>     select GT64120
>-    select ISA_SUPERIO
>     select PIIX
> 
> config MIPSSIM