Re: [PATCH 3/4] cmd: dtimg: Make <varname> an optional argument

Hi,

On Fri, Nov 29, 2019 at 9:30 PM Eugeniu Rosca erosca@de.adit-jv.com wrote:

Unlike dtimg, U-Boot commands like part [1], fstype [2] and uuid [3] accept an _optional_ <varname> parameter, which means that they will output the result to console whenever <varname> is skipped. This is extremely useful during development.

Allow "dtimg" to behave in a similar fashion [4]. In addition:

• replace env_set() by env_set_hex()

Thanks, didn't know that existed. I like it.

• track and report the failures of env_set_hex()

"grep -Ir env_set cmd/" shows nobody really cares to check env_set* error codes. Probably it's very unlikely that environment is broken at the point of commands execution?

• amend command's help/usage text

[1] => part start mmc 0 1 800 => part start mmc 0 1 myvar; print myvar myvar=800 [2] => fstype mmc 0:1 ext4 => fstype mmc 0:1 myvar; print myvar myvar=ext4 [3] => uuid b3909b50-55df-4173-b83c-b05343d2d5d2 => uuid myvar; print myvar myvar=4c04b15f-d0c1-4f98-9aca-ab62a66be864 [4] => dtimg start 0x48000000 0 0x480000e0 => dtimg start 0x48000000 0 myvar; print myvar myvar=480000e0

Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com

cmd/dtimg.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/cmd/dtimg.c b/cmd/dtimg.c index 5989081b0c14..5348a4ad46e8 100644 --- a/cmd/dtimg.c +++ b/cmd/dtimg.c @@ -55,9 +55,10 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd) char *endp; ulong fdt_addr; u32 fdt_size;

•   char buf[65];
  

•   ulong envval;
  

•   int ret;
  
  

•   if (argc != 4)
  

•   if (argc < 3)
            return CMD_RET_USAGE;
  
    if (dtimg_get_argv_addr(argv[1], &hdr_addr) != CMD_RET_SUCCESS)
  

@@ -74,17 +75,24 @@ static int dtimg_get_fdt(int argc, char * const argv[], enum cmd_dtimg_info cmd)

    switch (cmd) {
    case CMD_DTIMG_START:

•           snprintf(buf, sizeof(buf), "%lx", fdt_addr);
  

•           envval = fdt_addr;
            break;
    case CMD_DTIMG_SIZE:
  

•           snprintf(buf, sizeof(buf), "%x", fdt_size);
  

•           envval = fdt_size;
            break;
    default:
            printf("Error: Unknown cmd_dtimg_info value: %d\n", cmd);
            return CMD_RET_FAILURE;
    }
  
  

•   env_set(argv[3], buf);
  

•   if (argv[3]) {
  

•           ret = env_set_hex(argv[3], envval);
  

•           if (ret)
  

•                   printf("Error(%d) env-setting '%s=0x%lx'\n",
  

•                          ret, argv[3], envval);
  

•   } else {
  

•           printf("0x%lx\n", envval);
  

•   }
  
    return CMD_RET_SUCCESS;
  

} @@ -131,12 +139,12 @@ U_BOOT_CMD( "dump <addr>\n" " - parse specified image and print its structure info\n" " <addr>: image address in RAM, in hex\n"

•   "dtimg start <addr> <index> <varname>\n"
  

•   "dtimg start <addr> <index> [<varname>]\n"
  

Bikeshedding: maybe use just [varname]?

    "    - get address (hex) of FDT in the image, by index\n"
    "      <addr>: image address in RAM, in hex\n"
    "      <index>: index of desired FDT in the image\n"
    "      <varname>: name of variable where to store address of FDT\n"

•   "dtimg size <addr> <index> <varname>\n"
  

•   "dtimg size <addr> <index> [<varname>]\n"
    "    - get size (hex, bytes) of FDT in the image, by index\n"
    "      <addr>: image address in RAM, in hex\n"
    "      <index>: index of desired FDT in the image\n"
  

-- 2.24.0

Other than those minor comments:

Reviewed-by: Sam Protsenko semen.protsenko@linaro.org