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

Wolfgang Denk
2009-08-12T18:58:00Z

Dear Jean-Christophe PLAGNIOL-VILLARD,

In message <1250023747-20224-2-git-send-email-plagnioj@jcrosoft.com> you wrote:
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@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