1. Patchew
  2. QEMU
  3. [PATCH 0/4] mips: do not list individual devices from configs/
  4. View patch

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

Paolo Bonzini posted 4 patches 10 months ago
There is a newer version of this series
[PATCH 2/4] isa: extract FDC37M81X to a separate file
Posted by Paolo Bonzini 10 months ago 
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;
-}
-
-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_BUS
+    select IDE_ISA
+
 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
-- 
2.43.0
Re: [PATCH 2/4] isa: extract FDC37M81X to a separate file
Posted by Bernhard Beschow 9 months, 3 weeks ago 

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
Re: [PATCH 2/4] isa: extract FDC37M81X to a separate file
Posted by Paolo Bonzini 9 months, 3 weeks ago 
On Sun, Feb 4, 2024 at 9:00 PM Bernhard Beschow <shentey@gmail.com> wrote:
> >-/* 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?

I think it's there for clarity.

> >+config FDC37M81X
> >+    bool
>
> +    select ISA_SUPERIO
> (in alphabetical order)
>
> >+    select ISA_BUS
>
> Shouldn't it `depend on ISA_BUS`?

It should be there in "config ISA_SUPERIO". That was a mistake in the
original patch that introduced hw/isa/Kconfig. And VT82C686 should
also select ISA_BUS because it's a combined PCI-ISA bridge and SuperIO
chip.

Paolo

> >+    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
>
Re: [PATCH 2/4] isa: extract FDC37M81X to a separate file
Posted by Bernhard Beschow 10 months ago 

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

Replace this description...

>+ *
>+ * 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 */

... with this one?

>+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;
>+}
>+

Don't we prefer a macro for below code? While touching the code we could use it. (Sorry I can't recall its name from the top of my head and I don't have access to the code right now).

Best regards,
Bernhard

>+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;
>-}
>-
>-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_BUS
>+    select IDE_ISA
>+
> 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
Re: [PATCH 2/4] isa: extract FDC37M81X to a separate file
Posted by Paolo Bonzini 10 months ago 
On Mon, Jan 29, 2024 at 8:49 PM Bernhard Beschow <shentey@gmail.com> wrote:
> Don't we prefer a macro for below code? While touching the code we could use it. (Sorry I can't recall its name from the top of my head and I don't have access to the code right now).

Ah yeah, OBJECT_DEFINE_TYPE. Not sure it's much of a win because
neither finalize or instance_init are required here.

Paolo
Re: [PATCH 2/4] isa: extract FDC37M81X to a separate file
Posted by Philippe Mathieu-Daudé 10 months ago 
On 29/1/24 21:26, Paolo Bonzini wrote:
> On Mon, Jan 29, 2024 at 8:49 PM Bernhard Beschow <shentey@gmail.com> wrote:
>> Don't we prefer a macro for below code? While touching the code we could use it. (Sorry I can't recall its name from the top of my head and I don't have access to the code right now).
> 
> Ah yeah, OBJECT_DEFINE_TYPE. Not sure it's much of a win because
> neither finalize or instance_init are required here.

Bernhard likely meant type_init() -> DEFINE_TYPES().
Re: [PATCH 2/4] isa: extract FDC37M81X to a separate file
Posted by Bernhard Beschow 10 months ago 

Am 30. Januar 2024 07:48:50 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 29/1/24 21:26, Paolo Bonzini wrote:
>> On Mon, Jan 29, 2024 at 8:49 PM Bernhard Beschow <shentey@gmail.com> wrote:
>>> Don't we prefer a macro for below code? While touching the code we could use it. (Sorry I can't recall its name from the top of my head and I don't have access to the code right now).
>> 
>> Ah yeah, OBJECT_DEFINE_TYPE. Not sure it's much of a win because
>> neither finalize or instance_init are required here.
>
>Bernhard likely meant type_init() -> DEFINE_TYPES().

Indeed. And it doesn't cover the TypeInfo structure which for some reason I thought it did...

Anyway, I pointed this out because the maintainer of the device model gave similar comments in the past. So, speaking of maintainers: The MAINTAINERS file might need an update, too.

Best regards,
Bernhard

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.