| Message ID | 1369157542-25734-1-git-send-email-fabio.estevam@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
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.
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.
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.
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.
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.
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
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
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]);
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(-)