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

[Anthony Liguori <anthony@codemonkey.ws>]

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<aliguori@us.ibm.com>
>> ---
>>   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