Re: [PATCH v5 2/6] cmd: bcb: rework the command to U_BOOT_LONGHELP approach

Hi Dmitry,

Thank you for the patch.

On jeu., oct. 17, 2024 at 17:12, Dmitry Rokosov ddrokosov@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 ddrokosov@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 mkorpershoek@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