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 
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 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.