| Message ID | 20110409061647.D5DAE13909@rere.qmqm.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Sat, Apr 09 2011, Micha? Miros?aw wrote: > Fixes a cosmetic bug that affects printk() for SD-combo cards. > > Noticed-by: Prashanth Bhat <prashanth.bhat@manipal.net> > Signed-off-by: Micha? Miros?aw <mirq-linux@rere.qmqm.pl> > --- > drivers/mmc/core/bus.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c > index 63667a8..d6d62fd 100644 > --- a/drivers/mmc/core/bus.c > +++ b/drivers/mmc/core/bus.c > @@ -284,6 +284,7 @@ int mmc_add_card(struct mmc_card *card) > type = "SD-combo"; > if (mmc_card_blockaddr(card)) > type = "SDHC-combo"; > + break; > default: > type = "?"; > break; Thanks, pushed to mmc-next for .40. - Chris.
Michal, I hope you remember the email exchange we had about a month ago. I think I see an issue with SD-COMBO support. In include/linux/mmc/card.h, there are macros: > #define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC) > #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD) > #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO) In particular, the mmc_card_sd(c) is invoked in many places, to determine whether the card is SD, and certain action is taken. Shouldn't the same be done for Combo cards, because these also have SD on them? For instance, in drivers/mmc/core/core.c, routine mmc_set_data_timeout(), there is a call to mmc_card_sd(c). Also, in drivers/mmc/card/block.c, routine mmc_blk_alloc(). I would appreciate your opinion on this. Thanks, Prashanth On Saturday 09 April 2011 11:46 AM, Micha? Miros?aw wrote: > Fixes a cosmetic bug that affects printk() for SD-combo cards. > > Noticed-by: Prashanth Bhat<prashanth.bhat@manipal.net> > Signed-off-by: Micha? Miros?aw<mirq-linux@rere.qmqm.pl> > --- > drivers/mmc/core/bus.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c > index 63667a8..d6d62fd 100644 > --- a/drivers/mmc/core/bus.c > +++ b/drivers/mmc/core/bus.c > @@ -284,6 +284,7 @@ int mmc_add_card(struct mmc_card *card) > type = "SD-combo"; > if (mmc_card_blockaddr(card)) > type = "SDHC-combo"; > + break; > default: > type = "?"; > break; > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
To be more specific, I would think that the code change required in include/linux/mmc/card.h is: #define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC) - #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD) - #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO) + #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD || (c)->type == MMC_TYPE_SD_COMBO) + #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO || (c)->type == MMC_TYPE_SD_COMBO) Thanks, Prashanth On Tuesday 03 May 2011 04:50 PM, Prashanth Bhat wrote: > Michal, > > I hope you remember the email exchange we had about a month ago. I > think I see an issue with SD-COMBO support. > > In include/linux/mmc/card.h, there are macros: > >> #define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC) >> #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD) >> #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO) > > In particular, the mmc_card_sd(c) is invoked in many places, to > determine whether the card is SD, and certain action is taken. > Shouldn't the same be done for Combo cards, because these also have SD > on them? > > For instance, in drivers/mmc/core/core.c, routine > mmc_set_data_timeout(), there is a call to mmc_card_sd(c). > Also, in drivers/mmc/card/block.c, routine mmc_blk_alloc(). > > I would appreciate your opinion on this. > > Thanks, > Prashanth > > > On Saturday 09 April 2011 11:46 AM, Micha? Miros?aw wrote: >> Fixes a cosmetic bug that affects printk() for SD-combo cards. >> >> Noticed-by: Prashanth Bhat<prashanth.bhat@manipal.net> >> Signed-off-by: Micha? Miros?aw<mirq-linux@rere.qmqm.pl> >> --- >> drivers/mmc/core/bus.c | 1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c >> index 63667a8..d6d62fd 100644 >> --- a/drivers/mmc/core/bus.c >> +++ b/drivers/mmc/core/bus.c >> @@ -284,6 +284,7 @@ int mmc_add_card(struct mmc_card *card) >> type = "SD-combo"; >> if (mmc_card_blockaddr(card)) >> type = "SDHC-combo"; >> + break; >> default: >> type = "?"; >> break; > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 03, 2011 at 05:00:03PM +0530, Prashanth Bhat wrote: > To be more specific, I would think that the code change required in > include/linux/mmc/card.h is: > > #define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC) > - #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD) > - #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO) > > + #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD || > (c)->type == MMC_TYPE_SD_COMBO) > + #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO || > (c)->type == MMC_TYPE_SD_COMBO) You can actually use (c->type & MMC_TYPE_SD) and (c->type & MMC_TYPE_SDIO). Unless there will be more types of SD cards (unlikely) this way will generate less code on average. Best Regards, Micha? Miros?aw -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Interesting coincidence that the value of MMC_TYPE_SD and MMC_TYPE_SDIO and MMC_TYPE_SD_COMBO have the relationship that SD_COMBO is the bitwise OR of the other two. However, this seems to be more of a coincidence than intentional. The #defines were clearly meant to be numeric values rather than bit-masks. #define MMC_TYPE_MMC 0 /* MMC card */ #define MMC_TYPE_SD 1 /* SD card */ #define MMC_TYPE_SDIO 2 /* SDIO card */ #define MMC_TYPE_SD_COMBO 3 /* SD combo (IO+mem) card */ Using the bit-mask approach therefore doesn't feel like the natural way to me. Perhaps the #defines could be changed to #define MMC_TYPE_MMC (1 << 0) /* MMC card */ #define MMC_TYPE_SD (1 << 1) /* SD card */ #define MMC_TYPE_SDIO (1 << 2) /* SDIO card */ #define MMC_TYPE_SD_COMBO (MMC_TYPE_SD | MMC_TYPE_SDIO) /* SD combo (IO+mem) card */ Thanks, Prashanth On Wednesday 04 May 2011 04:53 PM, Micha? Miros?aw wrote: > On Tue, May 03, 2011 at 05:00:03PM +0530, Prashanth Bhat wrote: > >> To be more specific, I would think that the code change required in >> include/linux/mmc/card.h is: >> >> #define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC) >> - #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD) >> - #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO) >> >> + #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD || >> (c)->type == MMC_TYPE_SD_COMBO) >> + #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO || >> (c)->type == MMC_TYPE_SD_COMBO) >> > You can actually use (c->type& MMC_TYPE_SD) and (c->type& MMC_TYPE_SDIO). > Unless there will be more types of SD cards (unlikely) this way > will generate less code on average. > > Best Regards, > Micha? Miros?aw > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 04, 2011 at 10:06:20PM +0530, Prashanth Bhat wrote: > On Wednesday 04 May 2011 04:53 PM, Micha? Miros?aw wrote: > >On Tue, May 03, 2011 at 05:00:03PM +0530, Prashanth Bhat wrote: > >>To be more specific, I would think that the code change required in > >>include/linux/mmc/card.h is: > >> > >>#define mmc_card_mmc(c) ((c)->type == MMC_TYPE_MMC) > >>- #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD) > >>- #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO) > >> > >>+ #define mmc_card_sd(c) ((c)->type == MMC_TYPE_SD || > >>(c)->type == MMC_TYPE_SD_COMBO) > >>+ #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO || > >>(c)->type == MMC_TYPE_SD_COMBO) > >You can actually use (c->type& MMC_TYPE_SD) and (c->type& MMC_TYPE_SDIO). > >Unless there will be more types of SD cards (unlikely) this way > >will generate less code on average. > Interesting coincidence that the value of MMC_TYPE_SD and > MMC_TYPE_SDIO and MMC_TYPE_SD_COMBO have the relationship that > SD_COMBO is the bitwise OR of the other two. However, this seems to > be more of a coincidence than intentional. The #defines were clearly > meant to be numeric values rather than bit-masks. > > #define MMC_TYPE_MMC 0 /* MMC card */ > #define MMC_TYPE_SD 1 /* SD card */ > #define MMC_TYPE_SDIO 2 /* SDIO card */ > #define MMC_TYPE_SD_COMBO 3 /* SD combo (IO+mem) card */ > > > Using the bit-mask approach therefore doesn't feel like the natural > way to me. Perhaps the #defines could be changed to > > #define MMC_TYPE_MMC (1 << 0) /* MMC card */ > #define MMC_TYPE_SD (1 << 1) /* SD card */ > #define MMC_TYPE_SDIO (1 << 2) /* SDIO card */ > #define MMC_TYPE_SD_COMBO (MMC_TYPE_SD | MMC_TYPE_SDIO) /* > SD combo (IO+mem) card */ Since MMC == !SD && !SDIO, then MMC_TYPE_MMC bit would carry redundant information. Best Regards, Micha? Miros?aw PS. Please do not top-post. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c index 63667a8..d6d62fd 100644 --- a/drivers/mmc/core/bus.c +++ b/drivers/mmc/core/bus.c @@ -284,6 +284,7 @@ int mmc_add_card(struct mmc_card *card) type = "SD-combo"; if (mmc_card_blockaddr(card)) type = "SDHC-combo"; + break; default: type = "?"; break;
Fixes a cosmetic bug that affects printk() for SD-combo cards. Noticed-by: Prashanth Bhat <prashanth.bhat@manipal.net> Signed-off-by: Micha? Miros?aw <mirq-linux@rere.qmqm.pl> --- drivers/mmc/core/bus.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)