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

[Dong, Chuanxiao]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/