[PATCH v3 0/3] Trivial cleanups

[PATCH v3 0/3] Trivial cleanups

[Bernhard Beschow]

This series:
* Removes dead code from omap_uart and i82378
* Resolves redundant code in the i8254 timer devices

v3:
* Drop TYPE_ISA_PARALLEL since they became obsolete by
  https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/

v2:
* Export ParallelState and ISAParallelState (Mark)

Testing done:
* `make check`

Bernhard Beschow (3):
  hw/timer/i8254_common: Share "iobase" property via base class
  hw/arm/omap: Remove unused omap_uart_attach()
  hw/isa/i82378: Remove unused "io" attribute

 include/hw/arm/omap.h   | 1 -
 hw/char/omap_uart.c     | 9 ---------
 hw/i386/kvm/i8254.c     | 1 -
 hw/isa/i82378.c         | 1 -
 hw/timer/i8254.c        | 6 ------
 hw/timer/i8254_common.c | 6 ++++++
 6 files changed, 6 insertions(+), 18 deletions(-)

-- 
2.40.1

[PATCH v3 1/3] hw/timer/i8254_common: Share "iobase" property via base class

[Bernhard Beschow]

Both TYPE_KVM_I8254 and TYPE_I8254 have their own but same implementation of
the "iobase" property. The storage for the property already resides in
PITCommonState, so also move the property definition there.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/kvm/i8254.c     | 1 -
 hw/timer/i8254.c        | 6 ------
 hw/timer/i8254_common.c | 6 ++++++
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/i386/kvm/i8254.c b/hw/i386/kvm/i8254.c
index 191a26fa57..6a7383d877 100644
--- a/hw/i386/kvm/i8254.c
+++ b/hw/i386/kvm/i8254.c
@@ -301,7 +301,6 @@ static void kvm_pit_realizefn(DeviceState *dev, Error **errp)
 }
 
 static Property kvm_pit_properties[] = {
-    DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
     DEFINE_PROP_LOSTTICKPOLICY("lost_tick_policy", KVMPITState,
                                lost_tick_policy, LOST_TICK_POLICY_DELAY),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/timer/i8254.c b/hw/timer/i8254.c
index c8388ea432..c235496fc9 100644
--- a/hw/timer/i8254.c
+++ b/hw/timer/i8254.c
@@ -350,11 +350,6 @@ static void pit_realizefn(DeviceState *dev, Error **errp)
     pc->parent_realize(dev, errp);
 }
 
-static Property pit_properties[] = {
-    DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void pit_class_initfn(ObjectClass *klass, void *data)
 {
     PITClass *pc = PIT_CLASS(klass);
@@ -366,7 +361,6 @@ static void pit_class_initfn(ObjectClass *klass, void *data)
     k->get_channel_info = pit_get_channel_info_common;
     k->post_load = pit_post_load;
     dc->reset = pit_reset;
-    device_class_set_props(dc, pit_properties);
 }
 
 static const TypeInfo pit_info = {
diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index 050875b497..e4093e2904 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -240,6 +240,11 @@ static const VMStateDescription vmstate_pit_common = {
     }
 };
 
+static Property pit_common_properties[] = {
+    DEFINE_PROP_UINT32("iobase", PITCommonState, iobase,  -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void pit_common_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -252,6 +257,7 @@ static void pit_common_class_init(ObjectClass *klass, void *data)
      * done by board code.
      */
     dc->user_creatable = false;
+    device_class_set_props(dc, pit_common_properties);
 }
 
 static const TypeInfo pit_common_type = {
-- 
2.40.1

[PATCH v3 2/3] hw/arm/omap: Remove unused omap_uart_attach()

[Bernhard Beschow]

The function is unused since commit
bdad3654d3c55f478e538037d9eccd204e5fc8ee ('hw/arm/nseries: Remove
invalid/unnecessary n8x0_uart_setup()').

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/arm/omap.h | 1 -
 hw/char/omap_uart.c   | 9 ---------
 2 files changed, 10 deletions(-)

diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index c275d9b681..067e9419f7 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -724,7 +724,6 @@ struct omap_uart_s *omap2_uart_init(MemoryRegion *sysmem,
                 qemu_irq txdma, qemu_irq rxdma,
                 const char *label, Chardev *chr);
 void omap_uart_reset(struct omap_uart_s *s);
-void omap_uart_attach(struct omap_uart_s *s, Chardev *chr);
 
 struct omap_mpuio_s;
 qemu_irq *omap_mpuio_in_get(struct omap_mpuio_s *s);
diff --git a/hw/char/omap_uart.c b/hw/char/omap_uart.c
index 1c890b9201..6848bddb4e 100644
--- a/hw/char/omap_uart.c
+++ b/hw/char/omap_uart.c
@@ -175,12 +175,3 @@ struct omap_uart_s *omap2_uart_init(MemoryRegion *sysmem,
 
     return s;
 }
-
-void omap_uart_attach(struct omap_uart_s *s, Chardev *chr)
-{
-    /* TODO: Should reuse or destroy current s->serial */
-    s->serial = serial_mm_init(get_system_memory(), s->base, 2, s->irq,
-                               omap_clk_getrate(s->fclk) / 16,
-                               chr ?: qemu_chr_new("null", "null", NULL),
-                               DEVICE_NATIVE_ENDIAN);
-}
-- 
2.40.1

[PATCH v3 3/3] hw/isa/i82378: Remove unused "io" attribute

[Bernhard Beschow]

The attribute isn't used since commit 5c9736789b79ea49cd236ac326f0a414f63b1015
"i82378: Cleanup implementation".

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/isa/i82378.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c
index 5432ab5065..63e0857208 100644
--- a/hw/isa/i82378.c
+++ b/hw/isa/i82378.c
@@ -34,7 +34,6 @@ struct I82378State {
 
     qemu_irq cpu_intr;
     qemu_irq *isa_irqs_in;
-    MemoryRegion io;
 };
 
 static const VMStateDescription vmstate_i82378 = {
-- 
2.40.1

Re: [PATCH v3 3/3] hw/isa/i82378: Remove unused "io" attribute

[Philippe Mathieu-Daudé]

On 23/5/23 21:56, Bernhard Beschow wrote:
> The attribute isn't used since commit 5c9736789b79ea49cd236ac326f0a414f63b1015
> "i82378: Cleanup implementation".
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/isa/i82378.c | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 0/3] Trivial cleanups

[Mark Cave-Ayland]

On 23/05/2023 20:56, Bernhard Beschow wrote:

> This series:
> * Removes dead code from omap_uart and i82378
> * Resolves redundant code in the i8254 timer devices
> 
> v3:
> * Drop TYPE_ISA_PARALLEL since they became obsolete by
>    https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/

Oh I didn't see that this had already been merged :/

It's not a reason to block this series, but I'd still like to see your changes to 
ParallelState and ISAParallelState merged separately since they are a better match 
for our QOM coding standards.

> v2:
> * Export ParallelState and ISAParallelState (Mark)
> 
> Testing done:
> * `make check`
> 
> Bernhard Beschow (3):
>    hw/timer/i8254_common: Share "iobase" property via base class
>    hw/arm/omap: Remove unused omap_uart_attach()
>    hw/isa/i82378: Remove unused "io" attribute
> 
>   include/hw/arm/omap.h   | 1 -
>   hw/char/omap_uart.c     | 9 ---------
>   hw/i386/kvm/i8254.c     | 1 -
>   hw/isa/i82378.c         | 1 -
>   hw/timer/i8254.c        | 6 ------
>   hw/timer/i8254_common.c | 6 ++++++
>   6 files changed, 6 insertions(+), 18 deletions(-)

Do we know who is going to pick up these series? I can send a PR if no-one minds?


ATB,

Mark.

Re: [PATCH v3 0/3] Trivial cleanups

[Bernhard Beschow]

Am 25. Mai 2023 16:03:15 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 23/05/2023 20:56, Bernhard Beschow wrote:
>
>> This series:
>> * Removes dead code from omap_uart and i82378
>> * Resolves redundant code in the i8254 timer devices
>> 
>> v3:
>> * Drop TYPE_ISA_PARALLEL since they became obsolete by
>>    https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/
>
>Oh I didn't see that this had already been merged :/
>
>It's not a reason to block this series, but I'd still like to see your changes to ParallelState and ISAParallelState merged separately since they are a better match for our QOM coding standards.
>
>> v2:
>> * Export ParallelState and ISAParallelState (Mark)
>> 
>> Testing done:
>> * `make check`
>> 
>> Bernhard Beschow (3):
>>    hw/timer/i8254_common: Share "iobase" property via base class
>>    hw/arm/omap: Remove unused omap_uart_attach()
>>    hw/isa/i82378: Remove unused "io" attribute
>> 
>>   include/hw/arm/omap.h   | 1 -
>>   hw/char/omap_uart.c     | 9 ---------
>>   hw/i386/kvm/i8254.c     | 1 -
>>   hw/isa/i82378.c         | 1 -
>>   hw/timer/i8254.c        | 6 ------
>>   hw/timer/i8254_common.c | 6 ++++++
>>   6 files changed, 6 insertions(+), 18 deletions(-)
>
>Do we know who is going to pick up these series? I can send a PR if no-one minds?

I guess the silence means that no one minds ;)

Perhaps we could queue these changes together with my VIA/PCI IDE cleanup series once it is fully reviewed?

Best regards,
Bernhard
>
>
>ATB,
>
>Mark.

Re: [PATCH v3 0/3] Trivial cleanups

[Michael S. Tsirkin]

On Thu, May 25, 2023 at 05:03:15PM +0100, Mark Cave-Ayland wrote:
> On 23/05/2023 20:56, Bernhard Beschow wrote:
> 
> > This series:
> > * Removes dead code from omap_uart and i82378
> > * Resolves redundant code in the i8254 timer devices
> > 
> > v3:
> > * Drop TYPE_ISA_PARALLEL since they became obsolete by
> >    https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/
> 
> Oh I didn't see that this had already been merged :/
> 
> It's not a reason to block this series, but I'd still like to see your
> changes to ParallelState and ISAParallelState merged separately since they
> are a better match for our QOM coding standards.
> 
> > v2:
> > * Export ParallelState and ISAParallelState (Mark)
> > 
> > Testing done:
> > * `make check`
> > 
> > Bernhard Beschow (3):
> >    hw/timer/i8254_common: Share "iobase" property via base class
> >    hw/arm/omap: Remove unused omap_uart_attach()
> >    hw/isa/i82378: Remove unused "io" attribute
> > 
> >   include/hw/arm/omap.h   | 1 -
> >   hw/char/omap_uart.c     | 9 ---------
> >   hw/i386/kvm/i8254.c     | 1 -
> >   hw/isa/i82378.c         | 1 -
> >   hw/timer/i8254.c        | 6 ------
> >   hw/timer/i8254_common.c | 6 ++++++
> >   6 files changed, 6 insertions(+), 18 deletions(-)
> 
> Do we know who is going to pick up these series? I can send a PR if no-one minds?
> 


Go ahead:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ATB,
> 
> Mark.

Re: [PATCH v3 0/3] Trivial cleanups

[Mark Cave-Ayland]

On 01/06/2023 13:07, Michael S. Tsirkin wrote:

> On Thu, May 25, 2023 at 05:03:15PM +0100, Mark Cave-Ayland wrote:
>> On 23/05/2023 20:56, Bernhard Beschow wrote:
>>
>>> This series:
>>> * Removes dead code from omap_uart and i82378
>>> * Resolves redundant code in the i8254 timer devices
>>>
>>> v3:
>>> * Drop TYPE_ISA_PARALLEL since they became obsolete by
>>>     https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/
>>
>> Oh I didn't see that this had already been merged :/
>>
>> It's not a reason to block this series, but I'd still like to see your
>> changes to ParallelState and ISAParallelState merged separately since they
>> are a better match for our QOM coding standards.
>>
>>> v2:
>>> * Export ParallelState and ISAParallelState (Mark)
>>>
>>> Testing done:
>>> * `make check`
>>>
>>> Bernhard Beschow (3):
>>>     hw/timer/i8254_common: Share "iobase" property via base class
>>>     hw/arm/omap: Remove unused omap_uart_attach()
>>>     hw/isa/i82378: Remove unused "io" attribute
>>>
>>>    include/hw/arm/omap.h   | 1 -
>>>    hw/char/omap_uart.c     | 9 ---------
>>>    hw/i386/kvm/i8254.c     | 1 -
>>>    hw/isa/i82378.c         | 1 -
>>>    hw/timer/i8254.c        | 6 ------
>>>    hw/timer/i8254_common.c | 6 ++++++
>>>    6 files changed, 6 insertions(+), 18 deletions(-)
>>
>> Do we know who is going to pick up these series? I can send a PR if no-one minds?
>>
> 
> 
> Go ahead:
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks Michael! Is there any objection to also including 
https://patchew.org/QEMU/20230531211043.41724-1-shentey@gmail.com/ at the same time?

Bernhard: if you are able to submit a rebased version of the ISA_PARALLEL cleanups at 
https://patchew.org/QEMU/20230521123049.312349-1-shentey@gmail.com/ I think it is 
worth considering those for inclusion in the PR as well (note the comments re: an 
updated commit message and register definitions, but I can't really do this myself 
because of the missing SoB).


ATB,

Mark.

Re: [PATCH v3 0/3] Trivial cleanups

[Michael S. Tsirkin]

On Thu, Jun 01, 2023 at 01:45:47PM +0100, Mark Cave-Ayland wrote:
> On 01/06/2023 13:07, Michael S. Tsirkin wrote:
> 
> > On Thu, May 25, 2023 at 05:03:15PM +0100, Mark Cave-Ayland wrote:
> > > On 23/05/2023 20:56, Bernhard Beschow wrote:
> > > 
> > > > This series:
> > > > * Removes dead code from omap_uart and i82378
> > > > * Resolves redundant code in the i8254 timer devices
> > > > 
> > > > v3:
> > > > * Drop TYPE_ISA_PARALLEL since they became obsolete by
> > > >     https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/
> > > 
> > > Oh I didn't see that this had already been merged :/
> > > 
> > > It's not a reason to block this series, but I'd still like to see your
> > > changes to ParallelState and ISAParallelState merged separately since they
> > > are a better match for our QOM coding standards.
> > > 
> > > > v2:
> > > > * Export ParallelState and ISAParallelState (Mark)
> > > > 
> > > > Testing done:
> > > > * `make check`
> > > > 
> > > > Bernhard Beschow (3):
> > > >     hw/timer/i8254_common: Share "iobase" property via base class
> > > >     hw/arm/omap: Remove unused omap_uart_attach()
> > > >     hw/isa/i82378: Remove unused "io" attribute
> > > > 
> > > >    include/hw/arm/omap.h   | 1 -
> > > >    hw/char/omap_uart.c     | 9 ---------
> > > >    hw/i386/kvm/i8254.c     | 1 -
> > > >    hw/isa/i82378.c         | 1 -
> > > >    hw/timer/i8254.c        | 6 ------
> > > >    hw/timer/i8254_common.c | 6 ++++++
> > > >    6 files changed, 6 insertions(+), 18 deletions(-)
> > > 
> > > Do we know who is going to pick up these series? I can send a PR if no-one minds?
> > > 
> > 
> > 
> > Go ahead:
> > 
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Thanks Michael! Is there any objection to also including
> https://patchew.org/QEMU/20230531211043.41724-1-shentey@gmail.com/ at the
> same time?

I don't know, I wasn't copied on that one.

> Bernhard: if you are able to submit a rebased version of the ISA_PARALLEL
> cleanups at
> https://patchew.org/QEMU/20230521123049.312349-1-shentey@gmail.com/ I think
> it is worth considering those for inclusion in the PR as well (note the
> comments re: an updated commit message and register definitions, but I can't
> really do this myself because of the missing SoB).
> 
> 
> ATB,
> 
> Mark.

Re: [PATCH v3 0/3] Trivial cleanups

[Mark Cave-Ayland]

On 01/06/2023 13:07, Michael S. Tsirkin wrote:

> On Thu, May 25, 2023 at 05:03:15PM +0100, Mark Cave-Ayland wrote:
>> On 23/05/2023 20:56, Bernhard Beschow wrote:
>>
>>> This series:
>>> * Removes dead code from omap_uart and i82378
>>> * Resolves redundant code in the i8254 timer devices
>>>
>>> v3:
>>> * Drop TYPE_ISA_PARALLEL since they became obsolete by
>>>     https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/
>>
>> Oh I didn't see that this had already been merged :/
>>
>> It's not a reason to block this series, but I'd still like to see your
>> changes to ParallelState and ISAParallelState merged separately since they
>> are a better match for our QOM coding standards.
>>
>>> v2:
>>> * Export ParallelState and ISAParallelState (Mark)
>>>
>>> Testing done:
>>> * `make check`
>>>
>>> Bernhard Beschow (3):
>>>     hw/timer/i8254_common: Share "iobase" property via base class
>>>     hw/arm/omap: Remove unused omap_uart_attach()
>>>     hw/isa/i82378: Remove unused "io" attribute
>>>
>>>    include/hw/arm/omap.h   | 1 -
>>>    hw/char/omap_uart.c     | 9 ---------
>>>    hw/i386/kvm/i8254.c     | 1 -
>>>    hw/isa/i82378.c         | 1 -
>>>    hw/timer/i8254.c        | 6 ------
>>>    hw/timer/i8254_common.c | 6 ++++++
>>>    6 files changed, 6 insertions(+), 18 deletions(-)
>>
>> Do we know who is going to pick up these series? I can send a PR if no-one minds?
>>
> 
> 
> Go ahead:
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Thanks! I've applied this series to my qemu-sparc branch.


ATB,

Mark.

Re: [PATCH v3 0/3] Trivial cleanups

[Bernhard Beschow]

Am 1. Juni 2023 12:45:47 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>On 01/06/2023 13:07, Michael S. Tsirkin wrote:
>
>> On Thu, May 25, 2023 at 05:03:15PM +0100, Mark Cave-Ayland wrote:
>>> On 23/05/2023 20:56, Bernhard Beschow wrote:
>>> 
>>>> This series:
>>>> * Removes dead code from omap_uart and i82378
>>>> * Resolves redundant code in the i8254 timer devices
>>>> 
>>>> v3:
>>>> * Drop TYPE_ISA_PARALLEL since they became obsolete by
>>>>     https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/
>>> 
>>> Oh I didn't see that this had already been merged :/
>>> 
>>> It's not a reason to block this series, but I'd still like to see your
>>> changes to ParallelState and ISAParallelState merged separately since they
>>> are a better match for our QOM coding standards.
>>> 
>>>> v2:
>>>> * Export ParallelState and ISAParallelState (Mark)
>>>> 
>>>> Testing done:
>>>> * `make check`
>>>> 
>>>> Bernhard Beschow (3):
>>>>     hw/timer/i8254_common: Share "iobase" property via base class
>>>>     hw/arm/omap: Remove unused omap_uart_attach()
>>>>     hw/isa/i82378: Remove unused "io" attribute
>>>> 
>>>>    include/hw/arm/omap.h   | 1 -
>>>>    hw/char/omap_uart.c     | 9 ---------
>>>>    hw/i386/kvm/i8254.c     | 1 -
>>>>    hw/isa/i82378.c         | 1 -
>>>>    hw/timer/i8254.c        | 6 ------
>>>>    hw/timer/i8254_common.c | 6 ++++++
>>>>    6 files changed, 6 insertions(+), 18 deletions(-)
>>> 
>>> Do we know who is going to pick up these series? I can send a PR if no-one minds?
>>> 
>> 
>> 
>> Go ahead:
>> 
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
>Thanks Michael! Is there any objection to also including https://patchew.org/QEMU/20230531211043.41724-1-shentey@gmail.com/ at the same time?
>
>Bernhard: if you are able to submit a rebased version of the ISA_PARALLEL cleanups at https://patchew.org/QEMU/20230521123049.312349-1-shentey@gmail.com/ I think it is worth considering those for inclusion in the PR as well (note the comments re: an updated commit message and register definitions, but I can't really do this myself because of the missing SoB).

What could I put into the commit message?

I'm also wondering: Why export the structure but not the register definitions? Are the register definitions not part of the interface? I think these could be used in unittests -- if we had any -- to avoid magic numbers.

Best regards,
Bernhard

>
>
>ATB,
>
>Mark.
>

Re: [PATCH v3 0/3] Trivial cleanups

[Mark Cave-Ayland]

On 05/06/2023 07:58, Bernhard Beschow wrote:

> Am 1. Juni 2023 12:45:47 UTC schrieb Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:
>> On 01/06/2023 13:07, Michael S. Tsirkin wrote:
>>
>>> On Thu, May 25, 2023 at 05:03:15PM +0100, Mark Cave-Ayland wrote:
>>>> On 23/05/2023 20:56, Bernhard Beschow wrote:
>>>>
>>>>> This series:
>>>>> * Removes dead code from omap_uart and i82378
>>>>> * Resolves redundant code in the i8254 timer devices
>>>>>
>>>>> v3:
>>>>> * Drop TYPE_ISA_PARALLEL since they became obsolete by
>>>>>      https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/
>>>>
>>>> Oh I didn't see that this had already been merged :/
>>>>
>>>> It's not a reason to block this series, but I'd still like to see your
>>>> changes to ParallelState and ISAParallelState merged separately since they
>>>> are a better match for our QOM coding standards.
>>>>
>>>>> v2:
>>>>> * Export ParallelState and ISAParallelState (Mark)
>>>>>
>>>>> Testing done:
>>>>> * `make check`
>>>>>
>>>>> Bernhard Beschow (3):
>>>>>      hw/timer/i8254_common: Share "iobase" property via base class
>>>>>      hw/arm/omap: Remove unused omap_uart_attach()
>>>>>      hw/isa/i82378: Remove unused "io" attribute
>>>>>
>>>>>     include/hw/arm/omap.h   | 1 -
>>>>>     hw/char/omap_uart.c     | 9 ---------
>>>>>     hw/i386/kvm/i8254.c     | 1 -
>>>>>     hw/isa/i82378.c         | 1 -
>>>>>     hw/timer/i8254.c        | 6 ------
>>>>>     hw/timer/i8254_common.c | 6 ++++++
>>>>>     6 files changed, 6 insertions(+), 18 deletions(-)
>>>>
>>>> Do we know who is going to pick up these series? I can send a PR if no-one minds?
>>>>
>>>
>>>
>>> Go ahead:
>>>
>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> Thanks Michael! Is there any objection to also including https://patchew.org/QEMU/20230531211043.41724-1-shentey@gmail.com/ at the same time?
>>
>> Bernhard: if you are able to submit a rebased version of the ISA_PARALLEL cleanups at https://patchew.org/QEMU/20230521123049.312349-1-shentey@gmail.com/ I think it is worth considering those for inclusion in the PR as well (note the comments re: an updated commit message and register definitions, but I can't really do this myself because of the missing SoB).
> 
> What could I put into the commit message?

That comment came from Zoltan (see 
https://patchew.org/QEMU/20230521123049.312349-1-shentey@gmail.com/20230521123049.312349-5-shentey@gmail.com/#77413450-244e-287b-ad21-e57cb5e2abf5@eik.bme.hu). 
Zoltan, would you like to suggest some alternative wording?

If not, feel free to take my message at 
https://patchew.org/QEMU/20230604131450.428797-1-mark.cave-ayland@ilande.co.uk/20230604131450.428797-14-mark.cave-ayland@ilande.co.uk/ 
and tweak it accordingly.

> I'm also wondering: Why export the structure but not the register definitions? Are the register definitions not part of the interface? I think these could be used in unittests -- if we had any -- to avoid magic numbers.

In theory that could be possible, but it's not something that people have requested 
(yet). From the QEMU perspective a device is something with memory regions and gpios 
that can be wired up within a board, so unless the #defines are used directly within 
ParallelState it doesn't make too much sense to export them currently.


ATB,

Mark.

Re: [PATCH v3 0/3] Trivial cleanups

[BALATON Zoltan]

On Tue, 6 Jun 2023, Mark Cave-Ayland wrote:
> On 05/06/2023 07:58, Bernhard Beschow wrote:
>> Am 1. Juni 2023 12:45:47 UTC schrieb Mark Cave-Ayland 
>> <mark.cave-ayland@ilande.co.uk>:
>>> On 01/06/2023 13:07, Michael S. Tsirkin wrote:
>>> 
>>>> On Thu, May 25, 2023 at 05:03:15PM +0100, Mark Cave-Ayland wrote:
>>>>> On 23/05/2023 20:56, Bernhard Beschow wrote:
>>>>> 
>>>>>> This series:
>>>>>> * Removes dead code from omap_uart and i82378
>>>>>> * Resolves redundant code in the i8254 timer devices
>>>>>> 
>>>>>> v3:
>>>>>> * Drop TYPE_ISA_PARALLEL since they became obsolete by
>>>>>>      https://lore.kernel.org/qemu-devel/20230522115014.1110840-9-thuth@redhat.com/
>>>>> 
>>>>> Oh I didn't see that this had already been merged :/
>>>>> 
>>>>> It's not a reason to block this series, but I'd still like to see your
>>>>> changes to ParallelState and ISAParallelState merged separately since 
>>>>> they
>>>>> are a better match for our QOM coding standards.
>>>>> 
>>>>>> v2:
>>>>>> * Export ParallelState and ISAParallelState (Mark)
>>>>>> 
>>>>>> Testing done:
>>>>>> * `make check`
>>>>>> 
>>>>>> Bernhard Beschow (3):
>>>>>>      hw/timer/i8254_common: Share "iobase" property via base class
>>>>>>      hw/arm/omap: Remove unused omap_uart_attach()
>>>>>>      hw/isa/i82378: Remove unused "io" attribute
>>>>>>
>>>>>>     include/hw/arm/omap.h   | 1 -
>>>>>>     hw/char/omap_uart.c     | 9 ---------
>>>>>>     hw/i386/kvm/i8254.c     | 1 -
>>>>>>     hw/isa/i82378.c         | 1 -
>>>>>>     hw/timer/i8254.c        | 6 ------
>>>>>>     hw/timer/i8254_common.c | 6 ++++++
>>>>>>     6 files changed, 6 insertions(+), 18 deletions(-)
>>>>> 
>>>>> Do we know who is going to pick up these series? I can send a PR if 
>>>>> no-one minds?
>>>>> 
>>>> 
>>>> 
>>>> Go ahead:
>>>> 
>>>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>> 
>>> Thanks Michael! Is there any objection to also including 
>>> https://patchew.org/QEMU/20230531211043.41724-1-shentey@gmail.com/ at the 
>>> same time?
>>> 
>>> Bernhard: if you are able to submit a rebased version of the ISA_PARALLEL 
>>> cleanups at 
>>> https://patchew.org/QEMU/20230521123049.312349-1-shentey@gmail.com/ I 
>>> think it is worth considering those for inclusion in the PR as well (note 
>>> the comments re: an updated commit message and register definitions, but I 
>>> can't really do this myself because of the missing SoB).
>> 
>> What could I put into the commit message?
>
> That comment came from Zoltan (see 
> https://patchew.org/QEMU/20230521123049.312349-1-shentey@gmail.com/20230521123049.312349-5-shentey@gmail.com/#77413450-244e-287b-ad21-e57cb5e2abf5@eik.bme.hu). 
> Zoltan, would you like to suggest some alternative wording?
>
> If not, feel free to take my message at 
> https://patchew.org/QEMU/20230604131450.428797-1-mark.cave-ayland@ilande.co.uk/20230604131450.428797-14-mark.cave-ayland@ilande.co.uk/ 
> and tweak it accordingly.

I'm OK with your suggestion too or maybe something like:

Export ParallelState to allow embedding it in other devices.

If you just want the

#define TYPE_ISA_PARALLEL "isa-parallel"
OBJECT_DECLARE_SIMPLE_TYPE(ISAParallelState, ISA_PARALLEL)

part to be in a header then moving the structure there as well is not 
necessary, only when you also want to use the structure somewhere where 
its size is needed like adding it to another structure.

>> I'm also wondering: Why export the structure but not the register 
>> definitions? Are the register definitions not part of the interface? I 
>> think these could be used in unittests -- if we had any -- to avoid magic 
>> numbers.
>
> In theory that could be possible, but it's not something that people have 
> requested (yet). From the QEMU perspective a device is something with memory 
> regions and gpios that can be wired up within a board, so unless the #defines 
> are used directly within ParallelState it doesn't make too much sense to 
> export them currently.

On the other hand keeping the structure and defines in the c file ensures 
encapsulation so nothing else will mess with the internals of the object 
and keep these as implementation detail (it's also easier to work with a 
single file than having to look up struct definition elsewhere) so I'd 
prefer keeping things together unless the struct is needed elsewhere. The 
coding style does not clearly say which is preferred, in fact it only 
mentions thet OBJECT_DECLARE_SIMPLE_TYPE ofthen goes in a header but the 
struct is declared separately. The moving of structs to headers only 
started with preferring embedded objects over allocating them so we don't 
have to care about freeing them which needs the struct definition in the 
header breaking encapsulation.

Regards,
BALATON Zoltan