* [PATCH] mmc: mmc_add_card(): fix missing break in switch statement @ 2011-04-09 6:16 Michał Mirosław 2011-04-09 14:08 ` Chris Ball 2011-05-03 11:20 ` Prashanth Bhat 0 siblings, 2 replies; 7+ messages in thread From: Michał Mirosław @ 2011-04-09 6:16 UTC (permalink / raw) To: linux-mmc; +Cc: Chris Ball, Prashanth Bhat 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; -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement 2011-04-09 6:16 [PATCH] mmc: mmc_add_card(): fix missing break in switch statement Michał Mirosław @ 2011-04-09 14:08 ` Chris Ball 2011-05-03 11:20 ` Prashanth Bhat 1 sibling, 0 replies; 7+ messages in thread From: Chris Ball @ 2011-04-09 14:08 UTC (permalink / raw) To: Michał Mirosław; +Cc: linux-mmc, Prashanth Bhat 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. -- Chris Ball <cjb@laptop.org> <http://printf.net/> One Laptop Per Child ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement 2011-04-09 6:16 [PATCH] mmc: mmc_add_card(): fix missing break in switch statement Michał Mirosław 2011-04-09 14:08 ` Chris Ball @ 2011-05-03 11:20 ` Prashanth Bhat 2011-05-03 11:30 ` Prashanth Bhat 1 sibling, 1 reply; 7+ messages in thread From: Prashanth Bhat @ 2011-05-03 11:20 UTC (permalink / raw) To: Michał Mirosław; +Cc: linux-mmc, Chris Ball 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; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement 2011-05-03 11:20 ` Prashanth Bhat @ 2011-05-03 11:30 ` Prashanth Bhat 2011-05-04 11:23 ` Michał Mirosław 0 siblings, 1 reply; 7+ messages in thread From: Prashanth Bhat @ 2011-05-03 11:30 UTC (permalink / raw) To: Michał Mirosław; +Cc: linux-mmc, Chris Ball 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; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement 2011-05-03 11:30 ` Prashanth Bhat @ 2011-05-04 11:23 ` Michał Mirosław 2011-05-04 16:36 ` Prashanth Bhat 0 siblings, 1 reply; 7+ messages in thread From: Michał Mirosław @ 2011-05-04 11:23 UTC (permalink / raw) To: Prashanth Bhat; +Cc: linux-mmc, Chris Ball 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement 2011-05-04 11:23 ` Michał Mirosław @ 2011-05-04 16:36 ` Prashanth Bhat 2011-05-04 16:42 ` Michał Mirosław 0 siblings, 1 reply; 7+ messages in thread From: Prashanth Bhat @ 2011-05-04 16:36 UTC (permalink / raw) To: Michał Mirosław; +Cc: linux-mmc, Chris Ball 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement 2011-05-04 16:36 ` Prashanth Bhat @ 2011-05-04 16:42 ` Michał Mirosław 0 siblings, 0 replies; 7+ messages in thread From: Michał Mirosław @ 2011-05-04 16:42 UTC (permalink / raw) To: Prashanth Bhat; +Cc: linux-mmc, Chris Ball 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-05-04 16:42 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-09 6:16 [PATCH] mmc: mmc_add_card(): fix missing break in switch statement Michał Mirosław 2011-04-09 14:08 ` Chris Ball 2011-05-03 11:20 ` Prashanth Bhat 2011-05-03 11:30 ` Prashanth Bhat 2011-05-04 11:23 ` Michał Mirosław 2011-05-04 16:36 ` Prashanth Bhat 2011-05-04 16:42 ` Michał Mirosław
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).