Hi again, [I would be willing to take this discussion offline, if you consider it too noisy for ML]
On Wed, Oct 23, 2019 at 05:34:22PM +0300, Sam Protsenko wrote: > +U_BOOT_CMD( > + bootimg, CONFIG_SYS_MAXARGS, 0, do_bootimg, > + "manipulate Android Boot Image", > + "set_addr <addr>\n" > + " - set the address in RAM where boot image is located\n" > + " ($loadaddr is used by default)\n" > + "bootimg ver <varname>\n" > + " - get header version\n" > + "bootimg get_dtbo <addr_var> [size_var]\n" > + " - get address and size (hex) of recovery DTBO area in the image\n" > + " <addr_var>: variable name to contain DTBO area address\n" > + " [size_var]: variable name to contain DTBO area size\n" > + "bootimg dtb_dump\n" > + " - print info for all files in DTB area\n" I now see that "DTBO area" is used intermixed with "DTB area". I think it makes sense to use one of the two consistently and drop the other. Otherwise, users might think there are two distinct areas in the same Android image. > + "bootimg dtb_load_addr <varname>\n" > + " - get load address (hex) of DTB\n" This is actually not something retrieved from the DTB/DTBO area, but from the "dtb_addr" field of the Android Image itself, as defined in https://android.googlesource.com/platform/system/tools/mkbootimg/+/refs/heads/master/include/bootimg/bootimg.h I think this little bit of information is essential, but not sure how to make it more transparent to the user, since purely based on the available help message, I tend to infer that there is connection between "DTB" in "get load address (hex) of DTB" and the "DTBO area", while there is no connection whatsoever. > + "bootimg get_dtb_file <index> <addr_var> [size_var]\n" > + " - get address and size (hex) of DTB file in the image\n" > + " <index>: index of desired DTB file in DTB area\n" > + " <addr_var>: variable name to contain DTB file address\n" > + " [size_var]: variable name to contain DTB file size\n" Would it make more sense to use "DTB entry" instead of "DTB file" since this is the wording used in the Google spec/header? Another general comment regarding the current sub-commands: - set_addr - ver - get_dtbo - dtb_dump - dtb_load_addr - get_dtb_file I observe the following (inconsistent) pattern: - <do>_<what> - <what> - <do>_<what> - <what>_<do> - <what>_<do>_<what> - <do>_<what> Looking at the "fdt" command, I find its sub-commands somehow better partitioned and easier to digest/remember: fdt addr [-c] <addr> [<length>] fdt apply <addr> fdt move <fdt> <newaddr> <length> fdt resize [<extrasize>] fdt print <path> [<prop>] fdt list <path> [<prop>] fdt get value <var> <path> <prop> fdt get name <var> <path> <index> fdt get addr <var> <path> <prop> fdt get size <var> <path> [<prop>] fdt set <path> <prop> [<val>] fdt mknode <path> <node> fdt rm <path> [<prop>] fdt header [get <var> <member>] Its syntax seems to be: <command> <do> <what> [options] Would it make sense to borrow this naming style/principle? It could translate to the following for abootimg: abootimg (current sub-command name enclosed in brackets): - addr (set_addr) - ver - dump dtbo (dtb_dump) - get dtbo (get_dtbo) - get dtbe (get_dtb_file) - get dtla (dtb_load_addr) > +); -- Best Regards, Eugeniu
