Hi, On 15 August 2014 12:25, Bryan Wu <coolo...@gmail.com> wrote: > On Fri, Aug 15, 2014 at 11:04 AM, Stephen Warren <swar...@wwwdotorg.org> > wrote: >> On 08/15/2014 11:49 AM, Bryan Wu wrote: >>> >>> Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for >> >> >> Can we spell out the commit description too, so it's easier to know what it >> refers too, and is useful if someone cherry-picks the commit so the commit >> ID changes: >> >> Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced... >> >> >>> booting FIT image. It's because calling fit_parse_config() twice will >>> give us wrong value in img_addr. >>> >>> Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and >>> return fit_uname_config and fit_uname_kernel for CONFIG_FIT. >> >> >>> diff --git a/common/image.c b/common/image.c >> >> >>> -ulong genimg_get_kernel_addr(char * const img_addr) >>> -{ >>> #if defined(CONFIG_FIT) >>> - const char *fit_uname_config = NULL; >>> - const char *fit_uname_kernel = NULL; >>> +ulong genimg_get_kernel_addr(char * const img_addr, >>> + const char **fit_uname_config, >>> + const char **fit_uname_kernel) >>> +#else
Let's avoid these #ifdefs inside a function signature, just too ugly. If the extra arguments are a big problem you could create a new function perhaps? Also, how about updating test/image/fit.py to test this behaviour? Then we can catch the bug. >>> +ulong genimg_get_kernel_addr(char * const img_addr) >>> #endif >> >> >> Indentation looks wrong on that #endif. >> >> Wouldn't it be better to avoid repeating the common parts, so this instead: >> >> ulong genimg_get_kernel_addr(char * const img_addr, >> #if defined(CONFIG_FIT) >> const char **fit_uname_config, >> const char **fit_uname_kernel) >> #endif >> ) >> { >> > > I got compiling error like this when I try this method then I moved to > 2 current one: > > --- > ulong genimg_get_kernel_addr(char * const img_addr, > #if defined(CONFIG_FIT) > const char **fit_uname_config, > const char **fit_uname_kernel > #endif > ); > --- > include/image.h:432:1: error: expected declaration specifiers or ‘...’ > before ‘)’ token > ); > ^ > > But if I use this: > > ---- > ulong genimg_get_kernel_addr(char * const img_addr > #if defined(CONFIG_FIT) > , const char **fit_uname_config, > const char **fit_uname_kernel > #endif > ); > ---- > Then compiler is happy, but the style looks weird to me. > > This is for declaration but definition has the same problem. > > -Bryan > > >>> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h >> >> >>> +#define CONFIG_FIT >> >> >> I think that crept in by mistake. >> >> >>> diff --git a/include/image.h b/include/image.h >>> index ca2fe86..a47c146 100644 >> >> >>> +#if defined(CONFIG_FIT) >>> +ulong genimg_get_kernel_addr(char * const img_addr, >>> + const char **fit_uname_config, >>> + const char **fit_uname_kernel); >>> +#else >>> ulong genimg_get_kernel_addr(char * const img_addr); >>> +#endif >> >> >> Same comment about #ifdef placement here. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot