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

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180109/af216d83/attachment.sig>