[U-Boot] [PATCH 01/27 v2] Blackfin: bfin_mac: force board_get_enetaddr() usage

[Mike Frysinger]

On Thursday 12 February 2009 02:33:46 Wolfgang Denk wrote:
> net: new utility functions for working with enetaddr's:
>
> 	Please ditch str_enetaddr() and rename str_enetaddr_r() into
> 	str_enetaddr(); using a static buffer for the return value is
> 	nothing I'd like to see in the code. It is trivial for the
> 	caller to allocate a buffer on the stack.

i'm lazy which is why i added that.  but easy enough to convert things over.

> 	eth_parse_enetaddr() should be "int"" and allow for error
> 	checking (to catch at least simple format errors).

there is no error checking in there now, just straight string -> number 
conversion.  what kind of error checking would you like added ?  i could tail 
off with a call to is_valid_ether_addr() which would reject addresses that are 
all 00's (technically valid, but the likely hood of anyone having that for 
real is nil) and any that have the multicast bit set (which i dont think will 
be a problem for any boards).

otherwise, i imagine we could add stuff to the simple_stroul() ?  i'd be a 
little iffy here though as no place that i found in u-boot ever checked this 
and so far, no one has complained about it.

> *: get mac address from environment
>
> 	I think these N patches should be squashed into one, ot at
> 	least a much smaller number of patches.
>
> *: do not initialize bi_enet*addr in global data
>
> 	Ditto.

i did it by boards/cpu dirs so maintainers could review/ack/merge easier.  but 
squashing is easy enough, and if you do the merge that'd address the collation 
issue.

> kup4k/kup4x: rename load_sernum_ethaddr() to kup_load_sernum_ethaddr()
> tqm8xx: rename load_sernum_ethaddr() to tqc_load_sernum_ethaddr()
>
> 	NAK for two reasons:
>
> 	- I see no reason for such a change, and especially not for
> 	  adding an empty function load_sernum_ethaddr(). [And you do
> 	  not give any explanation why you're making such a change
> 	  either.]
> 	- You add the calls to the new kup_load_sernum_ethaddr() /
> 	  tqc_load_sernum_ethaddr() functions right in the middle of a
> 	  list of declarations. That's not acceptable.

meh, i didnt really notice with the #ifdef mess

> ppc: do not initialize bi_enet*addr in global data:
>
> 	...
> 	Also stop calling load_sernum_ethaddr() since all boards now
> 	implement this as a stub.
>
> 	Makes no sense to me. I don't even see what such a stub would
> 	be needed for or where it was called. Just looks like a waste
> 	of memory footprint to me. NAK.
>
> drop now unused load_sernum_ethaddr() function:
>
> 	Ah. You propbably want to change the order of your changes so
> 	you can avoid these intermediate steps. But still please do
> 	not rename load_sernum_ethaddr() - there is no need for more
> 	complex names.

the point was so that things could be merged easily w/out requiring strict 
order, and so that bisection would not break.  otherwise i have to combine the 
ppc change with the board changes.  or break bisection.  doesnt matter to me 
as these arent my boards ;).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20090212/6595cd58/attachment.pgp