Re: [Qemu-devel] [PATCH v4 1/2] Add TEWS TPCI200 IndustryPack emulation

[Andreas Färber]

Am 08.01.2013 02:10, schrieb Alberto Garcia:
> The TPCI200 is a PCI board that supports up to 4 IndustryPack modules.
> 
> A new bus type called 'IndustryPack' has been created so any
> compatible module can be attached to this board.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  default-configs/pci.mak |    1 +
>  hw/Makefile.objs        |    3 +
>  hw/ipack.c              |  106 ++++++++
>  hw/ipack.h              |   75 ++++++
>  hw/pci/pci_ids.h        |    3 +
>  hw/tpci200.c            |  667 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  6 ficheiros modificados, 855 adições(+)
>  create mode 100644 hw/ipack.c
>  create mode 100644 hw/ipack.h
>  create mode 100644 hw/tpci200.c
> 
> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> index ae9d1eb..ee2d18d 100644
> --- a/default-configs/pci.mak
> +++ b/default-configs/pci.mak
> @@ -21,3 +21,4 @@ CONFIG_ESP=y
>  CONFIG_ESP_PCI=y
>  CONFIG_SERIAL=y
>  CONFIG_SERIAL_PCI=y
> +CONFIG_IPACK=y
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index b8bbed3..a42518e 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -100,6 +100,9 @@ common-obj-$(CONFIG_XGMAC) += xgmac.o
>  # PCI watchdog devices
>  common-obj-$(CONFIG_PCI) += wdt_i6300esb.o
>  
> +# IndustryPack
> +common-obj-$(CONFIG_IPACK) += tpci200.o ipack.o
> +
>  # PCI network cards
>  common-obj-$(CONFIG_NE2000_PCI) += ne2000.o
>  common-obj-$(CONFIG_EEPRO100_PCI) += eepro100.o
> diff --git a/hw/ipack.c b/hw/ipack.c
> new file mode 100644
> index 0000000..59e272b
> --- /dev/null
> +++ b/hw/ipack.c
> @@ -0,0 +1,106 @@
> +/*
> + * QEMU IndustryPack emulation
> + *
> + * Copyright (C) 2012 Igalia, S.L.
> + * Author: Alberto Garcia <address@hidden>
> + *
> + * This code is licensed under the GNU GPL v2 or (at your option) any
> + * later version.
> + */
> +
> +#include "ipack.h"
> +
> +IPackDevice *ipack_device_find(IPackBus *bus, int32_t slot)
> +{
> +    BusChild *kid;
> +
> +    QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
> +        DeviceState *qdev = kid->child;
> +        IPackDevice *ip = DO_UPCAST(IPackDevice, qdev, qdev);

IPACK_DEVICE(qdev)

> +        if (ip->slot == slot) {
> +            return ip;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static int ipack_device_dev_init(DeviceState *qdev)
> +{
> +    IPackBus *bus = DO_UPCAST(IPackBus, qbus, qdev->parent_bus);

IPACK_BUS()

> +    IPackDevice *dev = DO_UPCAST(IPackDevice, qdev, qdev);
[snip]

IPACK_DEVICE()

You have them defined in the header, please use them consistently.

Also, please avoid accessing internals like &s->bus.qbus (BUS(s->bus))
or &s->dev.qdev (DEVICE(s->dev)).

Another thing to check (could be a follow-up) is whether the initfn can
be split into instance_init (e.g., pci_set_*?) and initfn.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg