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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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;

	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?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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