[U-Boot] [PATCH v5 4/5] net: add eth_setenv_enetaddr_by_index()

[Joe Hershberger]

Hi Michael,

On Fri, May 11, 2012 at 5:50 PM, Michael Walle <michael at walle.cc> wrote:
> Signed-off-by: Michael Walle <michael at walle.cc>
> Cc: Joe Hershberger <joe.hershberger at gmail.com>
> ---
>  include/net.h |   16 ++++++++++++++++
>  net/eth.c     |   15 +++++++++++++++
>  2 files changed, 31 insertions(+), 0 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index f6aeba2..bdc3da6 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -104,7 +104,9 @@ extern struct eth_device *eth_get_dev_by_index(int index); /* get dev @ index */
>  extern int eth_get_dev_index (void);           /* get the device index */
>  extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
>  extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
> +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
>  extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
> +#endif

Which other function did you intend?  You already guarded the only
definition whose implementation is guarded.

>  /*
>  * Get the hardware address for an ethernet interface .
> @@ -118,6 +120,20 @@ extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
>  extern int eth_getenv_enetaddr_by_index(const char *base_name, int index,
>                                        uchar *enetaddr);
>
> +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX

Personally I don't like config options for something so tiny.  If you
really must guard against including it, use the CONFIG_RANDOM_MACADDR
instead.  I'd prefer to just let the linker exclude it when it's never
referenced.

> +/*
> + * Set the hardware address for an ethernet interface .
> + * Args:
> + *     base_name - base name for device (normally "eth")
> + *     index - device index number (0 for first)
> + *     enetaddr - returns 6 byte hardware address
> + * Returns:
> + *     0 on success, else 1.
> + */
> +extern int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> +                                       const uchar *enetaddr);
> +#endif
> +
>  #ifdef CONFIG_RANDOM_MACADDR
>  /*
>  * The u-boot policy does not allow hardcoded ethernet addresses. Under the
> diff --git a/net/eth.c b/net/eth.c
> index afce863..d66e22a 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -67,6 +67,21 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index,
>        return eth_getenv_enetaddr(enetvar, enetaddr);
>  }
>
> +#ifdef CONFIG_SETENV_ENETADDR_BY_INDEX
> +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> +               const uchar *enetaddr)
> +{
> +       char enetvar[32];
> +
> +       if (index)
> +               sprintf(enetvar, "%s%daddr", base_name, index);
> +       else
> +               sprintf(enetvar, "%saddr", base_name);
> +
> +       return eth_setenv_enetaddr(enetvar, enetaddr);
> +}
> +#endif

You should synchronize with Rob Herring who is also trying to add this
eth_setenv_enetaddr_by_index() function.
http://patchwork.ozlabs.org/patch/152590/

Also since Rob's patch would use this function all the time, the
CONFIG_SETENV_ENETADDR_BY_INDEX would be removed, right?

Thanks,
-Joe