[U-Boot] [PATCH v2 7/9] dfu: command: Extend "dfu" command to handle receiving data via TFTP

[Joe Hershberger]

Hi Lukasz,

On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majewski at majess.pl> wrote:
> The "dfu" command has been extended to support transfers via TFTP protocol.
>
> Signed-off-by: Lukasz Majewski <l.majewski at majess.pl>
> ---
> Changes for v2:
> - Remove "dfutftp" command
> - Modify "dfu" command to support optional [tftp] parameter
> - Only one flag (CONFIG_DFU_TFTP) needs to be enabled
> ---
>  common/cmd_dfu.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/common/cmd_dfu.c b/common/cmd_dfu.c
> index 857148f..aea466b 100644
> --- a/common/cmd_dfu.c
> +++ b/common/cmd_dfu.c
> @@ -1,6 +1,9 @@
>  /*
>   * cmd_dfu.c -- dfu command
>   *
> + * Copyright (C) 2015
> + * Lukasz Majewski <l.majewski at majess.pl>
> + *
>   * Copyright (C) 2012 Samsung Electronics
>   * authors: Andrzej Pietrasiewicz <andrzej.p at samsung.com>
>   *         Lukasz Majewski <l.majewski at samsung.com>
> @@ -13,6 +16,9 @@
>  #include <dfu.h>
>  #include <g_dnl.h>
>  #include <usb.h>
> +#ifdef CONFIG_DFU_TFTP
> +#include <net.h>
> +#endif

Generally you shouldn't need to guard an include. Just include net.h
all the time.

>  static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> @@ -26,7 +32,18 @@ static int do_dfu(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>         char *devstring = argv[3];
>
>         int ret, i = 0;
> +#ifdef CONFIG_DFU_TFTP
> +       unsigned long addr = 0;

Maybe you should reinitialize devstring to NULL here?

> +       if (!strcmp(usb_controller, "tftp")) {

This would look a lot clearer if you used argv[1] instead of
usb_controller. You are not using it as the usb_controller parameter,
it just happens to be the second word.

> +               usb_controller = argv[2];

You shouldn't be keeping the usb_controller parameter around. It makes
no sense for the tftp case. Why not just drop it?

> +               interface = argv[3];
> +               devstring = argv[4];

Is it safe to read argv[4] on your optional parameter without checking
for argc >= 5?

> +               if (argc == 6)
> +                       addr = simple_strtoul(argv[5], NULL, 0);
>
> +               return update_tftp(addr, interface, devstring);
> +       }
> +#endif
>         ret = dfu_init_env_entities(interface, devstring);
>         if (ret)
>                 goto done;
> @@ -84,9 +101,17 @@ done:
>
>  U_BOOT_CMD(dfu, CONFIG_SYS_MAXARGS, 1, do_dfu,
>         "Device Firmware Upgrade",
> +#ifdef CONFIG_DFU_TFTP
> +       "[tftp] <USB_controller> <interface> <dev> [list] [addr]\n"

Also drop <USB_controller> from the help here.

> +#else
>         "<USB_controller> <interface> <dev> [list]\n"
> +#endif
>         "  - device firmware upgrade via <USB_controller>\n"
>         "    on device <dev>, attached to interface\n"
>         "    <interface>\n"
>         "    [list] - list available alt settings\n"
> +#ifdef CONFIG_DFU_TFTP
> +       "    [tftp] - use TFTP protocol to download data\n"
> +       "    [addr] - address where FIT image has been stored\n"

Probably not helpful to include the '[' and ']' here. They are
supposed to indicate optional parameters. We already know they are
optional from above.  Good to fix up the '[list]' as well.

> +#endif
>  );
> --
> 2.1.4
>