Dear Patrick Delaunay,
In message
<20200124133332.3.I42c79507524e5ad68e85fd60bbd686c4c59523ae@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.delau...@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: w...@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?"
