On 16/06/17 22:28, Laszlo Ersek wrote:
> This patch is generally good, but I'd like to suggest improvements:
>
> (1) In the commit message, please mention that we are exposing the
> internals of FWCfgIoState and FWCfgMemState so that board code can map
> the MemoryRegion fields (such as comb_iomem) *by name*.
>
> (2) FWCfgEntry need not be moved from the C file to the header file,
> since we never depend on the *size* of that structure. Instead, please
> add a single line to "include/qemu/typedefs.h":
>
> typedef struct FWCfgEntry FWCfgEntry;
>
> and modify
>
> typedef struct FWCfgEntry {
> ...
> } FWCfgEntry;
>
> in "fw_cfg.c" to just
>
> struct FWCfgEntry {
> ...
> };
>
> Then "fw_cfg.h" can simply include "typedefs.h" and say
>
> ...
> FWCfgEntry *entries[2];
> ...
>
> IOW, it's fine to leave FWCfgEntry an incomplete type in "fw_cfg.h".
>
> (3) When you fix up patch #1 like I requested, removing the
> "FWCfgIoState.iobase" and "FWCfgIoState.dma_iobase" fields, please don't
> forget to update this patch as well, so that you not re-introduce those
> fields in the header file.
>
> ... I'm positively satisfied with this series (I plan to "grant" my R-b
> for PATCH v5 5/5, with the above updates), but given that I'm no QOM
> expert by any means, I'd like someone with more QOM experience to review
> the patchset as well.
Thanks for the feedback! I've just revised the patchset based upon your
comments and will post the v5 to the list shortly.
ATB,
Mark.
