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? 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? > + > + 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); > + > + 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()? 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
