On Mon, Jun 10, 2019 at 10:35:36AM +0100, Andre Przywara wrote:
> On Sat, 8 Jun 2019 09:13:52 -0400
> Tom Rini <tr...@konsulko.com> wrote:
>
> Hi Tom,
>
> thanks for having a look!
>
> > On Sat, Jun 08, 2019 at 02:26:54AM +0100, Andre Przywara wrote:
> > > So far we are required to always define the CONFIG_SYS_MMC_ENV_DEV
> > > variable, even if a platform specific function overrides the weak
> > > function that is using it.
> > >
> > > Check for the existence of this Kconfig variable, eliminating the need
> > > to define a dummy value.
> > >
> > > Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
> > > ---
> > > env/mmc.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/env/mmc.c b/env/mmc.c
> > > index c3cf35d01b..122fec3af8 100644
> > > --- a/env/mmc.c
> > > +++ b/env/mmc.c
> > > @@ -124,7 +124,11 @@ __weak int mmc_get_env_addr(struct mmc *mmc, int
> > > copy, u32 *env_addr)
> > >
> > > __weak int mmc_get_env_dev(void)
> > > {
> > > +#ifdef CONFIG_SYS_MMC_ENV_DEV
> > > return CONFIG_SYS_MMC_ENV_DEV;
> > > +#else
> > > + return 0;
> > > +#endif
> > > }
> > >
> > > #ifdef CONFIG_SYS_MMC_ENV_PART
> >
> > Since 0 is a valid device, I'm concerned this might lead to unintended
> > behavior. Can we return some error code here and catch it later?
>
> I see your point. I think originally my idea was that 0 would hopefully be a
> valid device in any case, so it would just work in those cases. But I see the
> trouble that this papers over a bug (namely CONFIG_SYS_MMC_ENV_DEV not being
> defined).
> Looking at all those users I find it rather dangerous (and tedious) to check
> for this in all callers.
>
> What about printing an error message, here in that function? Ideally this
> would be spotted immediately upon initial testing, so would never be exposed
> to a real user.
> Something like "Please either define CONFIG_SYS_MMC_ENV_DEV or provide a
> mmc_get_en_dev() implementation."?That might be OK. Just check the resulting binaries that the string does get discarded from the build when we have a non-weak function in that case. Thanks! -- Tom
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
