On Mon, Jun 10, 2019 at 10:35:36AM +0100, Andre Przywara wrote:
On Sat, 8 Jun 2019 09:13:52 -0400 Tom Rini trini@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.przywara@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!