[U-Boot] [PATCH v2 13/15] env: Mark env_get_location as weak

[Tom Rini <trini@konsulko.com>]

On Mon, Jan 22, 2018 at 09:48:26AM -0700, Simon Glass wrote:
> Hi,
> 
> On 22 January 2018 at 09:36, Tom Rini <trini@konsulko.com> wrote:
> > On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote:
> >> On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:
> >> > On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:
> >> > > Hi,
> >> > >
> >> > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
> >> > > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
> >> > > > >> On 16 January 2018 at 01:16, Maxime Ripard
> >> > > > >> <maxime.ripard@free-electrons.com> wrote:
> >> > > > >> > Allow boards and architectures to override the default environment lookup
> >> > > > >> > code by overriding env_get_location.
> >> > > > >> >
> >> > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> >> > > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> >> > > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >> > > > >> > ---
> >> > > > >> >  env/env.c | 20 +++++++++++++++++++-
> >> > > > >> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >> > > > >> >
> >> > > > >>
> >> > > > >> I still don't really understand why this needs to be a weak function.
> >> > > > >> If the board knows the priority order, can it not put it into
> >> > > > >> global_data? We could have a little u8 array of 4 items with a
> >> > > > >> terminator?
> >> > > > >
> >> > > > > Sure that would be simpler, but that would also prevent us from doing
> >> > > > > "smart" things based on data other than just whether the previous
> >> > > > > environment is usable. Things based for example on a GPIO state, or a
> >> > > > > custom algorithm to transition (or duplicate) the environment.
> >> > > >
> >> > > > In that case the board could read the GPIO state, or the algorithm,
> >> > > > and then set up the value.
> >> > > >
> >> > > > Basically I am saying it could set up the priority order in advance of
> >> > > > it being needed, rather than having a callback.
> >> > >
> >> > > Aren't we kind of stuck here?
> >> > >
> >> > > On the previous iterations, we already discussed this and Tom
> >> > > eventually told he was in favour of __weak functions, and the
> >> > > discussion stopped there. I assumed you were ok with it.
> >> > >
> >> > > I'd really want to move forward on that. This is something that is
> >> > > really biting us *now* and I'd hate to miss yet another merge window
> >> > > because of debates like this.
> >> >
> >> > Yes, I think this is where we want things to be weak, with a reasonable
> >> > default.  If we start to see that "everyone" does the same thing by and
> >> > large we can re-evaluate.
> >>
> >> Ok.
> >>
> >> I've fixed the bug I mentionned the other day on IRC, should I send a PR?
> >
> > Lets give Simon a chance to provide any other feedback here, or another
> > argument to convince me that no, we don't want to have this abstracted
> > by a weak function but instead ..., thanks!
> 
> I suspect there is a reason why this is better than what I propose.
> Perhaps when I try it out it will become apparent.
> 
> So let's go ahead and revisit later if we have new information.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks!  Maxime, please re-spin with the bugfix (or wait another day or
two for other feedback), repost and I'll take it in Thurs/Fri or so.

-- 
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/20180122/f752a01e/attachment.sig>