diff mbox

[U-Boot,V2,03/13] spl: mmc: refactor device location code to its own function

Message ID 1446024210-16517-4-git-send-email-nikita@compulab.co.il
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Nikita Kiryanov Oct. 28, 2015, 9:23 a.m. UTC
Simplify spl_mmc_load_image() code by moving the part that finds the mmc device
into its own function spl_mmc_find_device(), available in two flavors: DM and
non-DM.

This refactor fixes a bug in which an error in the device location sequence
does not necessarily aborts the rest of the code. With this refactor, we fail
the moment there is an error.

Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Paul Kocialkowski <contact@paulk.fr>
Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
Cc: Tom Rini <trini@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
---
Changes in V2:
	- No changes.

 common/spl/spl_mmc.c | 77 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 22 deletions(-)

Comments

Simon Glass Oct. 29, 2015, 5:19 p.m. UTC | #1
Hi Nikita,

On 28 October 2015 at 03:23, Nikita Kiryanov <nikita@compulab.co.il> wrote:
> Simplify spl_mmc_load_image() code by moving the part that finds the mmc device
> into its own function spl_mmc_find_device(), available in two flavors: DM and
> non-DM.
>
> This refactor fixes a bug in which an error in the device location sequence
> does not necessarily aborts the rest of the code. With this refactor, we fail
> the moment there is an error.
>
> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Paul Kocialkowski <contact@paulk.fr>
> Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> ---
> Changes in V2:
>         - No changes.
>
>  common/spl/spl_mmc.c | 77 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 55 insertions(+), 22 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

But can we only have one spl_mmc_find_device() function, with the
#ifdef CONFIG_DM_MMC inside it?

>
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index 6011f77..044be52 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -11,6 +11,7 @@
>  #include <spl.h>
>  #include <linux/compiler.h>
>  #include <asm/u-boot.h>
> +#include <errno.h>
>  #include <mmc.h>
>  #include <image.h>
>
> @@ -59,6 +60,58 @@ end:
>         return 0;
>  }
>
> +#ifdef CONFIG_DM_MMC
> +static int spl_mmc_find_device(struct mmc **mmc)
> +{
> +       struct udevice *dev;
> +       int err;
> +
> +       err = mmc_initialize(NULL);
> +       if (err) {
> +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +               printf("spl: could not initialize mmc. error: %d\n", err);
> +#endif
> +               return err;
> +       }
> +
> +       err = uclass_get_device(UCLASS_MMC, 0, &dev);
> +       if (err) {
> +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +               puts("spl: could not find mmc device. error: %d\n", err);
> +#endif
> +               return err;
> +       }
> +
> +       *mmc = NULL;
> +       *mmc = mmc_get_mmc_dev(dev);
> +       return *mmc != NULL ? 0 : -ENODEV;
> +}
> +#else
> +static int spl_mmc_find_device(struct mmc **mmc)
> +{
> +       int err;
> +
> +       err = mmc_initialize(gd->bd);
> +       if (err) {
> +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +               printf("spl: could not initialize mmc. error: %d\n", err);
> +#endif
> +               return err;
> +       }
> +
> +       /* We register only one device. So, the dev id is always 0 */
> +       *mmc = find_mmc_device(0);
> +       if (!*mmc) {
> +#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)
>  {
> @@ -110,30 +163,10 @@ void spl_mmc_load_image(void)
>         int err = 0;
>         __maybe_unused int part;
>
> -#ifdef CONFIG_DM_MMC
> -       struct udevice *dev;
> -
> -       mmc_initialize(NULL);
> -       err = uclass_get_device(UCLASS_MMC, 0, &dev);
> -       mmc = NULL;
> -       if (!err)
> -               mmc = mmc_get_mmc_dev(dev);
> -#else
> -       mmc_initialize(gd->bd);
> -
> -       /* We register only one device. So, the dev id is always 0 */
> -       mmc = find_mmc_device(0);
> -       if (!mmc) {
> -#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> -               puts("spl: mmc device not found\n");
> -#endif
> +       if (spl_mmc_find_device(&mmc))
>                 hang();
> -       }
> -#endif
> -
> -       if (!err)
> -               err = mmc_init(mmc);
>
> +       err = mmc_init(mmc);
>         if (err) {
>  #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
>                 printf("spl: mmc init failed with error: %d\n", err);
> --
> 1.9.1
>

Regards,
Simon
Nikita Kiryanov Oct. 31, 2015, 3:25 p.m. UTC | #2
Hi Simon,

On Thu, Oct 29, 2015 at 11:19:16AM -0600, Simon Glass wrote:
> Hi Nikita,
> 
> On 28 October 2015 at 03:23, Nikita Kiryanov <nikita@compulab.co.il> wrote:
> > Simplify spl_mmc_load_image() code by moving the part that finds the mmc device
> > into its own function spl_mmc_find_device(), available in two flavors: DM and
> > non-DM.
> >
> > This refactor fixes a bug in which an error in the device location sequence
> > does not necessarily aborts the rest of the code. With this refactor, we fail
> > the moment there is an error.
> >
> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il>
> > Cc: Igor Grinberg <grinberg@compulab.co.il>
> > Cc: Paul Kocialkowski <contact@paulk.fr>
> > Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > ---
> > Changes in V2:
> >         - No changes.
> >
> >  common/spl/spl_mmc.c | 77 +++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 55 insertions(+), 22 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But can we only have one spl_mmc_find_device() function, with the
> #ifdef CONFIG_DM_MMC inside it?

I prefer to have as few #ifdefs inside a function as possible.
Besides, once driver model becomes ubiquitous we're going to have
only one spl_mmc_find_device() anyway.

> > --
> > 1.9.1
> >
> 
> Regards,
> Simon
>
Tom Rini Nov. 3, 2015, 3:56 p.m. UTC | #3
On Wed, Oct 28, 2015 at 11:23:20AM +0200, Nikita Kiryanov wrote:

> Simplify spl_mmc_load_image() code by moving the part that finds the mmc device
> into its own function spl_mmc_find_device(), available in two flavors: DM and
> non-DM.
> 
> This refactor fixes a bug in which an error in the device location sequence
> does not necessarily aborts the rest of the code. With this refactor, we fail
> the moment there is an error.
[snip]
> +#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> +		puts("spl: could not find mmc device. error: %d\n", err);
> +#endif

Should be printf.  And this reminds me that after we dug into things
before, there's really not a reason to use 'puts' sometimes and 'printf'
another, we can always just do printf and it doesn't actually change the
size.
diff mbox

Patch

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index 6011f77..044be52 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -11,6 +11,7 @@ 
 #include <spl.h>
 #include <linux/compiler.h>
 #include <asm/u-boot.h>
+#include <errno.h>
 #include <mmc.h>
 #include <image.h>
 
@@ -59,6 +60,58 @@  end:
 	return 0;
 }
 
+#ifdef CONFIG_DM_MMC
+static int spl_mmc_find_device(struct mmc **mmc)
+{
+	struct udevice *dev;
+	int err;
+
+	err = mmc_initialize(NULL);
+	if (err) {
+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
+		printf("spl: could not initialize mmc. error: %d\n", err);
+#endif
+		return err;
+	}
+
+	err = uclass_get_device(UCLASS_MMC, 0, &dev);
+	if (err) {
+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
+		puts("spl: could not find mmc device. error: %d\n", err);
+#endif
+		return err;
+	}
+
+	*mmc = NULL;
+	*mmc = mmc_get_mmc_dev(dev);
+	return *mmc != NULL ? 0 : -ENODEV;
+}
+#else
+static int spl_mmc_find_device(struct mmc **mmc)
+{
+	int err;
+
+	err = mmc_initialize(gd->bd);
+	if (err) {
+#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
+		printf("spl: could not initialize mmc. error: %d\n", err);
+#endif
+		return err;
+	}
+
+	/* We register only one device. So, the dev id is always 0 */
+	*mmc = find_mmc_device(0);
+	if (!*mmc) {
+#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)
 {
@@ -110,30 +163,10 @@  void spl_mmc_load_image(void)
 	int err = 0;
 	__maybe_unused int part;
 
-#ifdef CONFIG_DM_MMC
-	struct udevice *dev;
-
-	mmc_initialize(NULL);
-	err = uclass_get_device(UCLASS_MMC, 0, &dev);
-	mmc = NULL;
-	if (!err)
-		mmc = mmc_get_mmc_dev(dev);
-#else
-	mmc_initialize(gd->bd);
-
-	/* We register only one device. So, the dev id is always 0 */
-	mmc = find_mmc_device(0);
-	if (!mmc) {
-#ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
-		puts("spl: mmc device not found\n");
-#endif
+	if (spl_mmc_find_device(&mmc))
 		hang();
-	}
-#endif
-
-	if (!err)
-		err = mmc_init(mmc);
 
+	err = mmc_init(mmc);
 	if (err) {
 #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
 		printf("spl: mmc init failed with error: %d\n", err);