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

[Bin Meng]

Hi Joe,


On Sat, Jan 23, 2016 at 6:32 AM, Joe Hershberger
 wrote:
> Hi Bin,
>
> On Sun, Jan 17, 2016 at 10:41 PM, Bin Meng  wrote:
>> Hi Simon,
>>
>> On Mon, Jan 18, 2016 at 5:51 AM, Simon Glass  wrote:
>>> Move the functions which set ethernet environment variables to the common
>>> file.
>>>
>>> Signed-off-by: Simon Glass 
>>> ---
>>>
>>>  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_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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Joe Hershberger]

Hi Bin,

On Sun, Jan 17, 2016 at 10:41 PM, Bin Meng  wrote:
> Hi Simon,
>
> On Mon, Jan 18, 2016 at 5:51 AM, Simon Glass  wrote:
>> Move the functions which set ethernet environment variables to the common
>> file.
>>
>> Signed-off-by: Simon Glass 
>> ---
>>
>>  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_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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Joe Hershberger]

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

Acked-by: Joe Hershberger 
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

[Bin Meng]

Hi Simon,

On Mon, Jan 18, 2016 at 5:51 AM, Simon Glass  wrote:
> Move the functions which set ethernet environment variables to the common
> file.
>
> Signed-off-by: Simon Glass 
> ---
>
>  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, , 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 
>  #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, , 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot