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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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