[U-Boot-Users] [PATCH v2 4/7] add SMSC LAN9x1x Network driver

[Ben Warren <biggerbadderben@gmail.com>]

Hi Peter,

Peter Pearse wrote:
>> -----Original Message-----
>> From: Ben Warren [mailto:biggerbadderben at gmail.com] 
>> Sent: 26 March 2008 20:08
>> To: Guennadi Liakhovetski
>> Cc: u-boot-users at lists.sourceforge.net; Wolfgang Denk; Peter Pearse
>> Subject: Re: [U-Boot-Users] [PATCH v2 4/7] add SMSC LAN9x1x 
>> Network driver
>>
>> Hi Guennadi,
>>
>> Guennadi Liakhovetski wrote:
>>     
>>> From: Sascha Hauer <s.hauer@pengutronix.de>
>>>
>>> This patch adds a driver for the following smsc network controllers:
>>> LAN9115
>>> LAN9116
>>> LAN9117
>>> LAN9215
>>> LAN9216
>>> LAN9217
>>>
>>>   
>>>       
>> How many of these have been tested, and on what platforms.  
>> I'm asking because the code seems to assume a 32-bit 
>> interface and these aren't all 32-bit chips.
>>     
>
> Comments please Sascha.
>
> ---snip---
>
>   
>>> diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c new file 
>>> mode 100644 index 0000000..5830368
>>> --- /dev/null
>>> +++ b/drivers/net/smc911x.c
>>>       
>
> ---snip---
>
>   
>>> +
>>> +#ifdef CONFIG_DRIVER_SMC911X
>>> +
>>>   
>>>       
>> This should be moved to the Makefile. 
>>     
>
> Agreed
>
>
> ---snip---
>
>   
>>>   
>>>       
>> Register and bitfield definitions should be in a header file.
>>     
>
> Not these file specific ones.
> Ben - where else would they be applicable? 
>
>   
Well, I can't come up with a better answer than 'precedent', so I guess 
it's OK to keep the #defines in the C code.
>>  More generally, only register addresses and bitfields should 
>> be defined.  
>>     
>
> Using macros to encapsulate both address and 
>   
>> function is bad form, IMHO.
>>     
>
> Agreed
>
>   
>> I haven't even gotten into the functionality, because I think 
>> there's a lot of work to be done just in coding style 
>>     
>
> Ben - perhaps you could help by pointing out some more examples 
>
>   
By coding style I mean the nasty macros, not stuff like brackets and 
whitespace.  If the code was more readable and addressed the bus width 
differences between chips, this would probably go in quickly.  I'll have 
another look to see if I can be more helpful rather than curmudgeonly.

regards,
Ben