[U-Boot] [PATCH v2 0/3] spl: mmc: Fix warning and unify spl_mmc_find_device()
This little series fixes a compiler warning in SPL MMC which affects Rockchip. It also joins up the spl_mmc_find_device() function again. I feel that splitting the function was the wrong approach as discussed here:
https://patchwork.ozlabs.org/patch/537276/
The first patch fixes the warning. The other two are suggested improvements but are separate from that problem.
Changes in v2: - Fix the warning in the commit message (it was the wrong one)
Simon Glass (3): spl: mmc: Fix compiler warning with CONFIG_DM_MMC spl: mmc: Rename 'mmc' variable to 'mmcp' spl: mmc: Unify non/driver model spl_mmc_find_device()
common/spl/spl_mmc.c | 44 +++++++++++--------------------------------- 1 file changed, 11 insertions(+), 33 deletions(-)
Since commit 4188ba3 we get the following warning on rockchip boards:
common/spl/spl_mmc.c:31:24: warning: ‘mmc’ may be used uninitialized in this function [-Wmaybe-uninitialized] count = mmc->block_dev.block_read(0, sector, 1, header); ^ common/spl/spl_mmc.c:251:14: note: ‘mmc’ was declared here struct mmc *mmc;
Correct this by move the variable init earlier.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Fix the warning in the commit message (it was the wrong one)
common/spl/spl_mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index b3c2c64..9df4786 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -84,6 +84,7 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device) struct udevice *dev; int err, mmc_dev;
+ *mmc = NULL; mmc_dev = spl_mmc_get_device_index(boot_device); if (mmc_dev < 0) return mmc_dev; @@ -104,7 +105,6 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device) return err; }
- *mmc = NULL; *mmc = mmc_get_mmc_dev(dev); return *mmc != NULL ? 0 : -ENODEV; }
The 'p' suffix makes it more obvious that we are dealing with a pointer to a (pointer) value that will be returned to its caller.
Signed-off-by: Simon Glass sjg@chromium.org Acked-by: Nikita Kiryanov nikita@compulab.co.il ---
Changes in v2: None
common/spl/spl_mmc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index 9df4786..8d47059 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -79,12 +79,12 @@ int spl_mmc_get_device_index(u32 boot_device) }
#ifdef CONFIG_DM_MMC -static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device) +static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device) { struct udevice *dev; int err, mmc_dev;
- *mmc = NULL; + *mmcp = NULL; mmc_dev = spl_mmc_get_device_index(boot_device); if (mmc_dev < 0) return mmc_dev; @@ -105,11 +105,11 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device) return err; }
- *mmc = mmc_get_mmc_dev(dev); - return *mmc != NULL ? 0 : -ENODEV; + *mmcp = mmc_get_mmc_dev(dev); + return *mmcp != NULL ? 0 : -ENODEV; } #else -static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device) +static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device) { int err, mmc_dev;
@@ -126,8 +126,8 @@ static int spl_mmc_find_device(struct mmc **mmc, u32 boot_device) }
/* We register only one device. So, the dev id is always 0 */ - *mmc = find_mmc_device(mmc_dev); - if (!*mmc) { + *mmcp = find_mmc_device(mmc_dev); + if (!*mmcp) { #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT puts("spl: mmc device not found\n"); #endif
It is risky to have two different functions with much the same code. Future authors may update one but not the other. It is hard to see which parts are the same and which are different.
Unify the functions and drop the differences that are not really needed.
Note that one puts() becomes printf() as Tom mentioned that this does not affect image size:
https://patchwork.ozlabs.org/patch/537276/
Note: It would be better to have an empty printf() and avoid the #ifdef for CONFIG_SPL_LIBCOMMON_SUPPORT.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
common/spl/spl_mmc.c | 40 +++++++++------------------------------- 1 file changed, 9 insertions(+), 31 deletions(-)
diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c index 8d47059..d9416a8 100644 --- a/common/spl/spl_mmc.c +++ b/common/spl/spl_mmc.c @@ -78,10 +78,11 @@ int spl_mmc_get_device_index(u32 boot_device) return -ENODEV; }
-#ifdef CONFIG_DM_MMC static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device) { +#ifdef CONFIG_DM_MMC struct udevice *dev; +#endif int err, mmc_dev;
*mmcp = NULL; @@ -97,46 +98,23 @@ static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device) return err; }
+#ifdef CONFIG_DM_MMC err = uclass_get_device(UCLASS_MMC, mmc_dev, &dev); - if (err) { -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT - printf("spl: could not find mmc device. error: %d\n", err); -#endif - return err; - } - - *mmcp = mmc_get_mmc_dev(dev); - return *mmcp != NULL ? 0 : -ENODEV; -} + if (!err) + *mmcp = mmc_get_mmc_dev(dev); #else -static int spl_mmc_find_device(struct mmc **mmcp, u32 boot_device) -{ - int err, mmc_dev; - - mmc_dev = spl_mmc_get_device_index(boot_device); - if (mmc_dev < 0) - return mmc_dev; - - err = mmc_initialize(gd->bd); + *mmcp = find_mmc_device(mmc_dev); + err = *mmcp ? 0 : -ENODEV; +#endif if (err) { #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT - printf("spl: could not initialize mmc. error: %d\n", err); + printf("spl: could not find mmc device. error: %d\n", err); #endif return err; }
- /* We register only one device. So, the dev id is always 0 */ - *mmcp = find_mmc_device(mmc_dev); - if (!*mmcp) { -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT - puts("spl: mmc device not found\n"); -#endif - return -ENODEV; - } - return 0; } -#endif
#ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION static int mmc_load_image_raw_partition(struct mmc *mmc, int partition)
participants (1)
-
Simon Glass