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

[Thomas Huth <thuth@redhat.com>]

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