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.