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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
