RE: [PATCH v2 1/1]mmc: implemented eMMC4.4 enhanced area feature

Dong, Chuanxiao
2011-01-20T06:09:55Z

Hi Chris,
Thanks for comments. I will fix the typo and line warp errors in the next submissions. :)

> > MMC_DEV_ATTR(name, "%s\n", card->cid.prod_name);
> > MMC_DEV_ATTR(oemid, "0x%04x\n", card->cid.oemid);
> > MMC_DEV_ATTR(serial, "0x%08x\n", card->cid.serial);
> > +MMC_DEV_ATTR(enhanced_area_offset, "0x%llxB\n",
> > + card->ext_csd.enhanced_area_offset);
> > +MMC_DEV_ATTR(enhanced_area_size, "0x%xKB\n",
> card->ext_csd.enhanced_area_size);
>
> I think these should probably be printed in decimal instead of hex.
>
> Hm, this way the sysfs files will show up regardless of whether we have
> an eMMC device or an SD card, which is very confusing. Can you not check
> whether enhanced_area_en == 1 before create these files?
>
SD card will not use this code when initializing. So I think these files only are created for eMMC card. If the eMMC card doesn't support enhanced area, can we use the same way like below scenario, exporting some value like -EINVAL to user? What do you think?

> >
> > static struct attribute *mmc_std_attrs[] = {
> > &dev_attr_cid.attr,
> > @@ -349,6 +387,8 @@ static struct attribute *mmc_std_attrs[] = {
> > &dev_attr_name.attr,
> > &dev_attr_oemid.attr,
> > &dev_attr_serial.attr,
> > + &dev_attr_enhanced_area_offset.attr,
> > + &dev_attr_enhanced_area_size.attr,
> > NULL,
> > };
> >
> > @@ -484,6 +524,39 @@ static int mmc_init_card(struct mmc_host *host, u32
> ocr,
> > }
> >
> > /*
> > + * If enhanced_area_en is TRUE, host need to enable
> > + * ERASE_GRP_DEF bit.
> > + * ERASE_GRP_DEF bit will be lost everytime
> > + * after a reset or power off.
> > + */
> > + if (card->ext_csd.enhanced_area_en) {
> > + err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> > + EXT_CSD_ERASE_GROUP_DEF, 1);
> > +
> > + if (err && err != -EBADMSG)
> > + goto free_card;
> > +
> > + if (err) {
> > + err = 0;
> > + /*
> > + * Just disable enhanced area off & sz
> > + * will try to enable ERASE_GROUP_DEF
> > + * during next time reinit
> > + */
> > + card->ext_csd.enhanced_area_offset = 0;
> > + card->ext_csd.enhanced_area_size = 0;
>
> It makes more sense to me to return -EINVAL instead of 0 if we know
> nothing about the enhanced_area. What do you think?
Agree. This can let user know the enhanced area offset and size are invalid argument.

Thanks
Chuanxiao