Patchwork [U-Boot,1/9] wandboard: Return error if the SDHC port index is invalid

login
register
mail settings
Submitter Fabio Estevam
Date May 21, 2013, 5:32 p.m.
Message ID <1369157542-25734-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/245366/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Fabio Estevam - May 21, 2013, 5:32 p.m.
When the SDHC port number index is invalid we should return an error code 
immediately.

Currently we return 'status', which has a value of zero, causing  
board_mmc_init() to incorrectly return sucess.

Fix this by returning -EINVAL instead.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 board/wandboard/wandboard.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Otavio Salvador - May 21, 2013, 6:26 p.m.
On Tue, May 21, 2013 at 2:32 PM, Fabio Estevam
<fabio.estevam@freescale.com>wrote:

> When the SDHC port number index is invalid we should return an error code
> immediately.
>
> Currently we return 'status', which has a value of zero, causing
> board_mmc_init() to incorrectly return sucess.
>
> Fix this by returning -EINVAL instead.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>

When I looked at this code I didn't change it because it raises a warning.
So it returns the status if any failed.
Fabio Estevam - May 21, 2013, 6:39 p.m.
On Tue, May 21, 2013 at 3:26 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:

> When I looked at this code I didn't change it because it raises a warning.
> So it returns the status if any failed.

Which warning are you talking about? Build warning or run-time warning?

I did not see any of these here.

status is zero at that point of code, so we should not 'return 0' on failure.
Otavio Salvador - May 21, 2013, 6:44 p.m.
On Tue, May 21, 2013 at 3:39 PM, Fabio Estevam <festevam@gmail.com> wrote:

> On Tue, May 21, 2013 at 3:26 PM, Otavio Salvador
> <otavio@ossystems.com.br> wrote:
>
> > When I looked at this code I didn't change it because it raises a
> warning.
> > So it returns the status if any failed.
>
> Which warning are you talking about? Build warning or run-time warning?
>
> I did not see any of these here.
>
> status is zero at that point of code, so we should not 'return 0' on
> failure.
>

In the loop; it does:

status |= fsl...

so it gets the output of it. In case any fails it won't be 0.
Fabio Estevam - May 21, 2013, 6:49 p.m.
On Tue, May 21, 2013 at 3:44 PM, Otavio Salvador
<otavio@ossystems.com.br> wrote:

> In the loop; it does:
>
> status |= fsl...
>
> so it gets the output of it. In case any fails it won't be 0.

It's good practice to return immediately when an error condition happens.
Otavio Salvador - May 21, 2013, 7 p.m.
On Tue, May 21, 2013 at 3:49 PM, Fabio Estevam <festevam@gmail.com> wrote:

> On Tue, May 21, 2013 at 3:44 PM, Otavio Salvador
> <otavio@ossystems.com.br> wrote:
>
> > In the loop; it does:
> >
> > status |= fsl...
> >
> > so it gets the output of it. In case any fails it won't be 0.
>
> It's good practice to return immediately when an error condition happens.
>

Well the idea of the code was to return in case anything goes wrong. So it
outputs a 'WARN'. If you wish to change this, please change to ERROR.
Wolfgang Denk - May 21, 2013, 9:24 p.m.
Dear Fabio Estevam,

In message <1369157542-25734-1-git-send-email-fabio.estevam@freescale.com> you wrote:
> When the SDHC port number index is invalid we should return an error code 
> immediately.
> 
> Currently we return 'status', which has a value of zero, causing  
> board_mmc_init() to incorrectly return sucess.
> 
> Fix this by returning -EINVAL instead.

Can we _please_ remove all this code?  A _runtime_ check for a _build_
_time_ _detectable_ situation makes no sense to me.

For such a misconfiguration, the build should fail.

Handling this at runtime is the wrong approach.


This comment applies for the whole series.


Actually - are you not surprised that you have to fix the same issue
for all boards?  This is a clear indication of duplicated code that
needs to be factored out.

Best regards,

Wolfgang Denk
Fabio Estevam - May 22, 2013, 12:53 a.m.
Hi Wolfgang,

On Tue, May 21, 2013 at 6:24 PM, Wolfgang Denk <wd@denx.de> wrote:

> Can we _please_ remove all this code?  A _runtime_ check for a _build_
> _time_ _detectable_ situation makes no sense to me.
>
> For such a misconfiguration, the build should fail.
>
> Handling this at runtime is the wrong approach.
>
>
> This comment applies for the whole series.

For the wandboard file, would the code bellow be better?

	for (index = 0; index < CONFIG_SYS_FSL_USDHC_NUM; ++index) {
		switch (index) {
		case 0:
			imx_iomux_v3_setup_multiple_pads(
				usdhc3_pads, ARRAY_SIZE(usdhc3_pads));
			usdhc_cfg[0].sdhc_clk = mxc_get_clock(MXC_ESDHC3_CLK);
			usdhc_cfg[0].max_bus_width = 4;
			gpio_direction_input(USDHC3_CD_GPIO);
			break;
		case 1:
			imx_iomux_v3_setup_multiple_pads(
				usdhc1_pads, ARRAY_SIZE(usdhc1_pads));
			usdhc_cfg[1].sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK);
			usdhc_cfg[1].max_bus_width = 4;
			gpio_direction_input(USDHC1_CD_GPIO);
			break;
		}

		status = fsl_esdhc_initialize(bis, &usdhc_cfg[index]);
		if (status)
			return status;
	}

	return 0;	
>
>
> Actually - are you not surprised that you have to fix the same issue
> for all boards?  This is a clear indication of duplicated code that
> needs to be factored out.

I will try factor out.

Regards,

Fabio Estevam

Patch

diff --git a/board/wandboard/wandboard.c b/board/wandboard/wandboard.c
index bb98352..70e070c 100644
--- a/board/wandboard/wandboard.c
+++ b/board/wandboard/wandboard.c
@@ -164,7 +164,7 @@  int board_mmc_init(bd_t *bis)
 			printf("Warning: you configured more USDHC controllers"
 			       "(%d) then supported by the board (%d)\n",
 			       index + 1, CONFIG_SYS_FSL_USDHC_NUM);
-			return status;
+			return -EINVAL;
 		}
 
 		status |= fsl_esdhc_initialize(bis, &usdhc_cfg[index]);