[U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf

[Simon Glass]

Hi Tom,

On Wed, Mar 20, 2013 at 12:40 PM, Tom Rini <trini at ti.com> wrote:
> On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
>> This function should be declared in net.h. At the same time, let's use
>> autoconf instead of #ifdef for its inclusion.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> Acked-by: Joe Hershberger <joe.hershberger at ni.com>
> [snip]
>> @@ -266,12 +254,16 @@ int update_tftp(ulong addr)
>>       /* get load address of downloaded update file */
>>       if ((env_addr = getenv("loadaddr")) != NULL)
>>               addr = simple_strtoul(env_addr, NULL, 16);
>> +     else if (autoconf_has_update_load_addr())
>> +             addr = autoconf_update_load_addr();
>>       else
>> -             addr = CONFIG_UPDATE_LOAD_ADDR;
>> +             addr = 0x100000;
>>
>> +     msec_max = autoconf_has_update_tftp_msec_max() ?
>> +                     autoconf_update_tftp_msec_max() : 100;
>>
>> -     if (update_load(filename, CONFIG_UPDATE_TFTP_MSEC_MAX,
>> -                                     CONFIG_UPDATE_TFTP_CNT_MAX, addr)) {
>> +     if (update_load(filename, msec_max, autoconf_update_tftp_cnt_max(),
>> +             addr)) {
>
> This doesn't read nearly as clean to me as the old code.  Part of the
> problem is that we really need a way to foce an CONFIG option to be set
> to something and give a default (so, the Kconfig switch-over).  Now, in
> cases like this it the compiler smart enough to say "oh, msec_max is a
> constant, lets not waste the space on a variable" ?  Would const'ing
> that help or confuse things would be a follow up question.

Yes I believe the compiler does the right thing here. Even if there is
a 'variable' it would only be in a register.

const can be used, but I don't think it would do anything in this case.

In reading the old code, you need to look at the top of the file,
where some code was removed, and take that into account.

-/* set configuration defaults if needed */
-#ifndef CONFIG_UPDATE_LOAD_ADDR
-#define CONFIG_UPDATE_LOAD_ADDR        0x100000
-#endif
-
-#ifndef CONFIG_UPDATE_TFTP_MSEC_MAX
-#define CONFIG_UPDATE_TFTP_MSEC_MAX    100
-#endif
-
-#ifndef CONFIG_UPDATE_TFTP_CNT_MAX
-#define CONFIG_UPDATE_TFTP_CNT_MAX     0
-#endif
-

Regards,
Simon