From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Fri, 15 Aug 2014 12:04:13 -0600
Subject: [U-Boot] [PATCH] image: fix bootm failure for FIT image
In-Reply-To: <1408124953-4708-1-git-send-email-pengw@nvidia.com>
References: <1408124953-4708-1-git-send-email-pengw@nvidia.com>
Message-ID: <53EE4B9D.7020400@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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
> +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
)
{

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