[Qemu-devel] Re: [PATCH 09/15] eepro100: convert to new pci interface

[Michael S. Tsirkin]

On Wed, Feb 10, 2010 at 07:32:10AM +0100, Stefan Weil wrote:
> See my inline comments.
> 
> 
> Anthony Liguori schrieb:
> >  - Removed some dead defines for TARGET_I386 so that we could build once
> >
> > Signed-off-by: Anthony Liguori <address@hidden>
> > ---
> >  hw/eepro100.c |  238 
> > ++++++++++++++-------------------------------------------
> >  1 files changed, 57 insertions(+), 181 deletions(-)
> >
> > diff --git a/hw/eepro100.c b/hw/eepro100.c
> > index b33dbb8..16230c9 100644
> > --- a/hw/eepro100.c
> > +++ b/hw/eepro100.c
> > @@ -33,10 +33,6 @@
> >   * Open Source Software Developer Manual
> >   */
> >  
> > -#if defined(TARGET_I386)
> > -# warning "PXE boot still not working!"
> > -#endif
> > -
> >   
> 
> You did not fix PXE boot here, did you?
> So the warning or a comment should stay there.
> 
> >  #include <stddef.h>             /* offsetof */
> >  #include <stdbool.h>
> >  #include "hw.h"
> >   
> ...
> 
> 
> > -static void ioport_write2(void *opaque, uint32_t addr, uint32_t val)
> > -{
> > -    EEPRO100State *s = opaque;
> > -    eepro100_write2(s, addr - s->region[1], val);
> > -}
> > -
> > -static void ioport_write4(void *opaque, uint32_t addr, uint32_t val)
> > -{
> > -    EEPRO100State *s = opaque;
> > -    eepro100_write4(s, addr - s->region[1], val);
> > -}
> > -
> >  /***********************************************************/
> >  /* PCI EEPRO100 definitions */
> >  
> > -static void pci_map(PCIDevice * pci_dev, int region_num,
> > -                    pcibus_t addr, pcibus_t size, int type)
> > +static void pci_io_write(PCIDevice *dev, pcibus_t addr, int size,
> > +                         uint32_t value)
> >  {
> > -    EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
> > -
> > -    TRACE(OTHER, logout("region %d, addr=0x%08"FMT_PCIBUS", "
> > -          "size=0x%08"FMT_PCIBUS", type=%d\n",
> > -          region_num, addr, size, type));
> > +    EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, dev);
> >   
> 
> Please don't change the name of the PCIDevice pointer argument
> from pci_dev to dev.
> 
> This dev, dev in DO_UPCAST is ugly and misleading.

It's a matter of taste: I actually think it's pretty: I never remember
which argument of DO_UPCAST is which, and if both are dev it doesn't
matter.

> 
> >  
> > -    assert(region_num == 1);
> > -    register_ioport_write(addr, size, 1, ioport_write1, s);
> > -    register_ioport_read(addr, size, 1, ioport_read1, s);
> > -    register_ioport_write(addr, size, 2, ioport_write2, s);
> > -    register_ioport_read(addr, size, 2, ioport_read2, s);
> > -    register_ioport_write(addr, size, 4, ioport_write4, s);
> > -    register_ioport_read(addr, size, 4, ioport_read4, s);
> > -
> > -    s->region[region_num] = addr;
> > -}
> > -
> > -/*****************************************************************************
> > - *
> > - * Memory mapped I/O.
> > - *
> > - 
> > ****************************************************************************/
> > -
> > -static void pci_mmio_writeb(void *opaque, target_phys_addr_t addr, 
> > uint32_t val)
> > -{
> > -    EEPRO100State *s = opaque;
> > -    //~ logout("addr=%s val=0x%02x\n", regname(addr), val);
> > -    eepro100_write1(s, addr, val);
> > -}
> > -
> > -static void pci_mmio_writew(void *opaque, target_phys_addr_t addr, 
> > uint32_t val)
> > -{
> > -    EEPRO100State *s = opaque;
> > -    //~ logout("addr=%s val=0x%02x\n", regname(addr), val);
> > -    eepro100_write2(s, addr, val);
> > -}
> > -
> > -static void pci_mmio_writel(void *opaque, target_phys_addr_t addr, 
> > uint32_t val)
> > -{
> > -    EEPRO100State *s = opaque;
> > -    //~ logout("addr=%s val=0x%02x\n", regname(addr), val);
> > -    eepro100_write4(s, addr, val);
> > -}
> > -
> > -static uint32_t pci_mmio_readb(void *opaque, target_phys_addr_t addr)
> > -{
> > -    EEPRO100State *s = opaque;
> > -    //~ logout("addr=%s\n", regname(addr));
> > -    return eepro100_read1(s, addr);
> > -}
> > -
> > -static uint32_t pci_mmio_readw(void *opaque, target_phys_addr_t addr)
> > -{
> > -    EEPRO100State *s = opaque;
> > -    //~ logout("addr=%s\n", regname(addr));
> > -    return eepro100_read2(s, addr);
> > -}
> > -
> > -static uint32_t pci_mmio_readl(void *opaque, target_phys_addr_t addr)
> > -{
> > -    EEPRO100State *s = opaque;
> > -    //~ logout("addr=%s\n", regname(addr));
> > -    return eepro100_read4(s, addr);
> > +    if (size == 1) {
> > +        eepro100_write1(s, addr, value);
> > +    } else if (size == 2) {
> > +        eepro100_write2(s, addr, value);
> > +    } else {
> > +        eepro100_write4(s, addr, value);
> > +    }
> >  }
> >  
> > -static CPUWriteMemoryFunc * const pci_mmio_write[] = {
> > -    pci_mmio_writeb,
> > -    pci_mmio_writew,
> > -    pci_mmio_writel
> > -};
> > -
> > -static CPUReadMemoryFunc * const pci_mmio_read[] = {
> > -    pci_mmio_readb,
> > -    pci_mmio_readw,
> > -    pci_mmio_readl
> > -};
> > -
> > -static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
> > -                         pcibus_t addr, pcibus_t size, int type)
> > +static uint32_t pci_io_read(PCIDevice *dev, pcibus_t addr, int size)
> >  {
> >   
> 
> Don't change pci_dev -> dev. See above.
> 
> ...
>