[U-Boot,4/8] net: Move environment functions to the common file

Move the functions which set ethernet environment variables to the common
file.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 net/eth.c          | 43 -------------------------------------------
 net/eth_common.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
 net/eth_internal.h | 16 ++++++++++++++++
 3 files changed, 59 insertions(+), 43 deletions(-)

Hi Simon,

On Mon, Jan 18, 2016 at 5:51 AM, Simon Glass <sjg@chromium.org> wrote:
> Move the functions which set ethernet environment variables to the common
> file.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  net/eth.c          | 43 -------------------------------------------
>  net/eth_common.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
>  net/eth_internal.h | 16 ++++++++++++++++
>  3 files changed, 59 insertions(+), 43 deletions(-)
>
> diff --git a/net/eth.c b/net/eth.c
> index 602925d..af8fcae 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -20,49 +20,6 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> -void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
> -{
> -       char *end;
> -       int i;
> -
> -       for (i = 0; i < 6; ++i) {
> -               enetaddr[i] = addr ? simple_strtoul(addr, &end, 16) : 0;
> -               if (addr)
> -                       addr = (*end) ? end + 1 : end;
> -       }
> -}
> -
> -int eth_getenv_enetaddr(const char *name, uchar *enetaddr)
> -{
> -       eth_parse_enetaddr(getenv(name), enetaddr);
> -       return is_valid_ethaddr(enetaddr);
> -}
> -
> -int eth_setenv_enetaddr(const char *name, const uchar *enetaddr)
> -{
> -       char buf[20];
> -
> -       sprintf(buf, "%pM", enetaddr);
> -
> -       return setenv(name, buf);
> -}
> -
> -int eth_getenv_enetaddr_by_index(const char *base_name, int index,
> -                                uchar *enetaddr)
> -{
> -       char enetvar[32];
> -       sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> -       return eth_getenv_enetaddr(enetvar, enetaddr);
> -}
> -
> -static inline int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> -                                uchar *enetaddr)
> -{
> -       char enetvar[32];
> -       sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> -       return eth_setenv_enetaddr(enetvar, enetaddr);
> -}
> -
>  static int eth_mac_skip(int index)
>  {
>         char enetvar[15];
> diff --git a/net/eth_common.c b/net/eth_common.c
> index ee0b6df..3fa6d83 100644
> --- a/net/eth_common.c
> +++ b/net/eth_common.c
> @@ -10,6 +10,49 @@
>  #include <miiphy.h>
>  #include "eth_internal.h"
>
> +void eth_parse_enetaddr(const char *addr, uchar *enetaddr)
> +{
> +       char *end;
> +       int i;
> +
> +       for (i = 0; i < 6; ++i) {
> +               enetaddr[i] = addr ? simple_strtoul(addr, &end, 16) : 0;
> +               if (addr)
> +                       addr = (*end) ? end + 1 : end;
> +       }
> +}
> +
> +int eth_getenv_enetaddr(const char *name, uchar *enetaddr)
> +{
> +       eth_parse_enetaddr(getenv(name), enetaddr);
> +       return is_valid_ethaddr(enetaddr);
> +}
> +
> +int eth_setenv_enetaddr(const char *name, const uchar *enetaddr)
> +{
> +       char buf[20];
> +
> +       sprintf(buf, "%pM", enetaddr);
> +
> +       return setenv(name, buf);
> +}
> +
> +int eth_getenv_enetaddr_by_index(const char *base_name, int index,
> +                                uchar *enetaddr)
> +{
> +       char enetvar[32];
> +       sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> +       return eth_getenv_enetaddr(enetvar, enetaddr);
> +}
> +
> +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> +                                uchar *enetaddr)
> +{
> +       char enetvar[32];
> +       sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> +       return eth_setenv_enetaddr(enetvar, enetaddr);
> +}
> +
>  void eth_common_init(void)
>  {
>         bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);
> diff --git a/net/eth_internal.h b/net/eth_internal.h
> index e65d898..38d8420 100644
> --- a/net/eth_internal.h
> +++ b/net/eth_internal.h
> @@ -12,4 +12,20 @@
>  /* Do init that is common to driver model and legacy networking */
>  void eth_common_init(void);
>
> +/**
> + * eth_setenv_enetaddr_by_index() - set the MAC address envrionment variable
> + *
> + * This sets up an environment variable with the given MAC address (@enetaddr).
> + * The environment variable to be set is defined by <@base_name><@index>addr.
> + * If @index is 0 it is omitted. For common Ethernet this means ethaddr,
> + * eth1addr, etc.
> + *
> + * @base_name: Base name for variable, typically "eth"
> + * @index:     Index of interface being updated (>=0)
> + * @enetaddr:  Pointer to MAC address to put into the variable
> + * @return 0 if OK, other value on error
> + */
> +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> +                                uchar *enetaddr);
> +
>  #endif
> --

Could you add some comments about the other routines
(eth_parse_enetaddr, eth_getenv_enetaddr, eth_setenv_enetaddr,
eth_getenv_enetaddr_by_index)?

Regards,
Bin

On Sun, Jan 17, 2016 at 3:51 PM, Simon Glass <sjg@chromium.org> wrote:
> Move the functions which set ethernet environment variables to the common
> file.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

Hi Bin,

On Sun, Jan 17, 2016 at 10:41 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Mon, Jan 18, 2016 at 5:51 AM, Simon Glass <sjg@chromium.org> wrote:
>> Move the functions which set ethernet environment variables to the common
>> file.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  net/eth.c          | 43 -------------------------------------------
>>  net/eth_common.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  net/eth_internal.h | 16 ++++++++++++++++
>>  3 files changed, 59 insertions(+), 43 deletions(-)
>>

<snip>

>> diff --git a/net/eth_internal.h b/net/eth_internal.h
>> index e65d898..38d8420 100644
>> --- a/net/eth_internal.h
>> +++ b/net/eth_internal.h
>> @@ -12,4 +12,20 @@
>>  /* Do init that is common to driver model and legacy networking */
>>  void eth_common_init(void);
>>
>> +/**
>> + * eth_setenv_enetaddr_by_index() - set the MAC address envrionment variable
>> + *
>> + * This sets up an environment variable with the given MAC address (@enetaddr).
>> + * The environment variable to be set is defined by <@base_name><@index>addr.
>> + * If @index is 0 it is omitted. For common Ethernet this means ethaddr,
>> + * eth1addr, etc.
>> + *
>> + * @base_name: Base name for variable, typically "eth"
>> + * @index:     Index of interface being updated (>=0)
>> + * @enetaddr:  Pointer to MAC address to put into the variable
>> + * @return 0 if OK, other value on error
>> + */
>> +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
>> +                                uchar *enetaddr);
>> +
>>  #endif
>> --
>
> Could you add some comments about the other routines
> (eth_parse_enetaddr, eth_getenv_enetaddr, eth_setenv_enetaddr,
> eth_getenv_enetaddr_by_index)?

That would be great, but those are already defined in include/net.h
with poor / incomplete comments. I don't think fixing that *needs* to
be part of this patch, but it would be welcomed.

>
> Regards,
> Bin
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Hi Joe,


On Sat, Jan 23, 2016 at 6:32 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> On Sun, Jan 17, 2016 at 10:41 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Simon,
>>
>> On Mon, Jan 18, 2016 at 5:51 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Move the functions which set ethernet environment variables to the common
>>> file.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>>  net/eth.c          | 43 -------------------------------------------
>>>  net/eth_common.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
>>>  net/eth_internal.h | 16 ++++++++++++++++
>>>  3 files changed, 59 insertions(+), 43 deletions(-)
>>>
>
> <snip>
>
>>> diff --git a/net/eth_internal.h b/net/eth_internal.h
>>> index e65d898..38d8420 100644
>>> --- a/net/eth_internal.h
>>> +++ b/net/eth_internal.h
>>> @@ -12,4 +12,20 @@
>>>  /* Do init that is common to driver model and legacy networking */
>>>  void eth_common_init(void);
>>>
>>> +/**
>>> + * eth_setenv_enetaddr_by_index() - set the MAC address envrionment variable
>>> + *
>>> + * This sets up an environment variable with the given MAC address (@enetaddr).
>>> + * The environment variable to be set is defined by <@base_name><@index>addr.
>>> + * If @index is 0 it is omitted. For common Ethernet this means ethaddr,
>>> + * eth1addr, etc.
>>> + *
>>> + * @base_name: Base name for variable, typically "eth"
>>> + * @index:     Index of interface being updated (>=0)
>>> + * @enetaddr:  Pointer to MAC address to put into the variable
>>> + * @return 0 if OK, other value on error
>>> + */
>>> +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
>>> +                                uchar *enetaddr);
>>> +
>>>  #endif
>>> --
>>
>> Could you add some comments about the other routines
>> (eth_parse_enetaddr, eth_getenv_enetaddr, eth_setenv_enetaddr,
>> eth_getenv_enetaddr_by_index)?
>
> That would be great, but those are already defined in include/net.h
> with poor / incomplete comments. I don't think fixing that *needs* to
> be part of this patch, but it would be welcomed.
>

Simon added comments for eth_setenv_enetaddr_by_index() which did not
have any comments before, so I'd assume we should add comments for
others in the same patch. Otherwise it looks odd to me.

Regards,
Bin

Hi Simon,

https://patchwork.ozlabs.org/patch/569320/ was applied to u-boot-net.git.

Thanks!
-Joe