[U-Boot] [PATCH v5 08/12] SPEAr : Support for HW mac id read/write from i2c mem

[Vipin KUMAR]

On 1/18/2010 11:45 AM, Ben Warren wrote:
>> int misc_init_r(void)
>>  {
>> +#if defined(CONFIG_CMD_NET)
>> +    uchar mac_id[6];
>> +
>> +    if (!eth_getenv_enetaddr("ethaddr", mac_id) &&
>> !i2c_read_mac(mac_id))
>> +        eth_setenv_enetaddr("ethaddr", mac_id);
>>   
> It's really not a good idea to programatically set an environment
> variable like this.  Also, there's already a well-defined and documented
> protocol for  how MAC addresses should be read and the precedence
> between EEPROM storage and environment storage.  Please find a way to
> fit into that model.  Since you don't have network support yet, I have
> no idea which network controller you're using, and recommend waiting
> until this is really needed.

I am referring to doc/README.enetaddr in the uboot source. This document says
"If the hardware design mandates that the MAC address is stored in some special
place (like EEPROM etc...), then the board specific init code (such as the
board-specific misc_init_r() function) is responsible for locating the MAC
address(es) and initializing the respective environment variable(s) from it.
Note that this shall be done if, and only if, the environment does not already
contain these environment variables, i.e. existing variable definitions must
not be overwritten."

Coming back to the code
   if (!eth_getenv_enetaddr("ethaddr", mac_id) && !i2c_read_mac(mac_id))
        eth_setenv_enetaddr("ethaddr", mac_id);

first I read the env variable "ethaddr" and skip setting it if it already
exists
then I read mac id from i2c memory and store the same in env variable

Please let me know if I am missing anything.

Although the patch is not really needed right now, it will be needed soon.
I am planning to send the net support patch during the next merging window.
Since the current patch does not have any side effects, it can be included 
as of now.

>>  
>>          if (chip->cpufreq == -1)
>> @@ -156,6 +215,13 @@ int do_chip_config(cmd_tbl_t *cmdtp, int flag,
>> int argc, char *argv[])
>>          else
>>              printf("DDR Type    = Not Known\n");
>>  
>> +        if (!i2c_read_mac(mac)) {
>> +            sprintf(i2c_mac, "%pM", mac);
>> +            printf("Ethaddr (from i2c mem) = %s\n", i2c_mac);
>>   
> Maybe I'm missing some context, but why's the sprintf needed?

Yes, sprintf is not needed here
Thanks for pointing out

Regards
Vipin