[U-Boot] [PATCH v3 0/3] spl: mmc: Fix warning and unify spl_mmc_find_device()

[U-Boot] [PATCH v3 0/3] spl: mmc: Fix warning and unify spl_mmc_find_device()

[Simon Glass]

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 v3:
- Move the NULL assignment to the caller

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 | 45 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 34 deletions(-)

-- 
2.6.0.rc2.230.g3dd15c0

[U-Boot] [PATCH v3 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Simon Glass]

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>
Tested-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v3:
- Move the NULL assignment to the caller

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..43748d0 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -248,7 +248,7 @@ int spl_mmc_do_fs_boot(struct mmc *mmc)
 
 int spl_mmc_load_image(u32 boot_device)
 {
-	struct mmc *mmc;
+	struct mmc *mmc = NULL;
 	u32 boot_mode;
 	int err = 0;
 	__maybe_unused int part;
-- 
2.6.0.rc2.230.g3dd15c0

[U-Boot] [PATCH v3 2/3] spl: mmc: Rename 'mmc' variable to 'mmcp'

[Simon Glass]

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>
Tested-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v3: None
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 43748d0..d8b514e 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -79,7 +79,7 @@ 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;
@@ -104,12 +104,12 @@ 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;
+	*mmcp = NULL;
+	*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
-- 
2.6.0.rc2.230.g3dd15c0

[U-Boot] [PATCH v3 3/3] spl: mmc: Unify non/driver model spl_mmc_find_device()

[Simon Glass]

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>
Tested-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v3: None
Changes in v2: None

 common/spl/spl_mmc.c | 41 +++++++++--------------------------------
 1 file changed, 9 insertions(+), 32 deletions(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index d8b514e..f2c1af5 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;
 
 	mmc_dev = spl_mmc_get_device_index(boot_device);
@@ -96,47 +97,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 = NULL;
-	*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)
-- 
2.6.0.rc2.230.g3dd15c0

[U-Boot] [PATCH v3 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Nikita Kiryanov]

On Wed, Dec 02, 2015 at 10:59:11AM -0700, Simon Glass wrote:
> 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.

Acked-by: Nikita Kiryanov <nikita@compulab.co.il>

> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Tested-by: Michal Simek <michal.simek@xilinx.com>
> ---

[U-Boot] [U-Boot, v3, 1/3] spl: mmc: Fix compiler warning with CONFIG_DM_MMC

[Tom Rini]

On Wed, Dec 02, 2015 at 10:59:11AM -0700, Simon Glass wrote:

> 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>
> Tested-by: Michal Simek <michal.simek@xilinx.com>
> Acked-by: Nikita Kiryanov <nikita@compulab.co.il>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151206/5f3c6b40/attachment.sig>

[U-Boot] [U-Boot, v3, 2/3] spl: mmc: Rename 'mmc' variable to 'mmcp'

[Tom Rini]

On Wed, Dec 02, 2015 at 10:59:12AM -0700, Simon Glass wrote:

> 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>
> Tested-by: Michal Simek <michal.simek@xilinx.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151206/c9c9519a/attachment.sig>

[U-Boot] [U-Boot, v3, 3/3] spl: mmc: Unify non/driver model spl_mmc_find_device()

[Tom Rini]

On Wed, Dec 02, 2015 at 10:59:13AM -0700, Simon Glass wrote:

> 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>
> Tested-by: Michal Simek <michal.simek@xilinx.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151206/13a46752/attachment.sig>