Hi Dmitry,
Thank you for the patch.
On jeu., oct. 17, 2024 at 17:12, Dmitry Rokosov <ddroko...@salutedevices.com>
wrote:
> U_BOOT_LONGHELP and U_BOOT_CMD_WITH_SUBCMDS offer numerous advantages,
> including:
> - common argument restrictions checking
> - automatic subcommand matching
> - improved usage and help handling
>
> By utilizing the U_BOOT_LONGHELP approach, we can reduce the amount of
> command management code and describe commands more succinctly.
>
> Signed-off-by: Dmitry Rokosov <ddroko...@salutedevices.com>
> ---
> cmd/bcb.c | 151
> ++++++++++++++++++--------------------------------------------
> 1 file changed, 43 insertions(+), 108 deletions(-)
Nice diffstat, always great to see code removed!
Reviewed-by: Mattijs Korpershoek <mkorpersh...@baylibre.com>
>
> diff --git a/cmd/bcb.c b/cmd/bcb.c
> index
> 97a96c009641cc094645607ef833575f3c03fe4b..98b2a71087a27b1721f4bed4160095d65ec75402
> 100644
> --- a/cmd/bcb.c
> +++ b/cmd/bcb.c
> @@ -16,15 +16,6 @@
> #include <vsprintf.h>
> #include <linux/err.h>
>
> -enum bcb_cmd {
> - BCB_CMD_LOAD,
> - BCB_CMD_FIELD_SET,
> - BCB_CMD_FIELD_CLEAR,
> - BCB_CMD_FIELD_TEST,
> - BCB_CMD_FIELD_DUMP,
> - BCB_CMD_STORE,
> -};
> -
> static const char * const fields[] = {
> "command",
> "status",
> @@ -38,67 +29,9 @@ static struct disk_partition partition_data;
> static struct blk_desc *block;
> static struct disk_partition *partition = &partition_data;
>
> -static int bcb_cmd_get(char *cmd)
> -{
> - if (!strcmp(cmd, "load"))
> - return BCB_CMD_LOAD;
> - if (!strcmp(cmd, "set"))
> - return BCB_CMD_FIELD_SET;
> - if (!strcmp(cmd, "clear"))
> - return BCB_CMD_FIELD_CLEAR;
> - if (!strcmp(cmd, "test"))
> - return BCB_CMD_FIELD_TEST;
> - if (!strcmp(cmd, "store"))
> - return BCB_CMD_STORE;
> - if (!strcmp(cmd, "dump"))
> - return BCB_CMD_FIELD_DUMP;
> - else
> - return -1;
> -}
> -
> -static int bcb_is_misused(int argc, char *const argv[])
> +static int bcb_not_loaded(void)
> {
> - int cmd = bcb_cmd_get(argv[0]);
> -
> - switch (cmd) {
> - case BCB_CMD_LOAD:
> - if (argc != 3 && argc != 4)
> - goto err;
> - break;
> - case BCB_CMD_FIELD_SET:
> - if (argc != 3)
> - goto err;
> - break;
> - case BCB_CMD_FIELD_TEST:
> - if (argc != 4)
> - goto err;
> - break;
> - case BCB_CMD_FIELD_CLEAR:
> - if (argc != 1 && argc != 2)
> - goto err;
> - break;
> - case BCB_CMD_STORE:
> - if (argc != 1)
> - goto err;
> - break;
> - case BCB_CMD_FIELD_DUMP:
> - if (argc != 2)
> - goto err;
> - break;
> - default:
> - printf("Error: 'bcb %s' not supported\n", argv[0]);
> - return -1;
> - }
> -
> - if (cmd != BCB_CMD_LOAD && !block) {
> - printf("Error: Please, load BCB first!\n");
> - return -1;
> - }
> -
> - return 0;
> -err:
> - printf("Error: Bad usage of 'bcb %s'\n", argv[0]);
> -
> + printf("Error: Please, load BCB first!\n");
> return -1;
> }
>
> @@ -216,6 +149,9 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag,
> int argc,
> char *endp;
> char *iface = "mmc";
>
> + if (argc < 3)
> + return CMD_RET_USAGE;
> +
> if (argc == 4) {
> iface = argv[1];
> argc--;
> @@ -270,6 +206,12 @@ static int __bcb_set(const char *fieldp, const char
> *valp)
> static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc,
> char * const argv[])
> {
> + if (argc < 3)
> + return CMD_RET_USAGE;
> +
> + if (!block)
> + return bcb_not_loaded();
> +
> return __bcb_set(argv[1], argv[2]);
> }
>
> @@ -279,6 +221,9 @@ static int do_bcb_clear(struct cmd_tbl *cmdtp, int flag,
> int argc,
> int size;
> char *field;
>
> + if (!block)
> + return bcb_not_loaded();
> +
> if (argc == 1) {
> memset(&bcb, 0, sizeof(bcb));
> return CMD_RET_SUCCESS;
> @@ -297,7 +242,15 @@ static int do_bcb_test(struct cmd_tbl *cmdtp, int flag,
> int argc,
> {
> int size;
> char *field;
> - char *op = argv[2];
> + char *op;
> +
> + if (argc < 4)
> + return CMD_RET_USAGE;
> +
> + if (!block)
> + return bcb_not_loaded();
> +
> + op = argv[2];
>
> if (bcb_field_get(argv[1], &field, &size))
> return CMD_RET_FAILURE;
> @@ -325,6 +278,12 @@ static int do_bcb_dump(struct cmd_tbl *cmdtp, int flag,
> int argc,
> int size;
> char *field;
>
> + if (argc < 2)
> + return CMD_RET_USAGE;
> +
> + if (!block)
> + return bcb_not_loaded();
> +
> if (bcb_field_get(argv[1], &field, &size))
> return CMD_RET_FAILURE;
>
> @@ -356,6 +315,9 @@ err:
> static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc,
> char * const argv[])
> {
> + if (!block)
> + return bcb_not_loaded();
> +
> return __bcb_store();
> }
>
> @@ -414,44 +376,7 @@ void bcb_reset(void)
> __bcb_reset();
> }
>
> -static struct cmd_tbl cmd_bcb_sub[] = {
> - U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""),
> - U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""),
> - U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""),
> - U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""),
> - U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""),
> - U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""),
> -};
> -
> -static int do_bcb(struct cmd_tbl *cmdtp, int flag, int argc, char *const
> argv[])
> -{
> - struct cmd_tbl *c;
> -
> - if (argc < 2)
> - return CMD_RET_USAGE;
> -
> - argc--;
> - argv++;
> -
> - c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub));
> - if (!c)
> - return CMD_RET_USAGE;
> -
> - if (bcb_is_misused(argc, argv)) {
> - /*
> - * We try to improve the user experience by reporting the
> - * root-cause of misusage, so don't return CMD_RET_USAGE,
> - * since the latter prints out the full-blown help text
> - */
> - return CMD_RET_FAILURE;
> - }
> -
> - return c->cmd(cmdtp, flag, argc, argv);
> -}
> -
> -U_BOOT_CMD(
> - bcb, CONFIG_SYS_MAXARGS, 1, do_bcb,
> - "Load/set/clear/test/dump/store Android BCB fields",
> +U_BOOT_LONGHELP(bcb,
> "load <interface> <dev> <part> - load BCB from <interface>
> <dev>:<part>\n"
> "load <dev> <part> - load BCB from mmc <dev>:<part>\n"
> "bcb set <field> <val> - set BCB <field> to <val>\n"
> @@ -472,3 +397,13 @@ U_BOOT_CMD(
> " NOTE: any ':' character in <val> will be replaced by
> line feed\n"
> " during 'bcb set' and used as separator by upper layers\n"
> );
> +
> +U_BOOT_CMD_WITH_SUBCMDS(bcb,
> + "Load/set/clear/test/dump/store Android BCB fields", bcb_help_text,
> + U_BOOT_SUBCMD_MKENT(load, 4, 1, do_bcb_load),
> + U_BOOT_SUBCMD_MKENT(set, 3, 1, do_bcb_set),
> + U_BOOT_SUBCMD_MKENT(clear, 2, 1, do_bcb_clear),
> + U_BOOT_SUBCMD_MKENT(test, 4, 1, do_bcb_test),
> + U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_bcb_dump),
> + U_BOOT_SUBCMD_MKENT(store, 1, 1, do_bcb_store),
> +);
>
> --
> 2.43.0
