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

Dear Mike Frysinger,

In message 1233187416-22378-2-git-send-email-vapier@gentoo.org you wrote:

Since the on-chip MAC does not have an eeprom or similar interface, force all Blackfin boards that use this to define their own board_get_enetaddr() function.

This patch makes littel sense to me.

• const char *ethaddr; struct eth_device *dev; dev = (struct eth_device *)malloc(sizeof(*dev)); if (dev == NULL)

@@ -89,6 +90,27 @@ int bfin_EMAC_initialize(bd_t *bis)

eth_register(dev);

• ethaddr = getenv("ethaddr");

+#ifndef CONFIG_ETHADDR

• if (ethaddr == NULL) {

• char nid[20];
  

• board_get_enetaddr(bd->bi_enetaddr);
  

• sprintf(nid, "%02X:%02X:%02X:%02X:%02X:%02X",
  

• 	bd->bi_enetaddr[0], bd->bi_enetaddr[1],
  

• 	bd->bi_enetaddr[2], bd->bi_enetaddr[3],
  

• 	bd->bi_enetaddr[4], bd->bi_enetaddr[5]);
  

• setenv("ethaddr", nid);
  

• } else

+#endif

Why don't you simply do the setenv("ethaddr",...) in some board init code, like other boards do?

• {

• int i;
  

• char *e;
  

• for (i = 0; i < 6; ++i) {
  

• 	bd->bi_enetaddr[i] = simple_strtoul(ethaddr, &e, 16);
  

• 	ethaddr = (*e) ? e + 1 : e;
  

• }
  

• }

Please no nested blocks without real need.

diff --git a/include/common.h b/include/common.h index afee188..d4c361a 100644 --- a/include/common.h +++ b/include/common.h @@ -354,7 +354,7 @@ void board_ether_init (void); #if defined(CONFIG_RPXCLASSIC) || defined(CONFIG_MBX) || \ defined(CONFIG_IAD210) || defined(CONFIG_XPEDITE1K) || \ defined(CONFIG_METROBOX) || defined(CONFIG_KAREF) || \

• defined(CONFIG_V38B)

• defined(CONFIG_V38B) || defined(CONFIG_BFIN_MAC)

Please keep lists sorted.

--- a/lib_blackfin/board.c +++ b/lib_blackfin/board.c @@ -378,35 +378,6 @@ void board_init_r(gd_t * id, ulong dest_addr) /* relocate environment function pointers etc. */ env_relocate();

-#ifdef CONFIG_CMD_NET

• /* board MAC address */
• s = getenv("ethaddr");
• if (s == NULL) {

-# ifndef CONFIG_ETHADDR -# if 0

• if (!board_get_enetaddr(bd->bi_enetaddr)) {
  

• 	char nid[20];
  

• 	sprintf(nid, "%02X:%02X:%02X:%02X:%02X:%02X",
  

• 		bd->bi_enetaddr[0], bd->bi_enetaddr[1],
  

• 		bd->bi_enetaddr[2], bd->bi_enetaddr[3],
  

• 		bd->bi_enetaddr[4], bd->bi_enetaddr[5]);
  

• 	setenv("ethaddr", nid);
  

• }
  

-# endif -# endif

• } else {

• int i;
  

• char *e;
  

• for (i = 0; i < 6; ++i) {
  

• 	bd->bi_enetaddr[i] = simple_strtoul(s, &e, 16);
  

• 	s = (*e) ? e + 1 : e;
  

• }
  

• }
• /* IP Address */
• bd->bi_ip_addr = getenv_IPaddr("ipaddr");

-#endif

To me this seems to be a better place for the code (which needs fixing, of course).

Best regards,

Wolfgang Denk