[PATCH v4 4/4] phytec: phycore_imx8mp: override env_get_location

[Tommaso Merciai]

On Tue, Feb 01, 2022 at 05:22:06AM -0600, Adam Ford wrote:
> On Tue, Feb 1, 2022 at 3:09 AM Tommaso Merciai
> <tommaso.merciai at amarulasolutions.com> wrote:
> >
> > On Tue, Feb 01, 2022 at 04:16:52AM +0100, Marek Vasut wrote:
> > > On 1/31/22 23:15, Tommaso Merciai wrote:
> > > > On Mon, Jan 31, 2022 at 06:03:58PM +0100, Marek Vasut wrote:
> > > > > On 1/31/22 17:58, Tommaso Merciai wrote:
> > > > > > Override env_get_location function at board level, previously dropped
> > > > > > down from arch/arm/mach-imx/imx8m/soc.c
> > > > > >
> > > > > > References:
> > > > > >    - commit 98af80d3c969e69a1b8ce98bb20e5ad844022da2
> > > > > >
> > > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai at amarulasolutions.com>
> > > > > > ---
> > > > > >    board/phytec/phycore_imx8mp/phycore-imx8mp.c | 33 ++++++++++++++++++++
> > > > > >    1 file changed, 33 insertions(+)
> > > > > >
> > > > > > diff --git a/board/phytec/phycore_imx8mp/phycore-imx8mp.c b/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> > > > > > index a8f0821437..05926eefa3 100644
> > > > > > --- a/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> > > > > > +++ b/board/phytec/phycore_imx8mp/phycore-imx8mp.c
> > > > > > @@ -11,9 +11,42 @@
> > > > > >    #include <asm/mach-imx/boot_mode.h>
> > > > > >    #include <env.h>
> > > > > >    #include <miiphy.h>
> > > > > > +#include <env_internal.h>
> > > > > >    DECLARE_GLOBAL_DATA_PTR;
> > > > > > +enum env_location env_get_location(enum env_operation op, int prio)
> > > > > > +{
> > > > >
> > > > > Why don't you just turn this into default __weak function and override it on
> > > > > board level when it is really needed to be overridden ?
> > > >
> > > > Hi Marek,
> > > > env_get_location is already declared as __weak, check env/env.c. We
> > > > can't override it 2 times.
> > >
> > > Oh, it is this problem with missing ability to define multiple levels of
> > > symbol strength.
> > >
> > > __weak enum env_location arch_env_get_location(enum env_operation op, int
> > > prio)
> > > {
> > >         if (prio >= ARRAY_SIZE(env_locations))
> > >                 return ENVL_UNKNOWN;
> > >
> > >         return env_locations[prio];
> > > }
> > >
> > > __weak enum env_location board_env_get_location(enum env_operation op, int
> > > prio)
> > > {
> > >       return arch_env_get_location(op, prio);
> > > }
> > >
> > > __weak enum env_location env_get_location(enum env_operation op, int prio)
> > > {
> > >       return board_env_get_location(op, prio);
> > > }
> > >
> > > By default, the compiler will optimize it all out. If you have arch-specific
> > > default (like imx does), implement arch_env_get_location(), if you have even
> > > board-specific default (like your board likely does), implement
> > > board_env_get_location(), if you need to override both, then override
> > > env_get_location() (unlikely).
> > >
> > > This is also inline with all the other arch_*() and board_*() functions we
> > > have, and you won't have much duplication either.
> >
> > Hi Marek,
> > Thanks for the tips, then if I understand correctly, your idea is: use:
> >
> > arch_env_get_location in (soc.c)
> >
> > In this way imx8m users can override this function at board level using:
> >
> > board_env_get_location
> >
> > right?
> 
> What about those of us who want to use the default option found in
> env.c?

Hi Adam,
You are right. Mmm in this way you have to duplicate the code of env.c
into your board.c. This doesn't look very functional.
I think remove it from soc.c is the right way.

> It seems like we're creating more abstraction to address the
> abstraction we don't all want.  From my interpretation, the whole
> point of creating the default in env.c was to let people define the
> location of their environment, and this function in soc.c undid that.
> If people want it for their boards, put this function in their boards,
> otherwise, just use the default, or write your own.

Ack.

tommaso
> 
> adam
> >
> > Thanks,
> > Tommaso
> >
> > --
> > Tommaso Merciai
> > Embedded Linux Developer
> > tommaso.merciai at amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > T. +39 042 243 5310
> > info at amarulasolutions.com
> > www.amarulasolutions.com

-- 
Tommaso Merciai
Embedded Linux Developer
tommaso.merciai at amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info at amarulasolutions.com
www.amarulasolutions.com