[U-Boot] [PATCH v1 1/1] mpc512x: adjust and improve AC14xx board support

[Stefano Babic]

Hi Gerhard,

On 03/06/2013 13:10, Gerhard Sittig wrote:

> -	s = getenv("install_in_progress");
> +	s = getenv("want_recovery");
>  	if ((s != NULL) && (*s != '\0')) {
> -		printf("previous installation aborted, running RECOVERY\n");
> +		printf("detected recovery request (environment)\n");
>  		want_recovery = 1;
>  	}
> -	s = getenv("install_failed");
> +	s = getenv("install_in_progress");
>  	if ((s != NULL) && (*s != '\0')) {
> -		printf("previous installation FAILED, running RECOVERY\n");
> +		printf("previous installation has not completed\n");
>  		want_recovery = 1;
>  	}
> -	s = getenv("want_recovery");
> +	s = getenv("install_failed");

I am asking myself if it is strictly required to have multiple variables
to identify if the "recovery" script must be run or not. If a previous
install was interrupted ("install_in_progress"
), or a request was initiated ("want recovery") or the last installation
failed, is not so important. In all cases you set the want_recovery flag
and starts the script. But using multiple variables  it is not atomic,
and could be, due for example to a system reset, that a variable is set
without clearing the other one.

Why is not better to set a single variable, maybe with multiple values ?
Value could be a simple integer (0=no recovery,
1=install_in_progress,,..) or still a string, if you prefer this solution.

> -	"rootpath=/opt/eldk/ppc_6xx\n"					\
> +	"rootpath=/opt/eldk/ppc_6xx\0"					\

We do not set IP addresses or fix rootpath. Someone could have installed
the rootfs on NFS on a different path. Also, IMHO rootpath should be
simply removed.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================