Hi Tom,
On Wed, Mar 20, 2013 at 1:30 PM, Tom Rini trini@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@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@chromium.org Acked-by: Joe Hershberger joe.hershberger@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