diff mbox

[U-Boot,3/3] mx6boards: Fix error handling in board_mmc_init()

Message ID 1416595378-22255-3-git-send-email-festevam@gmail.com
State Awaiting Upstream
Delegated to: Stefano Babic
Headers show

Commit Message

Fabio Estevam Nov. 21, 2014, 6:42 p.m. UTC
From: Fabio Estevam <fabio.estevam@freescale.com>

When an invalid USDHC port is passed we should return -EINVAL instead of 0.
    
Also, return the error immediately on fsl_esdhc_initialize() failure.

Cc: Eric Benard <eric@eukrea.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 board/embest/mx6boards/mx6boards.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Stefano Babic Nov. 24, 2014, 11:06 a.m. UTC | #1
On 21/11/2014 19:42, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> When an invalid USDHC port is passed we should return -EINVAL instead of 0.
>     
> Also, return the error immediately on fsl_esdhc_initialize() failure.
> 
> Cc: Eric Benard <eric@eukrea.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---


Applied to u-boot-imx, thanks !

Best regards,
Stefano Babic
Nikolay Dimitrov Dec. 22, 2014, 4:32 p.m. UTC | #2
Hi Fabio,

On 11/21/2014 08:42 PM, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> When an invalid USDHC port is passed we should return -EINVAL instead of 0.
>
> Also, return the error immediately on fsl_esdhc_initialize() failure.
>
> Cc: Eric Benard <eric@eukrea.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>   board/embest/mx6boards/mx6boards.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/board/embest/mx6boards/mx6boards.c b/board/embest/mx6boards/mx6boards.c
> index 02fb3fa..f8c7468 100644
> --- a/board/embest/mx6boards/mx6boards.c
> +++ b/board/embest/mx6boards/mx6boards.c
> @@ -216,7 +216,7 @@ int board_mmc_getcd(struct mmc *mmc)
>
>   int board_mmc_init(bd_t *bis)
>   {
> -	s32 status = 0;
> +	int ret;
>   	int i;
>
>   	/*
> @@ -268,13 +268,15 @@ int board_mmc_init(bd_t *bis)
>   			printf("Warning: you configured more USDHC controllers"
>   			       "(%d) then supported by the board (%d)\n",
>   			       i + 1, CONFIG_SYS_FSL_USDHC_NUM);
> -			return status;
> +			return -EINVAL;
>   		}
>
> -		status |= fsl_esdhc_initialize(bis, &usdhc_cfg[i]);
> +		ret = fsl_esdhc_initialize(bis, &usdhc_cfg[i]);
> +		if (ret)
> +			return ret;
>   	}
>
> -	return status;
> +	return 0;
>   }
>   #endif

Excuse me for the (very) late question, but just stumbled upon this
patch.

Isn't it possible to continue the initialization of the next ESDHC
module when the current one fails?

Even if there's a case of common initialization failure across all
ESDHC interfaces, isn't it better to continue the initialization and
preserve the interface abstraction, as if we don't know the
implementation details, e.g. pretend we don't know there's a common
failure mechanism across all ESDHCs and still try to init them 1 by 1.

Please don't consider this as NACK, I'm just sharing my thoughts.

Kind regards,
Nikolay
Fabio Estevam Dec. 22, 2014, 4:38 p.m. UTC | #3
On Mon, Dec 22, 2014 at 2:32 PM, Nikolay Dimitrov <picmaster@mail.bg> wrote:

> Excuse me for the (very) late question, but just stumbled upon this
> patch.
>
> Isn't it possible to continue the initialization of the next ESDHC
> module when the current one fails?

If the initialization of any esdhc port failed, it means that
something very bad happened, so better to return the error
immediately.

Regards,

Fabio Estevam
diff mbox

Patch

diff --git a/board/embest/mx6boards/mx6boards.c b/board/embest/mx6boards/mx6boards.c
index 02fb3fa..f8c7468 100644
--- a/board/embest/mx6boards/mx6boards.c
+++ b/board/embest/mx6boards/mx6boards.c
@@ -216,7 +216,7 @@  int board_mmc_getcd(struct mmc *mmc)
 
 int board_mmc_init(bd_t *bis)
 {
-	s32 status = 0;
+	int ret;
 	int i;
 
 	/*
@@ -268,13 +268,15 @@  int board_mmc_init(bd_t *bis)
 			printf("Warning: you configured more USDHC controllers"
 			       "(%d) then supported by the board (%d)\n",
 			       i + 1, CONFIG_SYS_FSL_USDHC_NUM);
-			return status;
+			return -EINVAL;
 		}
 
-		status |= fsl_esdhc_initialize(bis, &usdhc_cfg[i]);
+		ret = fsl_esdhc_initialize(bis, &usdhc_cfg[i]);
+		if (ret)
+			return ret;
 	}
 
-	return status;
+	return 0;
 }
 #endif