[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH v5 2/6] MIPS: Initial support of vt82686b south
From: |
Juan Quintela |
Subject: |
[Qemu-devel] Re: [PATCH v5 2/6] MIPS: Initial support of vt82686b south bridge used by fulong mini pc |
Date: |
Mon, 28 Jun 2010 21:23:01 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Huacai Chen <address@hidden> wrote:
> Signed-off-by: Huacai Chen <address@hidden>
> +static void superio_ioport_writeb(void *opaque, uint32_t addr, uint32_t data)
> +{
> + int can_write;
> + SuperIOConfig *superio_conf = (SuperIOConfig *)opaque;
Useless cast from void *.
> +static uint32_t superio_ioport_readb(void *opaque, uint32_t addr)
> +{
> + SuperIOConfig *superio_conf = (SuperIOConfig *)opaque;
>
the same.
> +static void vt82c686b_save(QEMUFile * f, void *opaque)
> +{
> + PCIDevice *d = opaque;
> + pci_device_save(d, f);
> +}
> +
> +static int vt82c686b_load(QEMUFile * f, void *opaque, int version_id)
> +{
> + PCIDevice *d = opaque;
> + if (version_id != 1)
> + return -EINVAL;
> + return pci_device_load(d, f);
> +}
You use vmstate in the rest of devices, why use old style here?
> +typedef struct VT686AC97State {
> + PCIDevice dev;
> + int unused;
not needed really.
> +} VT686AC97State;
> +
> +typedef struct VT686MC97State {
> + PCIDevice dev;
> + int unused;
also not needed.
> +} VT686MC97State;
> +
> +#define RTC_EN (1 << 10)
> +#define PWRBTN_EN (1 << 8)
> +#define GBL_EN (1 << 5)
> +#define TMROF_EN (1 << 0)
> +#define SCI_EN (1 << 0)
not used in the rest of the code. Suspicions to be the same bit that
previous one.
> +
> + s = (VT686AC97State *)pci_register_device(bus,
> + "vt82c686b_AC97",
> sizeof(VT686AC97State),
> + devfn, NULL, NULL);
use DO_UPCAST like rest of driver instead of cast.
> +void vt82c686b_mc97_init(PCIBus *bus, int devfn)
> +{
> + VT686MC97State *s;
> + uint8_t *pci_conf;
> +
> + s = (VT686MC97State *)pci_register_device(bus,
> + "vt82c686b_MC97",
> sizeof(VT686MC97State),
> + devfn, NULL, NULL);
Same than previous comment.
> + /* set super io config */
> + vt686->superio_conf = qemu_mallocz(sizeof(SuperIOConfig));
Why do you use a pointer instead of changing it to a embeded estruct
inside struct VT82C686BState?
Later, Juan.