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

[Prashanth Bhat <prashanth.bhat@manipal.net>]

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