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