[PATCH 3/5] cmd: env: check real location for env info command

[Wolfgang Denk]

Dear Patrick Delaunay,

In message <20200124133332.3.I42c79507524e5ad68e85fd60bbd686c4c59523ae at changeid> you wrote:
> Check the current ENV location, dynamically provided by the weak
> function env_get_location to be sure that the environment can be
> persistent.
>
> The compilation flag ENV_IS_IN_DEVICE is not enough when the board
> dynamically select the available storage location (according boot
> device for example).
>
> This patch solves issue for stm32mp1 platform, when the boot device
> is USB.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> ---
>
>  cmd/nvedit.c           | 13 ++++++++++---
>  env/env.c              | 18 ------------------
>  include/env_internal.h | 20 ++++++++++++++++++++
>  3 files changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 3d1054e763..a37b7c094a 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -1269,9 +1269,16 @@ static int do_env_info(cmd_tbl_t *cmdtp, int flag,
>  	/* evaluate whether environment can be persisted */
>  	if (eval_flags & ENV_INFO_IS_PERSISTED) {
>  #if defined(CONFIG_CMD_SAVEENV) && defined(ENV_IS_IN_DEVICE)
> -		if (!quiet)
> -			printf("Environment can be persisted\n");
> -		eval_results |= ENV_INFO_IS_PERSISTED;
> +		enum env_location loc = env_get_location(ENVOP_SAVE,

Please do not declare variables right in the middle of the code!


> +++ b/env/env.c
> @@ -105,24 +105,6 @@ static void env_set_inited(enum env_location location)
>  	gd->env_has_init |= BIT(location);
>  }
>  
> -/**
> - * env_get_location() - Returns the best env location for a board
> - * @op: operations performed on the environment
> - * @prio: priority between the multiple environments, 0 being the
> - *        highest priority
> - *
> - * This will return the preferred environment for the given priority.
> - * This is overridable by boards if they need to.
> - *
> - * All implementations are free to use the operation, the priority and
> - * any other data relevant to their choice, but must take into account
> - * the fact that the lowest prority (0) is the most important location
> - * in the system. The following locations should be returned by order
> - * of descending priorities, from the highest to the lowest priority.
> - *
> - * Returns:
> - * an enum env_location value on success, a negative error code otherwise
> - */
>  __weak enum env_location env_get_location(enum env_operation op, int prio)

I think it is a really bad idea to remove the comment from the
implementation.  Please keep it here.

> --- a/include/env_internal.h
> +++ b/include/env_internal.h
> @@ -209,6 +209,26 @@ struct env_driver {
>  
>  extern struct hsearch_data env_htab;
>  
> +/**
> + * env_get_location() - Returns the best env location for a board
> + * @op: operations performed on the environment
> + * @prio: priority between the multiple environments, 0 being the
> + *        highest priority
> + *
> + * This will return the preferred environment for the given priority.
> + * This is overridable by boards if they need to.
> + *
> + * All implementations are free to use the operation, the priority and
> + * any other data relevant to their choice, but must take into account
> + * the fact that the lowest prority (0) is the most important location
> + * in the system. The following locations should be returned by order
> + * of descending priorities, from the highest to the lowest priority.
> + *
> + * Returns:
> + * an enum env_location value on success, a negative error code otherwise
> + */
> +enum env_location env_get_location(enum env_operation op, int prio);

If absolutely necessary, copuy only what is needed for API
documentation.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"This is a test of the Emergency Broadcast System. If this had been an
actual emergency, do you really think we'd stick around to tell you?"