[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 1:30 PM, Tom Rini <trini at ti.com> wrote:
> On Wed, Mar 20, 2013 at 12:57:37PM -0700, Simon Glass wrote:
>> 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;
>
> OK, this in particular would looke a little better as:
> addr = autoconf_has_update_load_addr() ?
>              autoconf_update_load_addr() : 0x100000;
> if ((env_addr = ...)

Yes, agreed.

>
> Set the addr to the default, then override.
>
>> >>
>> >> +     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.
>
> OK.
>
>> 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
>
> That's what I mean.  We have defaults that can be overriden and we use
> the value simply.  This was a decent way of saying "we know most people
> will use X as the value, but allow for override".  Now we have to do
> if (autoconf_has_foo())
>   x = autoconf_foo()
> else
>   x = default;
>
> Or:
> x = autoconf_has_foo() ? autoconf_foo() : default;
> Which usually spills out into 2 lines and isn't my favorite construct
> ever.  Lets see where I'm at after going over the whole series.

OK, in fact we need to set a way of doing this, and also whether in
some cases we are better off with #ifdefs. My main dislikes of #ifdef
were mentioned in the cover letter, but they do have their place.

Regards,
Simon