| Message ID | 1323073424-16656-1-git-send-email-thierry.reding@avionic-design.de |
|---|---|
| State | Superseded |
| Delegated to: | Andy Fleming |
| Headers | show |
> The new API no longer uses the extra cd parameter that was used to store > the card presence state. Instead, this information is returned via the > function's return value. board_mmc_getcd() returns -1 to indicate that > no card-detection mechanism is implemented; 0 indicates that no card is > present and 1 is returned if it was detected that a card is present. > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> A silly question -- why do we need this change ? Can you explain it in the changelog of V2 too? M
* Marek Vasut wrote: > > The new API no longer uses the extra cd parameter that was used to store > > the card presence state. Instead, this information is returned via the > > function's return value. board_mmc_getcd() returns -1 to indicate that > > no card-detection mechanism is implemented; 0 indicates that no card is > > present and 1 is returned if it was detected that a card is present. > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > A silly question -- why do we need this change ? Can you explain it in the > changelog of V2 too? It's the first step in implementing card-detection. I discussed this with Andy and he came up with the idea that board_mmc_getcd() should really have had the mmc parameter as first argument in the first place instead of the cd parameter. Furthermore, the cd parameter is used inconsistently in individual implementations. After some discussion we came to the conclusion that the cd parameter wasn't required at all and the same information can be represented in the return value. The whole discussion is in this thread: http://lists.denx.de/pipermail/u-boot/2011-November/110180.html It's not really a necessary change, but it makes board_mmc_getcd() much more consistent with other MMC-related functions. Perhaps this last sentence would be a good explanation to put in the v2 commit message? Thierry
On 05/12/2011 09:23, Thierry Reding wrote: > The new API no longer uses the extra cd parameter that was used to store > the card presence state. Instead, this information is returned via the > function's return value. board_mmc_getcd() returns -1 to indicate that > no card-detection mechanism is implemented; 0 indicates that no card is > present and 1 is returned if it was detected that a card is present. > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> Hi Thierry, it is not clear to me what you think with the new API, or better what you think as new. As I can see, in the current U-Boot TOT there is still : drivers/mmc/mmc.c:int __board_mmc_getcd(u8 *cd, struct mmc *mmc) It seems there is not (yet) a new API... If it is true, which is the reason to change it ? > --- > board/efikamx/efikamx.c | 8 +++----- > board/emk/top9000/top9000.c | 12 ++---------- > board/freescale/mx51evk/mx51evk.c | 8 +++----- > board/freescale/mx53ard/mx53ard.c | 8 +++----- > board/freescale/mx53evk/mx53evk.c | 8 +++----- > board/freescale/mx53loco/mx53loco.c | 8 +++----- > board/freescale/mx53smd/mx53smd.c | 6 ++---- > doc/README.atmel_mci | 12 ++---------- > drivers/mmc/fsl_esdhc.c | 8 +++++--- > drivers/mmc/mmc.c | 4 ++-- > include/mmc.h | 2 +- > 11 files changed, 29 insertions(+), 55 deletions(-) > > diff --git a/board/efikamx/efikamx.c b/board/efikamx/efikamx.c > index b78bf6c..451d709 100644 > --- a/board/efikamx/efikamx.c > +++ b/board/efikamx/efikamx.c > @@ -309,17 +309,15 @@ static inline uint32_t efika_mmc_cd(void) > return MX51_PIN_EIM_CS2; > } > > -int board_mmc_getcd(u8 *absent, struct mmc *mmc) > +int board_mmc_getcd(struct mmc *mmc) > { > struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; > uint32_t cd = efika_mmc_cd(); > > if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) > - *absent = gpio_get_value(IOMUX_TO_GPIO(cd)); > - else > - *absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8)); > + return !gpio_get_value(IOMUX_TO_GPIO(cd)); It seems to me you are inverting the logic. In you commit message, "1" means that a card is detected, exactly as it is done now. Am I wrong ? > diff --git a/board/freescale/mx51evk/mx51evk.c b/board/freescale/mx51evk/mx51evk.c > index 37e6e4d..bc03496 100644 > --- a/board/freescale/mx51evk/mx51evk.c > +++ b/board/freescale/mx51evk/mx51evk.c > @@ -261,16 +261,14 @@ static void power_init(void) > } > > #ifdef CONFIG_FSL_ESDHC > -int board_mmc_getcd(u8 *cd, struct mmc *mmc) > +int board_mmc_getcd(struct mmc *mmc) > { > struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; > > if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) > - *cd = gpio_get_value(0); > - else > - *cd = gpio_get_value(6); > + return !gpio_get_value(0); Ditto. gpio_get_value(0) returns not-zero if the pin is on logical level high, telling that a card was inserted. > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c > index 37ce6e8..936259f 100644 > --- a/drivers/mmc/mmc.c > +++ b/drivers/mmc/mmc.c > @@ -40,11 +40,11 @@ > static struct list_head mmc_devices; > static int cur_dev_num = -1; > > -int __board_mmc_getcd(u8 *cd, struct mmc *mmc) { > +int __board_mmc_getcd(struct mmc *mmc) { > return -1; > } > > -int board_mmc_getcd(u8 *cd, struct mmc *mmc)__attribute__((weak, > +int board_mmc_getcd(struct mmc *mmc)__attribute__((weak, > alias("__board_mmc_getcd"))); Why is it this better as before ? Best regards, Stefano Babic
On 05/12/2011 11:00, Thierry Reding wrote: > * Marek Vasut wrote: >>> The new API no longer uses the extra cd parameter that was used >>> to store the card presence state. Instead, this information is >>> returned via the function's return value. board_mmc_getcd() >>> returns -1 to indicate that no card-detection mechanism is >>> implemented; 0 indicates that no card is present and 1 is >>> returned if it was detected that a card is present. >>> >>> Signed-off-by: Thierry Reding >>> <thierry.reding@avionic-design.de> >> >> A silly question -- why do we need this change ? Can you explain >> it in the changelog of V2 too? > > It's the first step in implementing card-detection. I discussed > this with Andy and he came up with the idea that board_mmc_getcd() > should really have had the mmc parameter as first argument in the > first place instead of the cd parameter. Ok, I get it now. > Furthermore, the cd parameter is used inconsistently in individual > implementations. After some discussion we came to the conclusion > that the cd parameter wasn't required at all and the same > information can be represented in the return value. The whole > discussion is in this thread: > > http://lists.denx.de/pipermail/u-boot/2011-November/110180.html > > It's not really a necessary change, but it makes board_mmc_getcd() > much more consistent with other MMC-related functions. > > Perhaps this last sentence would be a good explanation to put in > the v2 commit message? Ok, thanks - this explains much better which is your intention for the patchset. It is also not bad to add a reference to the above thread. Best regards, Stefano Babic
> On 05/12/2011 11:00, Thierry Reding wrote: > > * Marek Vasut wrote: > >>> The new API no longer uses the extra cd parameter that was used > >>> to store the card presence state. Instead, this information is > >>> returned via the function's return value. board_mmc_getcd() > >>> returns -1 to indicate that no card-detection mechanism is > >>> implemented; 0 indicates that no card is present and 1 is > >>> returned if it was detected that a card is present. > >>> > >>> Signed-off-by: Thierry Reding > >>> <thierry.reding@avionic-design.de> > >> > >> A silly question -- why do we need this change ? Can you explain > >> it in the changelog of V2 too? > > > > It's the first step in implementing card-detection. I discussed > > this with Andy and he came up with the idea that board_mmc_getcd() > > should really have had the mmc parameter as first argument in the > > first place instead of the cd parameter. > > Ok, I get it now. > > > Furthermore, the cd parameter is used inconsistently in individual > > implementations. After some discussion we came to the conclusion > > that the cd parameter wasn't required at all and the same > > information can be represented in the return value. The whole > > discussion is in this thread: > > > > http://lists.denx.de/pipermail/u-boot/2011-November/110180.html > > > > It's not really a necessary change, but it makes board_mmc_getcd() > > much more consistent with other MMC-related functions. > > > > Perhaps this last sentence would be a good explanation to put in > > the v2 commit message? > > Ok, thanks - this explains much better which is your intention for the > patchset. It is also not bad to add a reference to the above thread. > > Best regards, > Stefano Babic Yep, and when/if submitting V2, also make a cover letter (while also adding the explanation to this patch's commit message). Thanks! M
* Stefano Babic wrote: > On 05/12/2011 09:23, Thierry Reding wrote: [...] > > diff --git a/board/efikamx/efikamx.c b/board/efikamx/efikamx.c > > index b78bf6c..451d709 100644 > > --- a/board/efikamx/efikamx.c > > +++ b/board/efikamx/efikamx.c > > @@ -309,17 +309,15 @@ static inline uint32_t efika_mmc_cd(void) > > return MX51_PIN_EIM_CS2; > > } > > > > -int board_mmc_getcd(u8 *absent, struct mmc *mmc) > > +int board_mmc_getcd(struct mmc *mmc) > > { > > struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; > > uint32_t cd = efika_mmc_cd(); > > > > if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) > > - *absent = gpio_get_value(IOMUX_TO_GPIO(cd)); > > - else > > - *absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8)); > > + return !gpio_get_value(IOMUX_TO_GPIO(cd)); > > It seems to me you are inverting the logic. In you commit message, "1" > means that a card is detected, exactly as it is done now. Am I wrong ? The reason is that the fsl_esdhc driver actually calls the "cd" parameter "absent", so the logic is inverted in that driver. This change merely brings the board implementation in line with the new API. Perhaps it would help if the meaning of the return value should be documented explicitly in mmc.h? Thierry
diff --git a/board/efikamx/efikamx.c b/board/efikamx/efikamx.c index b78bf6c..451d709 100644 --- a/board/efikamx/efikamx.c +++ b/board/efikamx/efikamx.c @@ -309,17 +309,15 @@ static inline uint32_t efika_mmc_cd(void) return MX51_PIN_EIM_CS2; } -int board_mmc_getcd(u8 *absent, struct mmc *mmc) +int board_mmc_getcd(struct mmc *mmc) { struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; uint32_t cd = efika_mmc_cd(); if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) - *absent = gpio_get_value(IOMUX_TO_GPIO(cd)); - else - *absent = gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8)); + return !gpio_get_value(IOMUX_TO_GPIO(cd)); - return 0; + return !gpio_get_value(IOMUX_TO_GPIO(MX51_PIN_GPIO1_8)); } int board_mmc_init(bd_t *bis) diff --git a/board/emk/top9000/top9000.c b/board/emk/top9000/top9000.c index 61dee62..d156e32 100644 --- a/board/emk/top9000/top9000.c +++ b/board/emk/top9000/top9000.c @@ -108,17 +108,9 @@ int board_mmc_init(bd_t *bd) } /* this is a weak define that we are overriding */ -int board_mmc_getcd(u8 *cd, struct mmc *mmc) +int board_mmc_getcd(struct mmc *mmc) { - /* - * the only currently existing use of this function - * (fsl_esdhc.c) suggests this function must return - * *cs = TRUE if a card is NOT detected -> in most - * cases the value of the pin when the detect switch - * closes to GND - */ - *cd = at91_get_gpio_value(CONFIG_SYS_MMC_CD_PIN) ? 1 : 0; - return 0; + return !at91_get_gpio_value(CONFIG_SYS_MMC_CD_PIN); } #endif diff --git a/board/freescale/mx51evk/mx51evk.c b/board/freescale/mx51evk/mx51evk.c index 37e6e4d..bc03496 100644 --- a/board/freescale/mx51evk/mx51evk.c +++ b/board/freescale/mx51evk/mx51evk.c @@ -261,16 +261,14 @@ static void power_init(void) } #ifdef CONFIG_FSL_ESDHC -int board_mmc_getcd(u8 *cd, struct mmc *mmc) +int board_mmc_getcd(struct mmc *mmc) { struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) - *cd = gpio_get_value(0); - else - *cd = gpio_get_value(6); + return !gpio_get_value(0); - return 0; + return !gpio_get_value(6); } int board_mmc_init(bd_t *bis) diff --git a/board/freescale/mx53ard/mx53ard.c b/board/freescale/mx53ard/mx53ard.c index be32aee..786770a 100644 --- a/board/freescale/mx53ard/mx53ard.c +++ b/board/freescale/mx53ard/mx53ard.c @@ -83,16 +83,14 @@ struct fsl_esdhc_cfg esdhc_cfg[2] = { {MMC_SDHC2_BASE_ADDR, 1 }, }; -int board_mmc_getcd(u8 *cd, struct mmc *mmc) +int board_mmc_getcd(struct mmc *mmc) { struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) - *cd = gpio_get_value(1); /*GPIO1_1*/ - else - *cd = gpio_get_value(4); /*GPIO1_4*/ + return !gpio_get_value(1); /*GPIO1_1*/ - return 0; + return !gpio_get_value(4); /*GPIO1_4*/ } int board_mmc_init(bd_t *bis) diff --git a/board/freescale/mx53evk/mx53evk.c b/board/freescale/mx53evk/mx53evk.c index 335661f..a4cd983 100644 --- a/board/freescale/mx53evk/mx53evk.c +++ b/board/freescale/mx53evk/mx53evk.c @@ -208,16 +208,14 @@ struct fsl_esdhc_cfg esdhc_cfg[2] = { {MMC_SDHC3_BASE_ADDR, 1}, }; -int board_mmc_getcd(u8 *cd, struct mmc *mmc) +int board_mmc_getcd(struct mmc *mmc) { struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) - *cd = gpio_get_value(77); /*GPIO3_13*/ - else - *cd = gpio_get_value(75); /*GPIO3_11*/ + return !gpio_get_value(77); /*GPIO3_13*/ - return 0; + return !gpio_get_value(75); /*GPIO3_11*/ } int board_mmc_init(bd_t *bis) diff --git a/board/freescale/mx53loco/mx53loco.c b/board/freescale/mx53loco/mx53loco.c index b4c7f33..a0fe5fd 100644 --- a/board/freescale/mx53loco/mx53loco.c +++ b/board/freescale/mx53loco/mx53loco.c @@ -136,16 +136,14 @@ struct fsl_esdhc_cfg esdhc_cfg[2] = { {MMC_SDHC3_BASE_ADDR, 1}, }; -int board_mmc_getcd(u8 *cd, struct mmc *mmc) +int board_mmc_getcd(struct mmc *mmc) { struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv; if (cfg->esdhc_base == MMC_SDHC1_BASE_ADDR) - *cd = gpio_get_value(77); /*GPIO3_13*/ - else - *cd = gpio_get_value(75); /*GPIO3_11*/ + return !gpio_get_value(77); /*GPIO3_13*/ - return 0; + return !gpio_get_value(75); /*GPIO3_11*/ } int board_mmc_init(bd_t *bis) diff --git a/board/freescale/mx53smd/mx53smd.c b/board/freescale/mx53smd/mx53smd.c index 87fa7fa..39274f9 100644 --- a/board/freescale/mx53smd/mx53smd.c +++ b/board/freescale/mx53smd/mx53smd.c @@ -132,11 +132,9 @@ struct fsl_esdhc_cfg esdhc_cfg[1] = { {MMC_SDHC1_BASE_ADDR, 1}, }; -int board_mmc_getcd(u8 *cd, struct mmc *mmc) +int board_mmc_getcd(struct mmc *mmc) { - *cd = gpio_get_value(77); /*GPIO3_13*/ - - return 0; + return !gpio_get_value(77); /*GPIO3_13*/ } int board_mmc_init(bd_t *bis) diff --git a/doc/README.atmel_mci b/doc/README.atmel_mci index dee0cf0..0cbd909 100644 --- a/doc/README.atmel_mci +++ b/doc/README.atmel_mci @@ -59,17 +59,9 @@ int board_mmc_init(bd_t *bd) } /* this is a weak define that we are overriding */ -int board_mmc_getcd(u8 *cd, struct mmc *mmc) +int board_mmc_getcd(struct mmc *mmc) { - /* - * the only currently existing use of this function - * (fsl_esdhc.c) suggests this function must return - * *cs = TRUE if a card is NOT detected -> in most - * cases the value of the pin when the detect switch - * closes to GND - */ - *cd = at91_get_gpio_value (CONFIG_SYS_MMC_CD_PIN) ? 1 : 0; - return 0; + return !at91_get_gpio_value(CONFIG_SYS_MMC_CD_PIN); } #endif diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index ec953f0..f719afd 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -413,7 +413,6 @@ static int esdhc_init(struct mmc *mmc) struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base; int timeout = 1000; int ret = 0; - u8 card_absent; /* Reset the entire host controller */ esdhc_write32(®s->sysctl, SYSCTL_RSTA); @@ -441,7 +440,8 @@ static int esdhc_init(struct mmc *mmc) esdhc_clrsetbits32(®s->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16); /* Check if there is a callback for detecting the card */ - if (board_mmc_getcd(&card_absent, mmc)) { + ret = board_mmc_getcd(mmc); + if (ret < 0) { timeout = 1000; while (!(esdhc_read32(®s->prsstat) & PRSSTAT_CINS) && --timeout) @@ -450,8 +450,10 @@ static int esdhc_init(struct mmc *mmc) if (timeout <= 0) ret = NO_CARD_ERR; } else { - if (card_absent) + if (ret == 0) ret = NO_CARD_ERR; + else + ret = 0; } return ret; diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index 37ce6e8..936259f 100644 --- a/drivers/mmc/mmc.c +++ b/drivers/mmc/mmc.c @@ -40,11 +40,11 @@ static struct list_head mmc_devices; static int cur_dev_num = -1; -int __board_mmc_getcd(u8 *cd, struct mmc *mmc) { +int __board_mmc_getcd(struct mmc *mmc) { return -1; } -int board_mmc_getcd(u8 *cd, struct mmc *mmc)__attribute__((weak, +int board_mmc_getcd(struct mmc *mmc)__attribute__((weak, alias("__board_mmc_getcd"))); int mmc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) diff --git a/include/mmc.h b/include/mmc.h index 015a7f3..a850174 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -314,7 +314,7 @@ struct mmc *find_mmc_device(int dev_num); int mmc_set_dev(int dev_num); void print_mmc_devices(char separator); int get_mmc_num(void); -int board_mmc_getcd(u8 *cd, struct mmc *mmc); +int board_mmc_getcd(struct mmc *mmc); int mmc_switch_part(int dev_num, unsigned int part_num); #ifdef CONFIG_GENERIC_MMC
The new API no longer uses the extra cd parameter that was used to store the card presence state. Instead, this information is returned via the function's return value. board_mmc_getcd() returns -1 to indicate that no card-detection mechanism is implemented; 0 indicates that no card is present and 1 is returned if it was detected that a card is present. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- board/efikamx/efikamx.c | 8 +++----- board/emk/top9000/top9000.c | 12 ++---------- board/freescale/mx51evk/mx51evk.c | 8 +++----- board/freescale/mx53ard/mx53ard.c | 8 +++----- board/freescale/mx53evk/mx53evk.c | 8 +++----- board/freescale/mx53loco/mx53loco.c | 8 +++----- board/freescale/mx53smd/mx53smd.c | 6 ++---- doc/README.atmel_mci | 12 ++---------- drivers/mmc/fsl_esdhc.c | 8 +++++--- drivers/mmc/mmc.c | 4 ++-- include/mmc.h | 2 +- 11 files changed, 29 insertions(+), 55 deletions(-)