ons 2013-05-15 klockan 15:20 -0700 skrev Simon Glass:
> Hi Henrik,
>
> On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström
> <hen...@henriknordstrom.net> wrote:
> > Version 2 of this patch addressing the code comments by Simon and adding
> > some small missing pieces.
>
> Looks good.
>
> For change log, you should follow the standard approach - also instead
> of 'comments by Simon' it is good to list the things you changed. You
> might find patman useful for preparing and sending patches.
Just looked into it and looks nice. Giving it a try for next version.
Took a little while to get started, mostly because it crashes & burns
when not finding an matching alias for sandbox.
> blank line here, please fix globally (a blank line between
> declarations and code).
Ok.
> > +static int do_sandbox_info(cmd_tbl_t *cmdtp, int flag, int argc,
> > + char * const argv[])
> > +{
> > + if (argc < 1 || argc > 2)
> > + return CMD_RET_USAGE;
>
> Probably best to put this after the declarations
Ok. Also restrucured do_sandbox_bind a little to match this. There one
of the declarations depends on this being checked first.
> Move to top of function. Try to collect your declarations within a
> block when it's easy to do so.
Ok.
> > + printf("%3s %12s %s\n", "dev", "blocks", "path");
> > + for (dev = min_dev; dev <= max_dev; dev++) {
> > + printf("%3d ", dev);
> > + block_dev_desc_t *blk_dev = host_get_dev(dev);
> > + if (!blk_dev)
> > + continue;
>
> Does this case ever happen? If so you don't print \n.
Yes. And it then relies on the driver to print an error.
> > + struct host_block_dev *host_dev = blk_dev->priv;
>
> Maybe leave the assignment here but put the declaration at the start
> of the block.
Yes.
> > COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> > COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> > COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> > +COBJS-$(CONFIG_SANDBOX) += sandbox.o
>
> Alphabetic order so this should go up two.
up seven. sorted on filename here not CONFIG_..
> > +#include <sandboxblockdev.h>
>
> How about sandbox-blockdev.h
Yee.
> puts() when you don't have format args. Please fix globally.
Ok.
> > + if (host_dev->fd == -1) {
> > + printf("Failed to access host backing file '%s'\n",
> > + host_dev->filename);
> > + return 1;
>
> -ENOENT might be better (include errno.h)
or maybe just -errno?
and handle the error in do_sandbox_bind().
> > +int host_dev_bind(int dev, char *filename);
>
> Please add a comment as to what this function does, describing the
> meaning and valid values for each argument.
Ok.
> =>ext4load host 0:3
> Segmentation fault (core dumped)
Doesn't ext2load expect a address & filename to load?
Regads
Henrik
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
