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

[Anthony Liguori]

On 02/10/2010 12:32 AM, 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.
>    

A comment is fine, but the TARGET_I386 makes this file unnecessarily 
dependent on TARGET.  With this change, we only need to build eepro100.o 
once.
> >   /***********************************************************/
> >   /* 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 very common and I changed it for consistency.  I honestly don't 
care though either way.

Regards,

Anthony Liguori