qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 0/3] Trivial cleanups


From: BALATON Zoltan
Subject: Re: [PATCH v3 0/3] Trivial cleanups
Date: Tue, 6 Jun 2023 11:55:31 +0200 (CEST)

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 
20230604131450.428797-1-mark.cave-ayland@ilande.co.uk/20230604131450.428797-14-mark.cave-ayland@ilande.co.uk/">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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]