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

[Tom Rini]

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 = ...)

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130320/0ca0f483/attachment.pgp>