On Thu, 1 Feb 2024, Thomas Huth wrote:
On 01/02/2024 13.54, BALATON Zoltan wrote:
On Thu, 1 Feb 2024, Thomas Huth wrote:
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 <[email protected]>
---
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 ?
Maybe that it's not obvious from the name that it belongs to qdev.c as the
names are not the same.
I didn't want to just name the header "qdev.h" since that could easily be
confused in #include statements...
IMHO qdev.c is already a very bad idea for a file name here... maybe
something like ide-dev.c and ide-dev.h would be better?
The comment at the beginning of qdev.c says "ide bus support" but a lot of
functions in it have ide_dev prefix. Maybe this comes from the original
qdev-ification of this code and then adding some more unrelated parts in
this file. It's clearly beyond scope of this patch to clean all this but
since you're changing it maybe this is a good opportunity to reduce the
mess a bit. I'm OK with renaming qdev.c to ide-dev.c or qdev-ide.c or
whatever else as long as it's clear that the header belongs to the c file.
Also some of the qdev stuff that should be in this header are in
include/hw/ide/internal.h so these will still be split arbitrarily.
Oh, well, it seems to be a mess already... hw/ide/pci.h includes the
internal.h header and thus opens the internal definitions to all PCI-based
IDE devices :-/
I've missed that so besides files directly including internal.h there
could be others using some stuff from it through pci.h so this makes it
more difficult to untangle.
If we can agree on a better name for qdev-ide.h, I can try to clean that mess
up a little bit...
My comment was only that if you make changes then do it in a way that
leaves the possibility to move stuff here to clean the current situation
but if you can also do some of it now that's even better but not required.
Regards,
BALATON Zoltan
