Re: [PATCH] hw/ide: Add the possibility to disable the CompactFlash device in the build

[Thomas Huth]

On 01/02/2024 13.39, BALATON Zoltan wrote:
> On Thu, 1 Feb 2024, Thomas Huth wrote:
> > For distros like downstream RHEL, it would be helpful to allow to disable
> > the CompactFlash device. For making this possible, we need a separate
> > Kconfig switch for this device, and the code should reside in a separate
> > file.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> > hw/ide/qdev-ide.h  | 41 ++++++++++++++++++++++++++++++++
> > hw/ide/cf.c        | 58 ++++++++++++++++++++++++++++++++++++++++++++++
> > hw/ide/qdev.c      | 51 ++--------------------------------------
> > hw/ide/Kconfig     |  4 ++++
> > hw/ide/meson.build |  1 +
> > 5 files changed, 106 insertions(+), 49 deletions(-)
> > create mode 100644 hw/ide/qdev-ide.h
> > create mode 100644 hw/ide/cf.c
> >
> > diff --git a/hw/ide/qdev-ide.h b/hw/ide/qdev-ide.h
> > new file mode 100644
> > index 0000000000..3dd977466c
> > --- /dev/null
> > +++ b/hw/ide/qdev-ide.h
>
> This may be unrelated to this patch but we already have 
> include/hw/ide/internal.h which may be a place these should go in but that 
> header is in inlcude because some files outside hw/ide include it. I've 
> found three places that include ide/internal.h: hw/arm/sbsa-ref.c, 
> hw/i386/pc.c and hw/misc/macio.h. Only macio is really needing internal IDE 
> parts the other two just uses some functions so macio is probably the reason 
> this wasn't cleaned up yet. In any case, maybe this could go in 
> include/hw/ide/internal.h to avoid introducing a new header or somehow make 
> this a local header where non-public parts of hw/ide/internal.h could be 
> moved in the future. Such as rename include/hw/ide/internal.h to ide.h and 
> name this one internal.h maybe?

I don't like headers that much that just collect a lot of only slightly 
related things. That only causes problems again when you have to unentangle 
the stuff one day. So what's wrong with having a dedicated header for the 
stuff in hw/ide/qdev.c ?

 Thomas