[U-Boot] [PATCH 12/14] env: Mark env_get_location as weak

[Maxime Ripard]

Hi,

On Tue, Jan 09, 2018 at 11:05:51AM -0500, Tom Rini wrote:
> On Tue, Jan 09, 2018 at 02:10:47PM +0100, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sun, Jan 07, 2018 at 09:52:23PM -0700, Simon Glass wrote:
> > > >> On 28 November 2017 at 03:24, Maxime Ripard
> > > >> <maxime.ripard at free-electrons.com> wrote:
> > > >> > Allow boards and architectures to override the default environment lookup
> > > >> > code by overriding env_get_location.
> > > >> >
> > > >> > Reviewed-by: Lukasz Majewski <lukma at denx.de>
> > > >> > Signed-off-by: Maxime Ripard <maxime.ripard at free-electrons.com>
> > > >> > ---
> > > >> >  env/env.c | 2 +-
> > > >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >> >
> > > >> > diff --git a/env/env.c b/env/env.c
> > > >> > index b4d8886e7a69..9b8b38cf3c2b 100644
> > > >> > --- a/env/env.c
> > > >> > +++ b/env/env.c
> > > >> > @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {
> > > >> >
> > > >> >  static enum env_location env_load_location;
> > > >> >
> > > >> > -static enum env_location env_get_location(enum env_operation op, int prio)
> > > >> > +__weak enum env_location env_get_location(enum env_operation op, int prio)
> > > >>
> > > >> Is it possible instead to make this deterministic, so that the board
> > > >> sets up the location during boot?
> > > >
> > > > I'm not really sure what you mean here. The default implementation is
> > > > deterministic, and the board implementations should make sure that it
> > > > is if it's of any value to them.
> > > >
> > > > Do you want me to add a comment to make that clearer?
> > > 
> > > I mean that the board presumably knows the location, so could set it
> > > up (perhaps in global_data). Then env_get_location() can use it.
> > 
> > Well, it's pretty much what happens when you only have a single
> > environment selected in Kconfig, except that you don't need to change
> > anything in all the boards, which would be the case with your solution
> > I guess?
> > 
> > > Making functions weak means everything becomes non-deterministic -
> > > e.g. I cannot tell from the code what it is going to do.
> > 
> > One of the point of this serie is to allow boards to select the
> > environment locations based on data of their choice, so it's kind of
> > expected that that function should be overriden I guess?
> 
> I am generally a fan of using __weak functions as well, with the weak
> version being clear about what an overriding version is supposed to do,
> when it needs to be overridden.

I'll add a comment then to detail what is expected from this function,
and any function that might override it.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180111/872c4e96/attachment.sig>