On May 21, 2011, at 6:20 PM, Chris Ball wrote: > Hi, > > On Sat, May 21 2011, Mok, Tze Siong wrote: >> The following patch https://patchwork.kernel.org/patch/792162/ is tested using Intel EG20T PCH, the Transcend MMC 1bit card can now be detected, read and write to the card successfully. >> Note : Need to add MMC_CAP_BUS_WIDTH_TEST caps into the SD host controller HW platform code in order to work. >> >> Tested-by: tze.siong.mok@xxxxxxxxx > > Great, thank you. Philip, a few comments: > >> +static int mmc_cmp_ext_csd(u8 *ext_csd, u8 *bw_ext_csd, unsigned bus_width) >> +{ >> + if (ext_csd == NULL || bw_ext_csd == NULL) >> + return bus_width != MMC_BUS_WIDTH_1; >> + >> + if (bus_width == MMC_BUS_WIDTH_1) >> + return 0; >> + >> + /* only compare read only fields */ >> >> + if (ext_csd[EXT_CSD_PARTITION_SUPPORT] != >> + bw_ext_csd[EXT_CSD_PARTITION_SUPPORT]) >> + return -1; >> + >> + if (ext_csd[EXT_CSD_ERASED_MEM_CONT] != >> + bw_ext_csd[EXT_CSD_ERASED_MEM_CONT]) >> + return -2; >> + >> + if (ext_csd[EXT_CSD_REV] != >> + bw_ext_csd[EXT_CSD_REV]) >> + return -3; >> + >> + if (ext_csd[EXT_CSD_STRUCTURE] != >> + bw_ext_csd[EXT_CSD_STRUCTURE]) >> + return -4; >> + >> + if (ext_csd[EXT_CSD_CARD_TYPE] != >> + bw_ext_csd[EXT_CSD_CARD_TYPE]) >> + return -5; >> + >> + if (ext_csd[EXT_CSD_S_A_TIMEOUT] != >> + bw_ext_csd[EXT_CSD_S_A_TIMEOUT]) >> + return -6; >> + >> + if (ext_csd[EXT_CSD_HC_WP_GRP_SIZE] != >> + bw_ext_csd[EXT_CSD_HC_WP_GRP_SIZE]) >> + return -7; >> + >> + if (ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT] != >> + bw_ext_csd[EXT_CSD_ERASE_TIMEOUT_MULT]) >> + return -8; >> + >> + if (ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE] != >> + bw_ext_csd[EXT_CSD_HC_ERASE_GRP_SIZE]) >> + return -9; >> + >> + if (ext_csd[EXT_CSD_SEC_TRIM_MULT] != >> + bw_ext_csd[EXT_CSD_SEC_TRIM_MULT]) >> + return -10; >> + >> + if (ext_csd[EXT_CSD_SEC_ERASE_MULT] != >> + bw_ext_csd[EXT_CSD_SEC_ERASE_MULT]) >> + return -11; >> + >> + if (ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT] != >> + bw_ext_csd[EXT_CSD_SEC_FEATURE_SUPPORT]) >> + return -12; >> + >> + if (ext_csd[EXT_CSD_TRIM_MULT] != >> + bw_ext_csd[EXT_CSD_TRIM_MULT]) >> + return -13; > > Hm, I think people reading dmesg are going to interpret these as errnos, > which they're ambiguous with. Is returning a different number for each > condition important? > No -- was just a way for me to debug -- not important > Perhaps just pick one errno to return, have a single long conditional, > and if we're going to fail all of mmc_init_card() because of an error > here, add a printk explaining the situation to this function? which errro would you suggest. I do not want to have a long conditional unless this is very important. Hard to follow. (see below) > >> + >> + return memcmp(&ext_csd[EXT_CSD_SEC_CNT], >> + &bw_ext_csd[EXT_CSD_SEC_CNT], >> + 4); >> +} >> + >> +static int mmc_compare_ext_csds(struct mmc_card *card, u8 *ext_csd, >> + unsigned bus_width) >> +{ >> + u8 *bw_ext_csd; >> + int err; >> + >> + err = mmc_get_ext_csd(card, &bw_ext_csd); >> + if (!err) >> + err = mmc_cmp_ext_csd(ext_csd, bw_ext_csd, bus_width); would it be better to add if (err) err = ERRNO WE WANT TO USE; >> + >> + mmc_free_ext_csd(bw_ext_csd); >> return err; >> } > > mmc_compare_ext_csds() and mmc_cmp_ext_csd() don't seem like they have > a strong reason for existing as separate functions -- perhaps collapse > them both into a single mmc_compare_ext_csds()? I can do this. > > Thanks! > > - Chris. > -- > Chris Ball <cjb@xxxxxxxxxx> <http://printf.net/> > One Laptop Per Child > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
