Re: [PATCH v2 2/2] mci: implement command to switch a mmc device to enhanced mode

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Uwe,

On Tue, Oct 02, 2018 at 10:33:10AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jan 22, 2018 at 09:12:34AM +0100, Sascha Hauer wrote:
> > On Fri, Jan 19, 2018 at 02:28:25PM +0100, Uwe Kleine-König wrote:
> > > new file mode 100644
> > > index 000000000000..2f5c7ad69a37
> > > --- /dev/null
> > > +++ b/commands/mmc.c
> > > @@ -0,0 +1,171 @@
> > > +#include <command.h>
> > > +#include <mci.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +
> > > +/* enh_area setmax <-y|-n|-c> /dev/mmcX */
> > > +static int do_mmc_enh_area(int argc, char *argv[])
> > > +{
> > > +	char *devname;
> > > +	struct mci *mci;
> > > +	u8 *ext_csd;
> > > +	int set_completed = 0;
> > > +	int ret;
> > > +
> > > +	if (argc != 4 || strcmp(argv[1], "setmax") ||
> > > +	    argv[2][0] != '-' ||
> > > +	    (argv[2][1] != 'y' && argv[2][1] != 'n' && argv[2][1] != 'c')) {
> > > +		printf("Usage: mmc enh_area setmax <-y|-n|-c> /dev/mmcX\n");
> > > +		return 1;
> > > +	}
> > 
> > I assume 'y' means 'yes', 'n' means 'no', but what does 'c' mean? It
> > doesn't seem to be implemented and none of these options is documented.
> 
> I'll rework to
> 
> 	enh_area setmax [-c] /dev/mmcX
> 
> and add some documentation.
> 
> > > +	if (argv[2][1] == 'y')
> > > +		set_completed = 1;
> > > +
> > > +	devname = argv[3];
> > > +	if (!strncmp(devname, "/dev/", 5))
> > > +		devname += 5;
> > > +
> > > +	mci = mci_get_device_by_name(devname);
> > > +	if (!mci) {
> > > +		printf("Failure to open %s as mci device\n", devname);
> > > +		return -ENOENT;
> > > +	}
> > 
> > This part is probably needed by all other future subcommands aswell.
> > Should it be an extra function?
> 
> Something like:
> 
> struct mci *mci_get_device_by_devname(const char *devname)
> {
> 	if (!strncmp(devname, "/dev/", 5))
> 		devname += 5;

devpath_to_name() instead.

> 
> 	return mci_get_device_by_name(devname);
> }
> 
> ?
> 
> > > +	if (!(ext_csd[EXT_CSD_PARTITIONING_SUPPORT] & EXT_CSD_ENH_ATTRIBUTE_EN_MASK)) {
> > > +		printf("Device doesn't support enhanced area\n");
> > > +		ret = -EIO;
> > > +		goto error;
> > > +	}
> > > +
> > > +	if (ext_csd[EXT_CSD_PARTITION_SETTING_COMPLETED]) {
> > > +		printf("Partitioning already finalized\n");
> > > +		ret = -EIO;
> > > +		goto error;
> > > +	}
> > > +
> > > +	ret = mci_switch(mci, EXT_CSD_ERASE_GROUP_DEF, 1);
> > > +	if (ret) {
> > > +		printf("Failure to write to EXT_CSD_ERASE_GROUP_DEF\n");
> > > +		goto error;
> > 
> > This command is *very* verbose in the error path. Would it be enough to
> > just write the register number of the failed access instead of the name?
> 
> It prints a single line if an error happens. I'd not say this is too
> verbose. Do you care about the output or the binary size?

The latter.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox