Re: [PATCH v2 17/24] hw/ide: Declare ide_get_[geometry/bios_chs_trans] in 'hw/ide/internal.h'

[Philippe Mathieu-Daudé]

On 27/2/23 12:17, Alex Bennée wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > ide_get_geometry() and ide_get_bios_chs_trans() are only
> > used by the TYPE_PC_MACHINE.
> > "hw/ide.h" is a mixed bag of lost IDE declarations. In order
> > to remove this (almost) pointless header soon, move these
> > declarations to "hw/ide/internal.h".
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/i386/pc.c              | 3 ++-
> >   include/hw/ide.h          | 4 ----
> >   include/hw/ide/internal.h | 4 ++++
> >   3 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 6e592bd969..79297a6ecd 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -34,7 +34,8 @@
> >   #include "hw/i386/vmport.h"
> >   #include "sysemu/cpus.h"
> >   #include "hw/block/fdc.h"
> > -#include "hw/ide.h"
> > +#include "hw/ide/internal.h"
> > +#include "hw/ide/isa.h"
>
> I do kind of wonder why hw/ide/internal.h isn't in the appropriate
> subdir (e.g. hw/ide and included as #include "internal.h") rather than
> the "public" include directory.

It should be internal but is included by PCI and MacIO:

include/hw/ide/pci.h:4:#include "hw/ide/internal.h"
include/hw/misc/macio/macio.h:31:#include "hw/ide/internal.h"

Long term I'd like to split it in internal vs public API.

> However QEMU isn't super consistent with
> that:
>
>    ➜  find . -iname "internal.h"
>    ./accel/tcg/internal.h
>    ./target/ppc/internal.h
>    ./target/mips/internal.h
>    ./target/hexagon/internal.h
>    ./include/hw/ide/internal.h
>    🕙11:15:58 alex@zen:qemu.git  on  review/qom-housekeeping-v2 [$?] took 7s
>    ➜  find . -iname "internals.h"
>    ./tests/fp/berkeley-softfloat-3/source/include/internals.h
>    ./target/arm/internals.h
>    ./target/riscv/internals.h
>    ./target/loongarch/internals.h
>    ./gdbstub/internals.h
>
> Anyway:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thanks!