Laszlo Ersek <ler...@redhat.com> writes:
> The sysbus_get_fw_dev_path() function formats OpenFirmware device path
> nodes ("driver-name@unit-address") for sysbus devices. The first choice
> for "unit-address" is the base address of the device's first MMIO region.
> The second choice is its first IO port.
>
> However, if two sysbus devices with the same "driver-name" lack both MMIO
> and PIO resources, then there is no good way to distinguish them based on
> their OFW nodes, because in this case unit-address is omitted completely
> for both devices.
Got an example for such a device? Mind adding it to the commit message?
> For the sake of such devices, introduce the explicit_ofw_unit_address()
> "virtual member function". With this function, each sysbus device in the
> same SysBusDeviceClass can state its own address.
>
> Cc: Markus Armbruster <arm...@redhat.com>
> Cc: Marcel Apfelbaum <mar...@redhat.com>
> Cc: Michael S. Tsirkin <m...@redhat.com>
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
>
> Notes:
> v4:
> - Yet another approach. Instead of allowing the creator of the device to
> set a string property statically, introduce a class level callback.
>
> v3:
> - new in v3
> - new approach
>
> include/hw/sysbus.h | 9 +++++++++
> hw/core/sysbus.c | 13 +++++++++++++
> 2 files changed, 22 insertions(+)
>
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index d1f3f00..63b036b 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -41,6 +41,15 @@ typedef struct SysBusDeviceClass {
> /*< public >*/
>
> int (*init)(SysBusDevice *dev);
> +
> + /*
> + * Sometimes a class of SysBusDevices has neither MMIO nor PIO resources,
> + * yet instances of it would like to distinguish themselves, in
> + * OpenFirmware device paths, from other instances of the same class on
> the
> + * same sysbus. For that end we expose this callback. It returns a
> + * dynamically allocated string.
> + */
> + char *(*explicit_ofw_unit_address)(SysBusDevice *dev);
I prefer function comments to follow a strict pattern:
/*
* Headline explaining the function's purpose[*]
* Zero or more paragraphs explaining preconditions, side effects,
* return values, error conditions.
*/
[*] If you can't come up with a headline fitting into a single line,
chances are the function does too many things.
> } SysBusDeviceClass;
>
> struct SysBusDevice {
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index 0ebb4e2..a0ec814 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -281,6 +281,7 @@ static void sysbus_dev_print(Monitor *mon, DeviceState
> *dev, int indent)
> static char *sysbus_get_fw_dev_path(DeviceState *dev)
> {
> SysBusDevice *s = SYS_BUS_DEVICE(dev);
> + SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(s);
>
> if (s->num_mmio) {
> return g_strdup_printf("%s@"TARGET_FMT_plx, qdev_fw_name(dev),
> @@ -289,6 +290,18 @@ static char *sysbus_get_fw_dev_path(DeviceState *dev)
> if (s->num_pio) {
> return g_strdup_printf("%s@i%04x", qdev_fw_name(dev), s->pio[0]);
> }
> + if (sbc->explicit_ofw_unit_address) {
> + char *addr;
> +
> + addr = sbc->explicit_ofw_unit_address(s);
> + if (addr) {
> + char *fw_dev_path;
> +
> + fw_dev_path = g_strdup_printf("%s@%s", qdev_fw_name(dev), addr);
> + g_free(addr);
> + return fw_dev_path;
> + }
> + }
> return g_strdup(qdev_fw_name(dev));
> }
In short functions like this one, I prefer to have declarations out of
the way in one place rather than cluttering inner blocks. Matter of
taste, so
Reviewed-by: Markus Armbruster <arm...@redhat.com>
