[PATCH 0/2] Export struct ISAParallelState

[PATCH 0/2] Export struct ISAParallelState

[Bernhard Beschow]

This series incorporates rebased versions of the ISAParallelState patches of
[1] as requested by Mark.

Changes since [1]:
* Don't export register definitions (Phil)
* Rephrase commit message of second patch (Zoltan)

[1] https://patchew.org/QEMU/20230521123049.312349-1-shentey@gmail.com/

Bernhard Beschow (2):
  hw/char/parallel: Export struct ParallelState
  hw/char/parallel-isa: Export struct ISAParallelState

 include/hw/char/parallel-isa.h | 46 ++++++++++++++++++++++++++++++++++
 include/hw/char/parallel.h     | 21 +++++++++++++++-
 hw/char/parallel-isa.c         |  1 +
 hw/char/parallel.c             | 32 +----------------------
 hw/i386/pc_piix.c              |  2 +-
 hw/i386/pc_q35.c               |  2 +-
 hw/isa/isa-superio.c           |  1 +
 hw/sparc64/sun4u.c             |  2 +-
 8 files changed, 72 insertions(+), 35 deletions(-)
 create mode 100644 include/hw/char/parallel-isa.h

-- 
2.41.0

[PATCH 1/2] hw/char/parallel: Export struct ParallelState

[Bernhard Beschow]

Exporting ParallelState is a precondition for exporing TYPE_ISA_PARALLEL to be
performed in the next patch.

Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/hw/char/parallel.h | 21 +++++++++++++++++++++
 hw/char/parallel.c         | 20 --------------------
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/include/hw/char/parallel.h b/include/hw/char/parallel.h
index 29d2876d00..9f76edca81 100644
--- a/include/hw/char/parallel.h
+++ b/include/hw/char/parallel.h
@@ -1,9 +1,30 @@
 #ifndef HW_PARALLEL_H
 #define HW_PARALLEL_H
 
+#include "exec/ioport.h"
+#include "exec/memory.h"
 #include "hw/isa/isa.h"
+#include "hw/irq.h"
+#include "chardev/char-fe.h"
 #include "chardev/char.h"
 
+typedef struct ParallelState {
+    MemoryRegion iomem;
+    uint8_t dataw;
+    uint8_t datar;
+    uint8_t status;
+    uint8_t control;
+    qemu_irq irq;
+    int irq_pending;
+    CharBackend chr;
+    int hw_driver;
+    int epp_timeout;
+    uint32_t last_read_offset; /* For debugging */
+    /* Memory-mapped interface */
+    int it_shift;
+    PortioList portio_list;
+} ParallelState;
+
 #define TYPE_ISA_PARALLEL "isa-parallel"
 
 void parallel_hds_isa_init(ISABus *bus, int n);
diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index 3d32589bb3..e75fc5019d 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -27,10 +27,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "chardev/char-parallel.h"
-#include "chardev/char-fe.h"
 #include "hw/acpi/acpi_aml_interface.h"
-#include "hw/irq.h"
-#include "hw/isa/isa.h"
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "migration/vmstate.h"
@@ -76,23 +73,6 @@
 
 #define PARA_CTR_SIGNAL (PARA_CTR_SELECT|PARA_CTR_INIT|PARA_CTR_AUTOLF|PARA_CTR_STROBE)
 
-typedef struct ParallelState {
-    MemoryRegion iomem;
-    uint8_t dataw;
-    uint8_t datar;
-    uint8_t status;
-    uint8_t control;
-    qemu_irq irq;
-    int irq_pending;
-    CharBackend chr;
-    int hw_driver;
-    int epp_timeout;
-    uint32_t last_read_offset; /* For debugging */
-    /* Memory-mapped interface */
-    int it_shift;
-    PortioList portio_list;
-} ParallelState;
-
 OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)
 
 struct ISAParallelState {
-- 
2.41.0

[PATCH 2/2] hw/char/parallel-isa: Export struct ISAParallelState

[Bernhard Beschow]

Allows the struct to be embedded directly into device models without additional
allocation.

Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 include/hw/char/parallel-isa.h | 46 ++++++++++++++++++++++++++++++++++
 include/hw/char/parallel.h     |  2 --
 hw/char/parallel-isa.c         |  1 +
 hw/char/parallel.c             | 12 +--------
 hw/i386/pc_piix.c              |  2 +-
 hw/i386/pc_q35.c               |  2 +-
 hw/isa/isa-superio.c           |  1 +
 hw/sparc64/sun4u.c             |  2 +-
 8 files changed, 52 insertions(+), 16 deletions(-)
 create mode 100644 include/hw/char/parallel-isa.h

diff --git a/include/hw/char/parallel-isa.h b/include/hw/char/parallel-isa.h
new file mode 100644
index 0000000000..27bdacf1a3
--- /dev/null
+++ b/include/hw/char/parallel-isa.h
@@ -0,0 +1,46 @@
+/*
+ * QEMU ISA Parallel PORT emulation
+ *
+ * Copyright (c) 2003-2005 Fabrice Bellard
+ * Copyright (c) 2007 Marko Kohtala
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_PARALLEL_ISA_H
+#define HW_PARALLEL_ISA_H
+
+#include "parallel.h"
+
+#include "hw/isa/isa.h"
+#include "qom/object.h"
+
+#define TYPE_ISA_PARALLEL "isa-parallel"
+OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)
+
+struct ISAParallelState {
+    ISADevice parent_obj;
+
+    uint32_t index;
+    uint32_t iobase;
+    uint32_t isairq;
+    ParallelState state;
+};
+
+#endif /* HW_PARALLEL_ISA_H */
diff --git a/include/hw/char/parallel.h b/include/hw/char/parallel.h
index 9f76edca81..7b5a309a03 100644
--- a/include/hw/char/parallel.h
+++ b/include/hw/char/parallel.h
@@ -25,8 +25,6 @@ typedef struct ParallelState {
     PortioList portio_list;
 } ParallelState;
 
-#define TYPE_ISA_PARALLEL "isa-parallel"
-
 void parallel_hds_isa_init(ISABus *bus, int n);
 
 bool parallel_mm_init(MemoryRegion *address_space,
diff --git a/hw/char/parallel-isa.c b/hw/char/parallel-isa.c
index 547ae69304..ab0f879998 100644
--- a/hw/char/parallel-isa.c
+++ b/hw/char/parallel-isa.c
@@ -13,6 +13,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/isa/isa.h"
 #include "hw/qdev-properties.h"
+#include "hw/char/parallel-isa.h"
 #include "hw/char/parallel.h"
 #include "qapi/error.h"
 
diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index e75fc5019d..147c900f0d 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -31,6 +31,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/qdev-properties-system.h"
 #include "migration/vmstate.h"
+#include "hw/char/parallel-isa.h"
 #include "hw/char/parallel.h"
 #include "sysemu/reset.h"
 #include "sysemu/sysemu.h"
@@ -73,17 +74,6 @@
 
 #define PARA_CTR_SIGNAL (PARA_CTR_SELECT|PARA_CTR_INIT|PARA_CTR_AUTOLF|PARA_CTR_STROBE)
 
-OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)
-
-struct ISAParallelState {
-    ISADevice parent_obj;
-
-    uint32_t index;
-    uint32_t iobase;
-    uint32_t isairq;
-    ParallelState state;
-};
-
 static void parallel_update_irq(ParallelState *s)
 {
     if (s->irq_pending)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 42af03dbb4..44146e6ff5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -26,7 +26,7 @@
 #include CONFIG_DEVICES
 
 #include "qemu/units.h"
-#include "hw/char/parallel.h"
+#include "hw/char/parallel-isa.h"
 #include "hw/dma/i8257.h"
 #include "hw/loader.h"
 #include "hw/i386/x86.h"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 6155427e48..a9a59ed42b 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -30,7 +30,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
-#include "hw/char/parallel.h"
+#include "hw/char/parallel-isa.h"
 #include "hw/loader.h"
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/rtc/mc146818rtc.h"
diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
index 9292ec3bcf..7dbfc374da 100644
--- a/hw/isa/isa-superio.c
+++ b/hw/isa/isa-superio.c
@@ -21,6 +21,7 @@
 #include "hw/isa/superio.h"
 #include "hw/qdev-properties.h"
 #include "hw/input/i8042.h"
+#include "hw/char/parallel-isa.h"
 #include "hw/char/serial.h"
 #include "trace.h"
 
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index e2858a0331..29e9b6cc26 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -35,7 +35,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/pci-host/sabre.h"
 #include "hw/char/serial.h"
-#include "hw/char/parallel.h"
+#include "hw/char/parallel-isa.h"
 #include "hw/rtc/m48t59.h"
 #include "migration/vmstate.h"
 #include "hw/input/i8042.h"
-- 
2.41.0

Re: [PATCH 2/2] hw/char/parallel-isa: Export struct ISAParallelState

[BALATON Zoltan]

On Sun, 11 Jun 2023, Bernhard Beschow wrote:
> Allows the struct to be embedded directly into device models without additional
> allocation.
>
> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Patches missing SoB, checkpatch should have cought this.

I don't see any of the machines or device models actually embedding 
ISAParallelState or ParallelState so don't know what this patch is trying 
to achieve. Please post the whole series with the patches that this is a 
preparation for so we can se where this leads.

Regards,
BALATON Zoltan

> ---
> include/hw/char/parallel-isa.h | 46 ++++++++++++++++++++++++++++++++++
> include/hw/char/parallel.h     |  2 --
> hw/char/parallel-isa.c         |  1 +
> hw/char/parallel.c             | 12 +--------
> hw/i386/pc_piix.c              |  2 +-
> hw/i386/pc_q35.c               |  2 +-
> hw/isa/isa-superio.c           |  1 +
> hw/sparc64/sun4u.c             |  2 +-
> 8 files changed, 52 insertions(+), 16 deletions(-)
> create mode 100644 include/hw/char/parallel-isa.h
>
> diff --git a/include/hw/char/parallel-isa.h b/include/hw/char/parallel-isa.h
> new file mode 100644
> index 0000000000..27bdacf1a3
> --- /dev/null
> +++ b/include/hw/char/parallel-isa.h
> @@ -0,0 +1,46 @@
> +/*
> + * QEMU ISA Parallel PORT emulation
> + *
> + * Copyright (c) 2003-2005 Fabrice Bellard
> + * Copyright (c) 2007 Marko Kohtala
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef HW_PARALLEL_ISA_H
> +#define HW_PARALLEL_ISA_H
> +
> +#include "parallel.h"
> +
> +#include "hw/isa/isa.h"
> +#include "qom/object.h"
> +
> +#define TYPE_ISA_PARALLEL "isa-parallel"
> +OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)
> +
> +struct ISAParallelState {
> +    ISADevice parent_obj;
> +
> +    uint32_t index;
> +    uint32_t iobase;
> +    uint32_t isairq;
> +    ParallelState state;
> +};
> +
> +#endif /* HW_PARALLEL_ISA_H */
> diff --git a/include/hw/char/parallel.h b/include/hw/char/parallel.h
> index 9f76edca81..7b5a309a03 100644
> --- a/include/hw/char/parallel.h
> +++ b/include/hw/char/parallel.h
> @@ -25,8 +25,6 @@ typedef struct ParallelState {
>     PortioList portio_list;
> } ParallelState;
>
> -#define TYPE_ISA_PARALLEL "isa-parallel"
> -
> void parallel_hds_isa_init(ISABus *bus, int n);
>
> bool parallel_mm_init(MemoryRegion *address_space,
> diff --git a/hw/char/parallel-isa.c b/hw/char/parallel-isa.c
> index 547ae69304..ab0f879998 100644
> --- a/hw/char/parallel-isa.c
> +++ b/hw/char/parallel-isa.c
> @@ -13,6 +13,7 @@
> #include "sysemu/sysemu.h"
> #include "hw/isa/isa.h"
> #include "hw/qdev-properties.h"
> +#include "hw/char/parallel-isa.h"
> #include "hw/char/parallel.h"
> #include "qapi/error.h"
>
> diff --git a/hw/char/parallel.c b/hw/char/parallel.c
> index e75fc5019d..147c900f0d 100644
> --- a/hw/char/parallel.c
> +++ b/hw/char/parallel.c
> @@ -31,6 +31,7 @@
> #include "hw/qdev-properties.h"
> #include "hw/qdev-properties-system.h"
> #include "migration/vmstate.h"
> +#include "hw/char/parallel-isa.h"
> #include "hw/char/parallel.h"
> #include "sysemu/reset.h"
> #include "sysemu/sysemu.h"
> @@ -73,17 +74,6 @@
>
> #define PARA_CTR_SIGNAL (PARA_CTR_SELECT|PARA_CTR_INIT|PARA_CTR_AUTOLF|PARA_CTR_STROBE)
>
> -OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)
> -
> -struct ISAParallelState {
> -    ISADevice parent_obj;
> -
> -    uint32_t index;
> -    uint32_t iobase;
> -    uint32_t isairq;
> -    ParallelState state;
> -};
> -
> static void parallel_update_irq(ParallelState *s)
> {
>     if (s->irq_pending)
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 42af03dbb4..44146e6ff5 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -26,7 +26,7 @@
> #include CONFIG_DEVICES
>
> #include "qemu/units.h"
> -#include "hw/char/parallel.h"
> +#include "hw/char/parallel-isa.h"
> #include "hw/dma/i8257.h"
> #include "hw/loader.h"
> #include "hw/i386/x86.h"
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 6155427e48..a9a59ed42b 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -30,7 +30,7 @@
>
> #include "qemu/osdep.h"
> #include "qemu/units.h"
> -#include "hw/char/parallel.h"
> +#include "hw/char/parallel-isa.h"
> #include "hw/loader.h"
> #include "hw/i2c/smbus_eeprom.h"
> #include "hw/rtc/mc146818rtc.h"
> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
> index 9292ec3bcf..7dbfc374da 100644
> --- a/hw/isa/isa-superio.c
> +++ b/hw/isa/isa-superio.c
> @@ -21,6 +21,7 @@
> #include "hw/isa/superio.h"
> #include "hw/qdev-properties.h"
> #include "hw/input/i8042.h"
> +#include "hw/char/parallel-isa.h"
> #include "hw/char/serial.h"
> #include "trace.h"
>
> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index e2858a0331..29e9b6cc26 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -35,7 +35,7 @@
> #include "hw/qdev-properties.h"
> #include "hw/pci-host/sabre.h"
> #include "hw/char/serial.h"
> -#include "hw/char/parallel.h"
> +#include "hw/char/parallel-isa.h"
> #include "hw/rtc/m48t59.h"
> #include "migration/vmstate.h"
> #include "hw/input/i8042.h"
>

Re: [PATCH 2/2] hw/char/parallel-isa: Export struct ISAParallelState

[Bernhard Beschow]

Am 11. Juni 2023 13:15:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Sun, 11 Jun 2023, Bernhard Beschow wrote:
>> Allows the struct to be embedded directly into device models without additional
>> allocation.
>> 
>> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
>Patches missing SoB, checkpatch should have cought this.

Thanks for catching again. Fixed in v2.

>
>I don't see any of the machines or device models actually embedding ISAParallelState or ParallelState so don't know what this patch is trying to achieve. Please post the whole series with the patches that this is a preparation for so we can se where this leads.

No further plans from my side.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> ---
>> include/hw/char/parallel-isa.h | 46 ++++++++++++++++++++++++++++++++++
>> include/hw/char/parallel.h     |  2 --
>> hw/char/parallel-isa.c         |  1 +
>> hw/char/parallel.c             | 12 +--------
>> hw/i386/pc_piix.c              |  2 +-
>> hw/i386/pc_q35.c               |  2 +-
>> hw/isa/isa-superio.c           |  1 +
>> hw/sparc64/sun4u.c             |  2 +-
>> 8 files changed, 52 insertions(+), 16 deletions(-)
>> create mode 100644 include/hw/char/parallel-isa.h
>> 
>> diff --git a/include/hw/char/parallel-isa.h b/include/hw/char/parallel-isa.h
>> new file mode 100644
>> index 0000000000..27bdacf1a3
>> --- /dev/null
>> +++ b/include/hw/char/parallel-isa.h
>> @@ -0,0 +1,46 @@
>> +/*
>> + * QEMU ISA Parallel PORT emulation
>> + *
>> + * Copyright (c) 2003-2005 Fabrice Bellard
>> + * Copyright (c) 2007 Marko Kohtala
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#ifndef HW_PARALLEL_ISA_H
>> +#define HW_PARALLEL_ISA_H
>> +
>> +#include "parallel.h"
>> +
>> +#include "hw/isa/isa.h"
>> +#include "qom/object.h"
>> +
>> +#define TYPE_ISA_PARALLEL "isa-parallel"
>> +OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)
>> +
>> +struct ISAParallelState {
>> +    ISADevice parent_obj;
>> +
>> +    uint32_t index;
>> +    uint32_t iobase;
>> +    uint32_t isairq;
>> +    ParallelState state;
>> +};
>> +
>> +#endif /* HW_PARALLEL_ISA_H */
>> diff --git a/include/hw/char/parallel.h b/include/hw/char/parallel.h
>> index 9f76edca81..7b5a309a03 100644
>> --- a/include/hw/char/parallel.h
>> +++ b/include/hw/char/parallel.h
>> @@ -25,8 +25,6 @@ typedef struct ParallelState {
>>     PortioList portio_list;
>> } ParallelState;
>> 
>> -#define TYPE_ISA_PARALLEL "isa-parallel"
>> -
>> void parallel_hds_isa_init(ISABus *bus, int n);
>> 
>> bool parallel_mm_init(MemoryRegion *address_space,
>> diff --git a/hw/char/parallel-isa.c b/hw/char/parallel-isa.c
>> index 547ae69304..ab0f879998 100644
>> --- a/hw/char/parallel-isa.c
>> +++ b/hw/char/parallel-isa.c
>> @@ -13,6 +13,7 @@
>> #include "sysemu/sysemu.h"
>> #include "hw/isa/isa.h"
>> #include "hw/qdev-properties.h"
>> +#include "hw/char/parallel-isa.h"
>> #include "hw/char/parallel.h"
>> #include "qapi/error.h"
>> 
>> diff --git a/hw/char/parallel.c b/hw/char/parallel.c
>> index e75fc5019d..147c900f0d 100644
>> --- a/hw/char/parallel.c
>> +++ b/hw/char/parallel.c
>> @@ -31,6 +31,7 @@
>> #include "hw/qdev-properties.h"
>> #include "hw/qdev-properties-system.h"
>> #include "migration/vmstate.h"
>> +#include "hw/char/parallel-isa.h"
>> #include "hw/char/parallel.h"
>> #include "sysemu/reset.h"
>> #include "sysemu/sysemu.h"
>> @@ -73,17 +74,6 @@
>> 
>> #define PARA_CTR_SIGNAL (PARA_CTR_SELECT|PARA_CTR_INIT|PARA_CTR_AUTOLF|PARA_CTR_STROBE)
>> 
>> -OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)
>> -
>> -struct ISAParallelState {
>> -    ISADevice parent_obj;
>> -
>> -    uint32_t index;
>> -    uint32_t iobase;
>> -    uint32_t isairq;
>> -    ParallelState state;
>> -};
>> -
>> static void parallel_update_irq(ParallelState *s)
>> {
>>     if (s->irq_pending)
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index 42af03dbb4..44146e6ff5 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -26,7 +26,7 @@
>> #include CONFIG_DEVICES
>> 
>> #include "qemu/units.h"
>> -#include "hw/char/parallel.h"
>> +#include "hw/char/parallel-isa.h"
>> #include "hw/dma/i8257.h"
>> #include "hw/loader.h"
>> #include "hw/i386/x86.h"
>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>> index 6155427e48..a9a59ed42b 100644
>> --- a/hw/i386/pc_q35.c
>> +++ b/hw/i386/pc_q35.c
>> @@ -30,7 +30,7 @@
>> 
>> #include "qemu/osdep.h"
>> #include "qemu/units.h"
>> -#include "hw/char/parallel.h"
>> +#include "hw/char/parallel-isa.h"
>> #include "hw/loader.h"
>> #include "hw/i2c/smbus_eeprom.h"
>> #include "hw/rtc/mc146818rtc.h"
>> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
>> index 9292ec3bcf..7dbfc374da 100644
>> --- a/hw/isa/isa-superio.c
>> +++ b/hw/isa/isa-superio.c
>> @@ -21,6 +21,7 @@
>> #include "hw/isa/superio.h"
>> #include "hw/qdev-properties.h"
>> #include "hw/input/i8042.h"
>> +#include "hw/char/parallel-isa.h"
>> #include "hw/char/serial.h"
>> #include "trace.h"
>> 
>> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
>> index e2858a0331..29e9b6cc26 100644
>> --- a/hw/sparc64/sun4u.c
>> +++ b/hw/sparc64/sun4u.c
>> @@ -35,7 +35,7 @@
>> #include "hw/qdev-properties.h"
>> #include "hw/pci-host/sabre.h"
>> #include "hw/char/serial.h"
>> -#include "hw/char/parallel.h"
>> +#include "hw/char/parallel-isa.h"
>> #include "hw/rtc/m48t59.h"
>> #include "migration/vmstate.h"
>> #include "hw/input/i8042.h"
>> 

Re: [PATCH 2/2] hw/char/parallel-isa: Export struct ISAParallelState

[BALATON Zoltan]

On Mon, 12 Jun 2023, Bernhard Beschow wrote:
> Am 11. Juni 2023 13:15:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Sun, 11 Jun 2023, Bernhard Beschow wrote:
>>> Allows the struct to be embedded directly into device models without additional
>>> allocation.
>>>
>>> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> Patches missing SoB, checkpatch should have cought this.
>
> Thanks for catching again. Fixed in v2.
>
>>
>> I don't see any of the machines or device models actually embedding 
>> ISAParallelState or ParallelState so don't know what this patch is 
>> trying to achieve. Please post the whole series with the patches that 
>> this is a preparation for so we can se where this leads.
>
> No further plans from my side.

Then IMO these patches are not needed. Keeping the struct definitions in 
parallel.c ensures they are not accessed by anything else and keeps the 
object encapsulation. I don't see a point for moving the defs to a header 
if nothing wants to use them. This is done for other devices to allow them 
to be embedded in other devices but if that's not the case here then why 
this series? (The TYPE_ISA_PARALLEL #define seems to be already in 
include/hw/chsr/parallel.h so if you only want to use that like in the 
series you've referenced in the cover letter then that can be done without 
these patches.)

Regards,
BALATON Zoltan

Re: [PATCH 2/2] hw/char/parallel-isa: Export struct ISAParallelState

[Mark Cave-Ayland]

On 12/06/2023 11:06, BALATON Zoltan wrote:

> On Mon, 12 Jun 2023, Bernhard Beschow wrote:
>> Am 11. Juni 2023 13:15:58 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>>> On Sun, 11 Jun 2023, Bernhard Beschow wrote:
>>>> Allows the struct to be embedded directly into device models without additional
>>>> allocation.
>>>>
>>>> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> Patches missing SoB, checkpatch should have cought this.
>>
>> Thanks for catching again. Fixed in v2.
>>
>>>
>>> I don't see any of the machines or device models actually embedding 
>>> ISAParallelState or ParallelState so don't know what this patch is trying to 
>>> achieve. Please post the whole series with the patches that this is a preparation 
>>> for so we can se where this leads.
>>
>> No further plans from my side.
> 
> Then IMO these patches are not needed. Keeping the struct definitions in parallel.c 
> ensures they are not accessed by anything else and keeps the object encapsulation. I 
> don't see a point for moving the defs to a header if nothing wants to use them. This 
> is done for other devices to allow them to be embedded in other devices but if that's 
> not the case here then why this series? (The TYPE_ISA_PARALLEL #define seems to be 
> already in include/hw/chsr/parallel.h so if you only want to use that like in the 
> series you've referenced in the cover letter then that can be done without these 
> patches.)

The TYPE_ISA_PARALLEL constant was only moved there in commit 963e94a97b 
("hw/char/parallel: Move TYPE_ISA_PARALLEL to the header file") but having each 
separate type defined in its own file is how we've done things for some time: there 
is nothing new here.

In particular it is done this way so that ParallelState could be used on a non-ISA 
bus, and to allow users who are security conscious to compile out particular devices 
as needed.


ATB,

Mark.