[Qemu-devel] Re: [PATCH v5 2/6] MIPS: Initial support of vt82686b south bridge used by fulong mini pc

[Juan Quintela]

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.