Hi Lukasz,
On Sat, Jul 25, 2015 at 3:11 AM, Lukasz Majewski <l.majew...@majess.pl> wrote:
> The "dfu" command has been extended to support transfers via TFTP protocol.
>
> Signed-off-by: Lukasz Majewski <l.majew...@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.majew...@majess.pl>
> + *
> * Copyright (C) 2012 Samsung Electronics
> * authors: Andrzej Pietrasiewicz <andrze...@samsung.com>
> * Lukasz Majewski <l.majew...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
