[PATCH] mmc: mmc_add_card(): fix missing break in switch statement

[PATCH] mmc: mmc_add_card(): fix missing break in switch statement

[Michał Mirosław]

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

Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement

[Chris Ball]

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

Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement

[Prashanth Bhat]

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;
>    

Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement

[Prashanth Bhat]

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;
>

Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement

[Michał Mirosław]

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

Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement

[Prashanth Bhat]

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
>
>
>    

Re: [PATCH] mmc: mmc_add_card(): fix missing break in switch statement

[Michał Mirosław]

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.