BALATON Zoltan <address@hidden> writes:
On Sun, 1 Mar 2020, Mark Cave-Ayland wrote:
On 29/02/2020 23:02, BALATON Zoltan wrote:
We'll need a flag for implementing some device specific behaviour in
via-ide but we already have a currently CMD646 specific field that can
be repurposed for this and leave room for furhter flags if needed in
the future. This patch changes the "secondary" field to "flags" and
define the flags for CMD646 and via-ide and change CMD646 and its
users accordingly.
Signed-off-by: BALATON Zoltan <address@hidden>
---
hw/alpha/dp264.c | 2 +-
hw/ide/cmd646.c | 12 ++++++------
hw/sparc64/sun4u.c | 9 ++-------
include/hw/ide.h | 4 ++--
include/hw/ide/pci.h | 7 ++++++-
5 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/hw/alpha/dp264.c b/hw/alpha/dp264.c
[...]
@@ -317,20 +317,20 @@ static void pci_cmd646_ide_exitfn(PCIDevice *dev)
}
}
-void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
- int secondary_ide_enabled)
+void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn,
+ bool secondary_ide_enabled)
{
PCIDevice *dev;
- dev = pci_create(bus, -1, "cmd646-ide");
- qdev_prop_set_uint32(&dev->qdev, "secondary", secondary_ide_enabled);
+ dev = pci_create(bus, devfn, "cmd646-ide");
+ qdev_prop_set_bit(&dev->qdev, "secondary", secondary_ide_enabled);
qdev_init_nofail(&dev->qdev);
pci_ide_create_devs(dev, hd_table);
}
Note that legacy init functions such as pci_cmd646_ide_init() should be removed
where
possible, and in fact I posted a patch last week to completely remove it. This
is
because using qdev directly allows each board to wire up the device as required,
rather than pushing it down into a set of init functions with different
defaults.
Given that you're working in this area, I'd highly recommend doing the same for
via_ide_init() too.
I could do that, however these ide init functions seem to exist for
piix, cmd646 and via-ide so that pci_ide_create_devs function is kept
local to hw/ide. Nothing else called that func apart from sun4u so
I've chosen this way to keep consistency (also keeps property type at
one place instead of needing to change each board that sets
property). If the consensus is that getting rid of these init funcs
even if that means pci_ide_create_devs will not be local to ide any
more I can go that way but would like to hear opinion of ide
maintainer as well.
I think Mark's point is that modelling a device and wiring up a device
model are separate things, and the latter belongs to the board.
pci_cmd646_ide_init() is a helper for boards. Similar helpers exist
elsewhere.
In the oldest stratum of qdev code, such helpers were static inline
functions in the device model's .h. That way, they're kind of separate
from the device model proper, in the .c, and kind of in the board code
where they belong, via inlining. I've always considered that a terrible
idea; it's "kind of" as in "not really". Over time, practice moved
first to putting the helpers into .c, then to open-coding the wiring
where it belongs: in the boards.
A few helper functions have survived, e.g. in hw/lm32/milkymist-hw.h,
and the IDE helpers we're discussing here.
Of course, when the code to wire up certain devices gets overly
repetitive, factoring out common code into helpers can make sense. But
where to put them? I can't see an obvious home for common board
helpers. We tend to put these wiring helpers into a device model's .c
code for want of a better place. Tolerable, I think.