[U-Boot] [PATCH 2/3] net: add getenv/setenv enetaddr function to use ethernet device num

[Wolfgang Denk]

Dear Jean-Christophe PLAGNIOL-VILLARD,

In message <1250023747-20224-2-git-send-email-plagnioj at jcrosoft.com> you wrote:
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj at jcrosoft.com>
> ---
>  include/net.h |    2 ++
>  net/eth.c     |   28 +++++++++++++++++++++++++---
>  2 files changed, 27 insertions(+), 3 deletions(-)

NAK.

First, there are formal issues:

- The subject line is way too long.

- There is no commit message and no description what this patch is
  supposed to do or to fix. Why should we add it?

> index 2a8a12d..dc4ae41 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -124,6 +124,8 @@ extern void eth_set_enetaddr(int num, char* a);	/* Set new MAC address */
>  extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
>  extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
>  extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
> +extern int eth_getenv_num_enetaddr(int num, uchar *enetaddr);
> +extern int eth_setenv_num_enetaddr(int num, const uchar *enetaddr);

What are these functions god for? Are they by any chance duplicationg
existing code, got example eth_getenv_enetaddr_by_index() ?

>  extern int eth_init(bd_t *bis);			/* Initialize the device */
>  extern int eth_send(volatile void *packet, int length);	   /* Send a packet */
> diff --git a/net/eth.c b/net/eth.c
> index 2316a22..9bdf961 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -27,6 +27,12 @@
>  #include <miidev.h>
>  
>  #ifdef CONFIG_CMD_NET
> +
> +static void get_enetvar(int num, char *enetvar)

Should this not be "uchar *", to stay consistent with the other code?

> +{
> +	sprintf(enetvar, num ? "eth%daddr" : "ethaddr", num);
> +}
> +
>  void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
>  {
>  	char *end;
> @@ -53,6 +59,24 @@ int eth_setenv_enetaddr(char *name, const uchar *enetaddr)
>  
>  	return setenv(name, buf);
>  }
> +
> +int eth_getenv_num_enetaddr(int num, uchar *enetaddr)
> +{
> +	char enetvar[32];
> +
> +	get_enetvar(num, enetvar);
> +
> +	return eth_getenv_enetaddr(enetvar, enetaddr);
> +}

Looks very much like eth_getenv_enetaddr_by_index() to me. What do you
need this for?


Please elucidate...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The algorithm to do that is extremely nasty. You might want  to  mug
someone with it."                   - M. Devine, Computer Science 340