[U-Boot] [PATCH v4 0/6] cmd: Simplify support for sub-commands

[U-Boot] [PATCH v4 0/6] cmd: Simplify support for sub-commands

[Boris Brezillon]

Hello,

Here is the 4th version of the sub-cmd patchset fixing the maxargs def
of the mtd bad sub-cmd (I noticed the bug just after sending v3 :-/).

Regards,

Boris

Boris Brezillon (6):
  common: command: Fix command auto-completion
  common: command: Expose a generic helper to auto-complete sub commands
  common: command: Rework the 'cmd is repeatable' logic
  command: commands: Add macros to declare commands with subcmds
  cmd: mtd: Use the subcmd infrastructure to declare mtd sub-commands
  cmd: adc: Use the sub-command infrastructure

 cmd/adc.c         |  33 +---
 cmd/dtimg.c       |   2 +-
 cmd/help.c        |   2 +-
 cmd/mmc.c         |   4 +-
 cmd/mtd.c         | 476 +++++++++++++++++++++++++++-------------------
 common/command.c  |  68 ++++++-
 include/command.h | 133 ++++++++++++-
 7 files changed, 477 insertions(+), 241 deletions(-)

-- 
2.17.1

[U-Boot] [PATCH v4 1/6] common: command: Fix command auto-completion

[Boris Brezillon]

When auto-completing command arguments, the last argument is not
necessarily the one we need to auto-complete. When the last character is
a space, a tab or '\0' what we want instead is list all possible values,
or if there's only one possible value, place this value on the command
line instead of trying to suffix the last valid argument with missing
chars.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
-None

Changes in v3:
- Add Tom's R-b

Changes in v2:
- None
---
 common/command.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/common/command.c b/common/command.c
index 2433a89e0a8e..435824356b50 100644
--- a/common/command.c
+++ b/common/command.c
@@ -362,13 +362,21 @@ int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp)
 	sep = NULL;
 	seplen = 0;
 	if (i == 1) { /* one match; perfect */
-		k = strlen(argv[argc - 1]);
+		if (last_char != '\0' && !isblank(last_char))
+			k = strlen(argv[argc - 1]);
+		else
+			k = 0;
+
 		s = cmdv[0] + k;
 		len = strlen(s);
 		sep = " ";
 		seplen = 1;
 	} else if (i > 1 && (j = find_common_prefix(cmdv)) != 0) { /* more */
-		k = strlen(argv[argc - 1]);
+		if (last_char != '\0' && !isblank(last_char))
+			k = strlen(argv[argc - 1]);
+		else
+			k = 0;
+
 		j -= k;
 		if (j > 0) {
 			s = cmdv[0] + k;
-- 
2.17.1

[U-Boot] [PATCH v4 2/6] common: command: Expose a generic helper to auto-complete sub commands

[Boris Brezillon]

Some commands have a table of sub-commands. With minor adjustments,
complete_cmdv() is able to provide auto-completion for sub-commands
(it's just about passing the table of commands instead of taking the
global one).
We rename this function into complete_subcmd() and implement
complete_cmdv() as a wrapper around complete_subcmdv().

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
-None

Changes in v3:
- Add Tom's R-b

Changes in v2:
- None
---
 common/command.c  | 20 ++++++++++++++++----
 include/command.h |  3 +++
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/common/command.c b/common/command.c
index 435824356b50..e13cb47ac18b 100644
--- a/common/command.c
+++ b/common/command.c
@@ -161,11 +161,11 @@ int var_complete(int argc, char * const argv[], char last_char, int maxv, char *
 
 /*************************************************************************************/
 
-static int complete_cmdv(int argc, char * const argv[], char last_char, int maxv, char *cmdv[])
+int complete_subcmdv(cmd_tbl_t *cmdtp, int count, int argc,
+		     char * const argv[], char last_char,
+		     int maxv, char *cmdv[])
 {
 #ifdef CONFIG_CMDLINE
-	cmd_tbl_t *cmdtp = ll_entry_start(cmd_tbl_t, cmd);
-	const int count = ll_entry_count(cmd_tbl_t, cmd);
 	const cmd_tbl_t *cmdend = cmdtp + count;
 	const char *p;
 	int len, clen;
@@ -193,7 +193,7 @@ static int complete_cmdv(int argc, char * const argv[], char last_char, int maxv
 
 	/* more than one arg or one but the start of the next */
 	if (argc > 1 || last_char == '\0' || isblank(last_char)) {
-		cmdtp = find_cmd(argv[0]);
+		cmdtp = find_cmd_tbl(argv[0], cmdtp, count);
 		if (cmdtp == NULL || cmdtp->complete == NULL) {
 			cmdv[0] = NULL;
 			return 0;
@@ -238,6 +238,18 @@ static int complete_cmdv(int argc, char * const argv[], char last_char, int maxv
 #endif
 }
 
+static int complete_cmdv(int argc, char * const argv[], char last_char,
+			 int maxv, char *cmdv[])
+{
+#ifdef CONFIG_CMDLINE
+	return complete_subcmdv(ll_entry_start(cmd_tbl_t, cmd),
+				ll_entry_count(cmd_tbl_t, cmd), argc, argv,
+				last_char, maxv, cmdv);
+#else
+	return 0;
+#endif
+}
+
 static int make_argv(char *s, int argvsz, char *argv[])
 {
 	int argc = 0;
diff --git a/include/command.h b/include/command.h
index 200c7a5e9f4e..89efcecfa926 100644
--- a/include/command.h
+++ b/include/command.h
@@ -54,6 +54,9 @@ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
 	      flag, int argc, char * const argv[]);
 cmd_tbl_t *find_cmd(const char *cmd);
 cmd_tbl_t *find_cmd_tbl (const char *cmd, cmd_tbl_t *table, int table_len);
+int complete_subcmdv(cmd_tbl_t *cmdtp, int count, int argc,
+		     char * const argv[], char last_char, int maxv,
+		     char *cmdv[]);
 
 extern int cmd_usage(const cmd_tbl_t *cmdtp);
 
-- 
2.17.1

[U-Boot] [PATCH v4 3/6] common: command: Rework the 'cmd is repeatable' logic

[Boris Brezillon]

The repeatable property is currently attached to the main command and
sub-commands have no way to change the repeatable value (the
->repeatable field in sub-command entries is ignored).

Replace the ->repeatable field by an extended ->cmd() hook (called
->cmd_rep()) which takes a new int pointer to store the repeatable cap
of the command being executed.

With this trick, we can let sub-commands decide whether they are
repeatable or not.

We also patch mmc and dtimg who are testing the ->repeatable field
directly (they now use cmd_is_repeatable() instead), and fix the help
entry manually since it doesn't use the U_BOOT_CMD() macro.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
-None

Changes in v3:
- Add Tom's R-b

Changes in v2:
- New patch
---
 cmd/dtimg.c       |  2 +-
 cmd/help.c        |  2 +-
 cmd/mmc.c         |  4 ++--
 common/command.c  | 36 ++++++++++++++++++++++++++++----
 include/command.h | 52 +++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/cmd/dtimg.c b/cmd/dtimg.c
index 65c8d101b98e..ae7d82f26dd2 100644
--- a/cmd/dtimg.c
+++ b/cmd/dtimg.c
@@ -116,7 +116,7 @@ static int do_dtimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	if (!cp || argc > cp->maxargs)
 		return CMD_RET_USAGE;
-	if (flag == CMD_FLAG_REPEAT && !cp->repeatable)
+	if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp))
 		return CMD_RET_SUCCESS;
 
 	return cp->cmd(cmdtp, flag, argc, argv);
diff --git a/cmd/help.c b/cmd/help.c
index 503fa632e74e..fa2010c67eb1 100644
--- a/cmd/help.c
+++ b/cmd/help.c
@@ -29,7 +29,7 @@ U_BOOT_CMD(
 
 /* This does not use the U_BOOT_CMD macro as ? can't be used in symbol names */
 ll_entry_declare(cmd_tbl_t, question_mark, cmd) = {
-	"?",	CONFIG_SYS_MAXARGS,	1,	do_help,
+	"?",	CONFIG_SYS_MAXARGS, cmd_always_repeatable,	do_help,
 	"alias for 'help'",
 #ifdef  CONFIG_SYS_LONGHELP
 	""
diff --git a/cmd/mmc.c b/cmd/mmc.c
index 3920a1836a59..42e38900df12 100644
--- a/cmd/mmc.c
+++ b/cmd/mmc.c
@@ -247,7 +247,7 @@ static int do_mmcrpmb(cmd_tbl_t *cmdtp, int flag,
 
 	if (cp == NULL || argc > cp->maxargs)
 		return CMD_RET_USAGE;
-	if (flag == CMD_FLAG_REPEAT && !cp->repeatable)
+	if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp))
 		return CMD_RET_SUCCESS;
 
 	mmc = init_mmc_device(curr_device, false);
@@ -907,7 +907,7 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	if (cp == NULL || argc > cp->maxargs)
 		return CMD_RET_USAGE;
-	if (flag == CMD_FLAG_REPEAT && !cp->repeatable)
+	if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp))
 		return CMD_RET_SUCCESS;
 
 	if (curr_device < 0) {
diff --git a/common/command.c b/common/command.c
index e13cb47ac18b..19f0534a76ea 100644
--- a/common/command.c
+++ b/common/command.c
@@ -501,6 +501,30 @@ void fixup_cmdtable(cmd_tbl_t *cmdtp, int size)
 }
 #endif
 
+int cmd_always_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
+			  char * const argv[], int *repeatable)
+{
+	*repeatable = 1;
+
+	return cmdtp->cmd(cmdtp, flag, argc, argv);
+}
+
+int cmd_never_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
+			 char * const argv[], int *repeatable)
+{
+	*repeatable = 0;
+
+	return cmdtp->cmd(cmdtp, flag, argc, argv);
+}
+
+int cmd_discard_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
+			   char * const argv[])
+{
+	int repeatable;
+
+	return cmdtp->cmd_rep(cmdtp, flag, argc, argv, &repeatable);
+}
+
 /**
  * Call a command function. This should be the only route in U-Boot to call
  * a command, so that we can track whether we are waiting for input or
@@ -510,13 +534,15 @@ void fixup_cmdtable(cmd_tbl_t *cmdtp, int size)
  * @param flag		Some flags normally 0 (see CMD_FLAG_.. above)
  * @param argc		Number of arguments (arg 0 must be the command text)
  * @param argv		Arguments
+ * @param repeatable	Can the command be repeated
  * @return 0 if command succeeded, else non-zero (CMD_RET_...)
  */
-static int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+static int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
+		    int *repeatable)
 {
 	int result;
 
-	result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
+	result = cmdtp->cmd_rep(cmdtp, flag, argc, argv, repeatable);
 	if (result)
 		debug("Command failed, result=%d\n", result);
 	return result;
@@ -553,12 +579,14 @@ enum command_ret_t cmd_process(int flag, int argc, char * const argv[],
 
 	/* If OK so far, then do the command */
 	if (!rc) {
+		int newrep;
+
 		if (ticks)
 			*ticks = get_timer(0);
-		rc = cmd_call(cmdtp, flag, argc, argv);
+		rc = cmd_call(cmdtp, flag, argc, argv, &newrep);
 		if (ticks)
 			*ticks = get_timer(*ticks);
-		*repeatable &= cmdtp->repeatable;
+		*repeatable &= newrep;
 	}
 	if (rc == CMD_RET_USAGE)
 		rc = cmd_usage(cmdtp);
diff --git a/include/command.h b/include/command.h
index 89efcecfa926..bb93f022c514 100644
--- a/include/command.h
+++ b/include/command.h
@@ -29,7 +29,16 @@
 struct cmd_tbl_s {
 	char		*name;		/* Command Name			*/
 	int		maxargs;	/* maximum number of arguments	*/
-	int		repeatable;	/* autorepeat allowed?		*/
+					/*
+					 * Same as ->cmd() except the command
+					 * tells us if it can be repeated.
+					 * Replaces the old ->repeatable field
+					 * which was not able to make
+					 * repeatable property different for
+					 * the main command and sub-commands.
+					 */
+	int		(*cmd_rep)(struct cmd_tbl_s *cmd, int flags, int argc,
+				   char * const argv[], int *repeatable);
 					/* Implementation function	*/
 	int		(*cmd)(struct cmd_tbl_s *, int, int, char * const []);
 	char		*usage;		/* Usage message	(short)	*/
@@ -60,6 +69,19 @@ int complete_subcmdv(cmd_tbl_t *cmdtp, int count, int argc,
 
 extern int cmd_usage(const cmd_tbl_t *cmdtp);
 
+/* Dummy ->cmd and ->cmd_rep wrappers. */
+int cmd_always_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
+			  char * const argv[], int *repeatable);
+int cmd_never_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
+			 char * const argv[], int *repeatable);
+int cmd_discard_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
+			   char * const argv[]);
+
+static inline bool cmd_is_repeatable(cmd_tbl_t *cmdtp)
+{
+	return cmdtp->cmd_rep == cmd_always_repeatable;
+}
+
 #ifdef CONFIG_AUTO_COMPLETE
 extern int var_complete(int argc, char * const argv[], char last_char, int maxv, char *cmdv[]);
 extern int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp);
@@ -188,16 +210,28 @@ int board_run_command(const char *cmdline);
 #endif
 
 #ifdef CONFIG_CMDLINE
+#define U_BOOT_CMDREP_MKENT_COMPLETE(_name, _maxargs, _cmd_rep,		\
+				     _usage, _help, _comp)		\
+		{ #_name, _maxargs, _cmd_rep, cmd_discard_repeatable,	\
+		  _usage, _CMD_HELP(_help) _CMD_COMPLETE(_comp) }
+
 #define U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd,		\
 				_usage, _help, _comp)			\
-		{ #_name, _maxargs, _rep, _cmd, _usage,			\
-			_CMD_HELP(_help) _CMD_COMPLETE(_comp) }
+		{ #_name, _maxargs,					\
+		 _rep ? cmd_always_repeatable : cmd_never_repeatable,	\
+		 _cmd, _usage, _CMD_HELP(_help) _CMD_COMPLETE(_comp) }
 
 #define U_BOOT_CMD_COMPLETE(_name, _maxargs, _rep, _cmd, _usage, _help, _comp) \
 	ll_entry_declare(cmd_tbl_t, _name, cmd) =			\
 		U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd,	\
 						_usage, _help, _comp);
 
+#define U_BOOT_CMDREP_COMPLETE(_name, _maxargs, _cmd_rep, _usage,	\
+			       _help, _comp)				\
+	ll_entry_declare(cmd_tbl_t, _name, cmd) =			\
+		U_BOOT_CMDREP_MKENT_COMPLETE(_name, _maxargs, _cmd_rep,	\
+					     _usage, _help, _comp)
+
 #else
 #define U_BOOT_SUBCMD_START(name)	static cmd_tbl_t name[] = {};
 #define U_BOOT_SUBCMD_END
@@ -209,15 +243,25 @@ int board_run_command(const char *cmdline);
 			_cmd(NULL, 0, 0, NULL);				\
 		return 0;						\
 	}
+
+#define U_BOOT_CMDREP_MKENT_COMPLETE(_name, _maxargs, _cmd_rep,		\
+				     _usage, _help, _comp)		\
+		{ #_name, _maxargs, 0 ? _cmd_rep : NULL, NULL, _usage,	\
+			_CMD_HELP(_help) _CMD_COMPLETE(_comp) }
+
 #define U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd, _usage,	\
 				  _help, _comp)				\
-		{ #_name, _maxargs, _rep, 0 ? _cmd : NULL, _usage,	\
+		{ #_name, _maxargs, NULL, 0 ? _cmd : NULL, _usage,	\
 			_CMD_HELP(_help) _CMD_COMPLETE(_comp) }
 
 #define U_BOOT_CMD_COMPLETE(_name, _maxargs, _rep, _cmd, _usage, _help,	\
 			    _comp)				\
 	_CMD_REMOVE(sub_ ## _name, _cmd)
 
+#define U_BOOT_CMDREP_COMPLETE(_name, _maxargs, _cmd_rep, _usage,	\
+			       _help, _comp)				\
+	_CMD_REMOVE(sub_ ## _name, _cmd_rep)
+
 #endif /* CONFIG_CMDLINE */
 
 #define U_BOOT_CMD(_name, _maxargs, _rep, _cmd, _usage, _help)		\
-- 
2.17.1

[U-Boot] [PATCH v4 4/6] command: commands: Add macros to declare commands with subcmds

[Boris Brezillon]

Most cmd/xxx.c source files expose several commands through a single
entry point. Some of them are doing the sub-command parsing manually in
their do_<cmd>() function, others are declaring a table of sub-commands
and then use find_cmd_tbl() to delegate the request to the sub command
handler.

In either case, the amount of code to do that is not negligible and
repetitive, not to mention that almost no commands are implementing
the auto-completion hook, which means most u-boot commands lack
auto-completion.

Provide several macros to easily define commands exposing sub-commands.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
-None

Changes in v3:
- Add Tom's R-b
- Fix the commit message

Changes in v2:
- Adjust based on the changes done in the sub-command infra
---
 include/command.h | 78 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/include/command.h b/include/command.h
index bb93f022c514..461b17447c0d 100644
--- a/include/command.h
+++ b/include/command.h
@@ -209,6 +209,70 @@ int board_run_command(const char *cmdline);
 # define _CMD_HELP(x)
 #endif
 
+#ifdef CONFIG_NEEDS_MANUAL_RELOC
+#define U_BOOT_SUBCMDS_RELOC(_cmdname)					\
+	static void _cmdname##_subcmds_reloc(void)			\
+	{								\
+		static int relocated;					\
+									\
+		if (relocated)						\
+			return;						\
+									\
+		fixup_cmdtable(_cmdname##_subcmds,			\
+			       ARRAY_SIZE(_cmdname##_subcmds));		\
+		relocated = 1;						\
+	}
+#else
+#define U_BOOT_SUBCMDS_RELOC(_cmdname)					\
+	static void _cmdname##_subcmds_reloc(void) { }
+#endif
+
+#define U_BOOT_SUBCMDS_DO_CMD(_cmdname)					\
+	static int do_##_cmdname(cmd_tbl_t *cmdtp, int flag, int argc,	\
+				 char * const argv[], int *repeatable)	\
+	{								\
+		cmd_tbl_t *subcmd;					\
+									\
+		_cmdname##_subcmds_reloc();				\
+									\
+		/* We need at least the cmd and subcmd names. */	\
+		if (argc < 2 || argc > CONFIG_SYS_MAXARGS)		\
+			return CMD_RET_USAGE;				\
+									\
+		subcmd = find_cmd_tbl(argv[1], _cmdname##_subcmds,	\
+				      ARRAY_SIZE(_cmdname##_subcmds));	\
+		if (!subcmd || argc - 1 > subcmd->maxargs)		\
+			return CMD_RET_USAGE;				\
+									\
+		if (flag == CMD_FLAG_REPEAT &&				\
+		    !cmd_is_repeatable(subcmd))				\
+			return CMD_RET_SUCCESS;				\
+									\
+		return subcmd->cmd_rep(subcmd, flag, argc - 1,		\
+				       argv + 1, repeatable);		\
+	}
+
+#ifdef CONFIG_AUTO_COMPLETE
+#define U_BOOT_SUBCMDS_COMPLETE(_cmdname)				\
+	static int complete_##_cmdname(int argc, char * const argv[],	\
+				       char last_char, int maxv,	\
+				       char *cmdv[])			\
+	{								\
+		return complete_subcmdv(_cmdname##_subcmds,		\
+					ARRAY_SIZE(_cmdname##_subcmds),	\
+					argc - 1, argv + 1, last_char,	\
+					maxv, cmdv);			\
+	}
+#else
+#define U_BOOT_SUBCMDS_COMPLETE(_cmdname)
+#endif
+
+#define U_BOOT_SUBCMDS(_cmdname, ...)					\
+	static cmd_tbl_t _cmdname##_subcmds[] = { __VA_ARGS__ };	\
+	U_BOOT_SUBCMDS_RELOC(_cmdname)					\
+	U_BOOT_SUBCMDS_DO_CMD(_cmdname)					\
+	U_BOOT_SUBCMDS_COMPLETE(_cmdname)
+
 #ifdef CONFIG_CMDLINE
 #define U_BOOT_CMDREP_MKENT_COMPLETE(_name, _maxargs, _cmd_rep,		\
 				     _usage, _help, _comp)		\
@@ -271,4 +335,18 @@ int board_run_command(const char *cmdline);
 	U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd,		\
 					_usage, _help, NULL)
 
+#define U_BOOT_SUBCMD_MKENT_COMPLETE(_name, _maxargs, _rep, _do_cmd,	\
+				     _comp)				\
+	U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _do_cmd,	\
+				  "", "", _comp)
+
+#define U_BOOT_SUBCMD_MKENT(_name, _maxargs, _rep, _do_cmd)		\
+	U_BOOT_SUBCMD_MKENT_COMPLETE(_name, _maxargs, _rep, _do_cmd,	\
+				     NULL)
+
+#define U_BOOT_CMD_WITH_SUBCMDS(_name, _usage, _help, ...)		\
+	U_BOOT_SUBCMDS(_name, __VA_ARGS__)				\
+	U_BOOT_CMDREP_COMPLETE(_name, CONFIG_SYS_MAXARGS, do_##_name,	\
+			       _usage, _help, complete_##_name)
+
 #endif	/* __COMMAND_H */
-- 
2.17.1

[U-Boot] [PATCH v4 5/6] cmd: mtd: Use the subcmd infrastructure to declare mtd sub-commands

[Boris Brezillon]

It's way simpler this way, and we also gain auto-completion support for
free (MTD name auto-completion has been added with mtd_name_complete())

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
- Fix maxargs in mtd bad sub-cmd
- s/do_mtd_name_complete()/mtd_name_complete()/

Changes in v3:
- Add Tom's R-b

Changes in v2:
- Adjust based on changes done in the sub-cmd infra
---
 cmd/mtd.c | 476 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 281 insertions(+), 195 deletions(-)

diff --git a/cmd/mtd.c b/cmd/mtd.c
index 614222398467..cda702d18b16 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -15,6 +15,22 @@
 #include <mapmem.h>
 #include <mtd.h>
 
+#include <linux/ctype.h>
+
+static struct mtd_info *get_mtd_by_name(const char *name)
+{
+	struct mtd_info *mtd;
+
+	mtd_probe_devices();
+
+	mtd = get_mtd_device_nm(name);
+	if (IS_ERR_OR_NULL(mtd))
+		printf("MTD device %s not found, ret %ld\n", name,
+		       PTR_ERR(mtd));
+
+	return mtd;
+}
+
 static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
 {
 	do_div(len, mtd->writesize);
@@ -177,7 +193,8 @@ static bool mtd_oob_write_is_empty(struct mtd_oob_ops *op)
 	return true;
 }
 
-static int do_mtd_list(void)
+static int do_mtd_list(cmd_tbl_t *cmdtp, int flag, int argc,
+		       char * const argv[])
 {
 	struct mtd_info *mtd;
 	int dev_nb = 0;
@@ -221,229 +238,287 @@ static int mtd_special_write_oob(struct mtd_info *mtd, u64 off,
 	return ret;
 }
 
-static int do_mtd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+static int do_mtd_io(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
+	bool dump, read, raw, woob, write_empty_pages, has_pages = false;
+	u64 start_off, off, len, remaining, default_len;
+	struct mtd_oob_ops io_op = {};
+	uint user_addr = 0, npages;
+	const char *cmd = argv[0];
 	struct mtd_info *mtd;
-	const char *cmd;
-	char *mtd_name;
+	u32 oob_len;
+	u8 *buf;
+	int ret;
 
-	/* All MTD commands need at least two arguments */
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	/* Parse the command name and its optional suffixes */
-	cmd = argv[1];
-
-	/* List the MTD devices if that is what the user wants */
-	if (strcmp(cmd, "list") == 0)
-		return do_mtd_list();
-
-	/*
-	 * The remaining commands require also@least a device ID.
-	 * Check the selected device is valid. Ensure it is probed.
-	 */
-	if (argc < 3)
-		return CMD_RET_USAGE;
-
-	mtd_name = argv[2];
-	mtd_probe_devices();
-	mtd = get_mtd_device_nm(mtd_name);
-	if (IS_ERR_OR_NULL(mtd)) {
-		printf("MTD device %s not found, ret %ld\n",
-		       mtd_name, PTR_ERR(mtd));
+	mtd = get_mtd_by_name(argv[1]);
+	if (IS_ERR_OR_NULL(mtd))
 		return CMD_RET_FAILURE;
+
+	if (mtd->type == MTD_NANDFLASH || mtd->type == MTD_MLCNANDFLASH)
+		has_pages = true;
+
+	dump = !strncmp(cmd, "dump", 4);
+	read = dump || !strncmp(cmd, "read", 4);
+	raw = strstr(cmd, ".raw");
+	woob = strstr(cmd, ".oob");
+	write_empty_pages = !has_pages || strstr(cmd, ".dontskipff");
+
+	argc -= 2;
+	argv += 2;
+
+	if (!dump) {
+		if (!argc) {
+			ret = CMD_RET_USAGE;
+			goto out_put_mtd;
+		}
+
+		user_addr = simple_strtoul(argv[0], NULL, 16);
+		argc--;
+		argv++;
 	}
-	put_mtd_device(mtd);
 
-	argc -= 3;
-	argv += 3;
+	start_off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
+	if (!mtd_is_aligned_with_min_io_size(mtd, start_off)) {
+		printf("Offset not aligned with a page (0x%x)\n",
+		       mtd->writesize);
+		ret = CMD_RET_FAILURE;
+		goto out_put_mtd;
+	}
 
-	/* Do the parsing */
-	if (!strncmp(cmd, "read", 4) || !strncmp(cmd, "dump", 4) ||
-	    !strncmp(cmd, "write", 5)) {
-		bool has_pages = mtd->type == MTD_NANDFLASH ||
-				 mtd->type == MTD_MLCNANDFLASH;
-		bool dump, read, raw, woob, write_empty_pages;
-		struct mtd_oob_ops io_op = {};
-		uint user_addr = 0, npages;
-		u64 start_off, off, len, remaining, default_len;
-		u32 oob_len;
-		u8 *buf;
-		int ret;
+	default_len = dump ? mtd->writesize : mtd->size;
+	len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : default_len;
+	if (!mtd_is_aligned_with_min_io_size(mtd, len)) {
+		len = round_up(len, mtd->writesize);
+		printf("Size not on a page boundary (0x%x), rounding to 0x%llx\n",
+		       mtd->writesize, len);
+	}
 
-		dump = !strncmp(cmd, "dump", 4);
-		read = dump || !strncmp(cmd, "read", 4);
-		raw = strstr(cmd, ".raw");
-		woob = strstr(cmd, ".oob");
-		write_empty_pages = !has_pages || strstr(cmd, ".dontskipff");
+	remaining = len;
+	npages = mtd_len_to_pages(mtd, len);
+	oob_len = woob ? npages * mtd->oobsize : 0;
 
-		if (!dump) {
-			if (!argc)
-				return CMD_RET_USAGE;
+	if (dump)
+		buf = kmalloc(len + oob_len, GFP_KERNEL);
+	else
+		buf = map_sysmem(user_addr, 0);
 
-			user_addr = simple_strtoul(argv[0], NULL, 16);
-			argc--;
-			argv++;
-		}
+	if (!buf) {
+		printf("Could not map/allocate the user buffer\n");
+		ret = CMD_RET_FAILURE;
+		goto out_put_mtd;
+	}
 
-		start_off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
-		if (!mtd_is_aligned_with_min_io_size(mtd, start_off)) {
-			printf("Offset not aligned with a page (0x%x)\n",
-			       mtd->writesize);
-			return CMD_RET_FAILURE;
-		}
+	if (has_pages)
+		printf("%s %lld byte(s) (%d page(s)) at offset 0x%08llx%s%s%s\n",
+		       read ? "Reading" : "Writing", len, npages, start_off,
+		       raw ? " [raw]" : "", woob ? " [oob]" : "",
+		       !read && write_empty_pages ? " [dontskipff]" : "");
+	else
+		printf("%s %lld byte(s) at offset 0x%08llx\n",
+		       read ? "Reading" : "Writing", len, start_off);
 
-		default_len = dump ? mtd->writesize : mtd->size;
-		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) :
-				 default_len;
-		if (!mtd_is_aligned_with_min_io_size(mtd, len)) {
-			len = round_up(len, mtd->writesize);
-			printf("Size not on a page boundary (0x%x), rounding to 0x%llx\n",
-			       mtd->writesize, len);
-		}
+	io_op.mode = raw ? MTD_OPS_RAW : MTD_OPS_AUTO_OOB;
+	io_op.len = has_pages ? mtd->writesize : len;
+	io_op.ooblen = woob ? mtd->oobsize : 0;
+	io_op.datbuf = buf;
+	io_op.oobbuf = woob ? &buf[len] : NULL;
 
-		remaining = len;
-		npages = mtd_len_to_pages(mtd, len);
-		oob_len = woob ? npages * mtd->oobsize : 0;
+	/* Search for the first good block after the given offset */
+	off = start_off;
+	while (mtd_block_isbad(mtd, off))
+		off += mtd->erasesize;
 
-		if (dump)
-			buf = kmalloc(len + oob_len, GFP_KERNEL);
-		else
-			buf = map_sysmem(user_addr, 0);
-
-		if (!buf) {
-			printf("Could not map/allocate the user buffer\n");
-			return CMD_RET_FAILURE;
-		}
-
-		if (has_pages)
-			printf("%s %lld byte(s) (%d page(s)) at offset 0x%08llx%s%s%s\n",
-			       read ? "Reading" : "Writing", len, npages, start_off,
-			       raw ? " [raw]" : "", woob ? " [oob]" : "",
-			       !read && write_empty_pages ? " [dontskipff]" : "");
-		else
-			printf("%s %lld byte(s) at offset 0x%08llx\n",
-			       read ? "Reading" : "Writing", len, start_off);
-
-		io_op.mode = raw ? MTD_OPS_RAW : MTD_OPS_AUTO_OOB;
-		io_op.len = has_pages ? mtd->writesize : len;
-		io_op.ooblen = woob ? mtd->oobsize : 0;
-		io_op.datbuf = buf;
-		io_op.oobbuf = woob ? &buf[len] : NULL;
-
-		/* Search for the first good block after the given offset */
-		off = start_off;
-		while (mtd_block_isbad(mtd, off))
+	/* Loop over the pages to do the actual read/write */
+	while (remaining) {
+		/* Skip the block if it is bad */
+		if (mtd_is_aligned_with_block_size(mtd, off) &&
+		    mtd_block_isbad(mtd, off)) {
 			off += mtd->erasesize;
-
-		/* Loop over the pages to do the actual read/write */
-		while (remaining) {
-			/* Skip the block if it is bad */
-			if (mtd_is_aligned_with_block_size(mtd, off) &&
-			    mtd_block_isbad(mtd, off)) {
-				off += mtd->erasesize;
-				continue;
-			}
-
-			if (read)
-				ret = mtd_read_oob(mtd, off, &io_op);
-			else
-				ret = mtd_special_write_oob(mtd, off, &io_op,
-							    write_empty_pages,
-							    woob);
-
-			if (ret) {
-				printf("Failure while %s at offset 0x%llx\n",
-				       read ? "reading" : "writing", off);
-				return CMD_RET_FAILURE;
-			}
-
-			off += io_op.retlen;
-			remaining -= io_op.retlen;
-			io_op.datbuf += io_op.retlen;
-			io_op.oobbuf += io_op.oobretlen;
+			continue;
 		}
 
-		if (!ret && dump)
-			mtd_dump_device_buf(mtd, start_off, buf, len, woob);
-
-		if (dump)
-			kfree(buf);
+		if (read)
+			ret = mtd_read_oob(mtd, off, &io_op);
 		else
-			unmap_sysmem(buf);
+			ret = mtd_special_write_oob(mtd, off, &io_op,
+						    write_empty_pages, woob);
 
 		if (ret) {
-			printf("%s on %s failed with error %d\n",
-			       read ? "Read" : "Write", mtd->name, ret);
-			return CMD_RET_FAILURE;
+			printf("Failure while %s at offset 0x%llx\n",
+			       read ? "reading" : "writing", off);
+			break;
 		}
 
-	} else if (!strcmp(cmd, "erase")) {
-		bool scrub = strstr(cmd, ".dontskipbad");
-		struct erase_info erase_op = {};
-		u64 off, len;
-		int ret;
-
-		off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
-		len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->size;
-
-		if (!mtd_is_aligned_with_block_size(mtd, off)) {
-			printf("Offset not aligned with a block (0x%x)\n",
-			       mtd->erasesize);
-			return CMD_RET_FAILURE;
-		}
-
-		if (!mtd_is_aligned_with_block_size(mtd, len)) {
-			printf("Size not a multiple of a block (0x%x)\n",
-			       mtd->erasesize);
-			return CMD_RET_FAILURE;
-		}
-
-		printf("Erasing 0x%08llx ... 0x%08llx (%d eraseblock(s))\n",
-		       off, off + len - 1, mtd_div_by_eb(len, mtd));
-
-		erase_op.mtd = mtd;
-		erase_op.addr = off;
-		erase_op.len = len;
-		erase_op.scrub = scrub;
-
-		while (erase_op.len) {
-			ret = mtd_erase(mtd, &erase_op);
-
-			/* Abort if its not a bad block error */
-			if (ret != -EIO)
-				break;
-
-			printf("Skipping bad block at 0x%08llx\n",
-			       erase_op.fail_addr);
-
-			/* Skip bad block and continue behind it */
-			erase_op.len -= erase_op.fail_addr - erase_op.addr;
-			erase_op.len -= mtd->erasesize;
-			erase_op.addr = erase_op.fail_addr + mtd->erasesize;
-		}
-
-		if (ret && ret != -EIO)
-			return CMD_RET_FAILURE;
-	} else if (!strcmp(cmd, "bad")) {
-		loff_t off;
-
-		if (!mtd_can_have_bb(mtd)) {
-			printf("Only NAND-based devices can have bad blocks\n");
-			return CMD_RET_SUCCESS;
-		}
-
-		printf("MTD device %s bad blocks list:\n", mtd->name);
-		for (off = 0; off < mtd->size; off += mtd->erasesize)
-			if (mtd_block_isbad(mtd, off))
-				printf("\t0x%08llx\n", off);
-	} else {
-		return CMD_RET_USAGE;
+		off += io_op.retlen;
+		remaining -= io_op.retlen;
+		io_op.datbuf += io_op.retlen;
+		io_op.oobbuf += io_op.oobretlen;
 	}
 
+	if (!ret && dump)
+		mtd_dump_device_buf(mtd, start_off, buf, len, woob);
+
+	if (dump)
+		kfree(buf);
+	else
+		unmap_sysmem(buf);
+
+	if (ret) {
+		printf("%s on %s failed with error %d\n",
+		       read ? "Read" : "Write", mtd->name, ret);
+		ret = CMD_RET_FAILURE;
+	} else {
+		ret = CMD_RET_SUCCESS;
+	}
+
+out_put_mtd:
+	put_mtd_device(mtd);
+
+	return ret;
+}
+
+static int do_mtd_erase(cmd_tbl_t *cmdtp, int flag, int argc,
+			char * const argv[])
+{
+	struct erase_info erase_op = {};
+	struct mtd_info *mtd;
+	u64 off, len;
+	bool scrub;
+	int ret;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	mtd = get_mtd_by_name(argv[1]);
+	if (IS_ERR_OR_NULL(mtd))
+		return CMD_RET_FAILURE;
+
+	scrub = strstr(argv[0], ".dontskipbad");
+
+	argc -= 2;
+	argv += 2;
+
+	off = argc > 0 ? simple_strtoul(argv[0], NULL, 16) : 0;
+	len = argc > 1 ? simple_strtoul(argv[1], NULL, 16) : mtd->size;
+
+	if (!mtd_is_aligned_with_block_size(mtd, off)) {
+		printf("Offset not aligned with a block (0x%x)\n",
+		       mtd->erasesize);
+		ret = CMD_RET_FAILURE;
+		goto out_put_mtd;
+	}
+
+	if (!mtd_is_aligned_with_block_size(mtd, len)) {
+		printf("Size not a multiple of a block (0x%x)\n",
+		       mtd->erasesize);
+		ret = CMD_RET_FAILURE;
+		goto out_put_mtd;
+	}
+
+	printf("Erasing 0x%08llx ... 0x%08llx (%d eraseblock(s))\n",
+	       off, off + len - 1, mtd_div_by_eb(len, mtd));
+
+	erase_op.mtd = mtd;
+	erase_op.addr = off;
+	erase_op.len = len;
+	erase_op.scrub = scrub;
+
+	while (erase_op.len) {
+		ret = mtd_erase(mtd, &erase_op);
+
+		/* Abort if its not a bad block error */
+		if (ret != -EIO)
+			break;
+
+		printf("Skipping bad block at 0x%08llx\n", erase_op.fail_addr);
+
+		/* Skip bad block and continue behind it */
+		erase_op.len -= erase_op.fail_addr - erase_op.addr;
+		erase_op.len -= mtd->erasesize;
+		erase_op.addr = erase_op.fail_addr + mtd->erasesize;
+	}
+
+	if (ret && ret != -EIO)
+		ret = CMD_RET_FAILURE;
+	else
+		ret = CMD_RET_SUCCESS;
+
+out_put_mtd:
+	put_mtd_device(mtd);
+
+	return ret;
+}
+
+static int do_mtd_bad(cmd_tbl_t *cmdtp, int flag, int argc,
+		      char * const argv[])
+{
+	struct mtd_info *mtd;
+	loff_t off;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	mtd = get_mtd_by_name(argv[1]);
+	if (IS_ERR_OR_NULL(mtd))
+		return CMD_RET_FAILURE;
+
+	if (!mtd_can_have_bb(mtd)) {
+		printf("Only NAND-based devices can have bad blocks\n");
+		goto out_put_mtd;
+	}
+
+	printf("MTD device %s bad blocks list:\n", mtd->name);
+	for (off = 0; off < mtd->size; off += mtd->erasesize) {
+		if (mtd_block_isbad(mtd, off))
+			printf("\t0x%08llx\n", off);
+	}
+
+out_put_mtd:
+	put_mtd_device(mtd);
+
 	return CMD_RET_SUCCESS;
 }
 
+#ifdef CONFIG_AUTO_COMPLETE
+static int mtd_name_complete(int argc, char * const argv[], char last_char,
+			     int maxv, char *cmdv[])
+{
+	int len = 0, n_found = 0;
+	struct mtd_info *mtd;
+
+	argc--;
+	argv++;
+
+	if (argc > 1 ||
+	    (argc == 1 && (last_char == '\0' || isblank(last_char))))
+		return 0;
+
+	if (argc)
+		len = strlen(argv[0]);
+
+	mtd_for_each_device(mtd) {
+		if (argc &&
+		    (len > strlen(mtd->name) ||
+		     strncmp(argv[0], mtd->name, len)))
+			continue;
+
+		if (n_found >= maxv - 2) {
+			cmdv[n_found++] = "...";
+			break;
+		}
+
+		cmdv[n_found++] = mtd->name;
+	}
+
+	cmdv[n_found] = NULL;
+
+	return n_found;
+}
+#endif /* CONFIG_AUTO_COMPLETE */
+
 static char mtd_help_text[] =
 #ifdef CONFIG_SYS_LONGHELP
 	"- generic operations on memory technology devices\n\n"
@@ -470,4 +545,15 @@ static char mtd_help_text[] =
 #endif
 	"";
 
-U_BOOT_CMD(mtd, 10, 1, do_mtd, "MTD utils", mtd_help_text);
+U_BOOT_CMD_WITH_SUBCMDS(mtd, "MTD utils", mtd_help_text,
+		U_BOOT_SUBCMD_MKENT(list, 1, 1, do_mtd_list),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(read, 5, 0, do_mtd_io,
+					     mtd_name_complete),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(write, 5, 0, do_mtd_io,
+					     mtd_name_complete),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(dump, 4, 0, do_mtd_io,
+					     mtd_name_complete),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(erase, 4, 0, do_mtd_erase,
+					     mtd_name_complete),
+		U_BOOT_SUBCMD_MKENT_COMPLETE(bad, 2, 1, do_mtd_bad,
+					     mtd_name_complete));
-- 
2.17.1

[U-Boot] [PATCH v4 6/6] cmd: adc: Use the sub-command infrastructure

[Boris Brezillon]

And you get sub-command auto-completion for free.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Tom Rini <trini@konsulko.com>
---
Changes in v4:
-None

Changes in v3:
- Add Tom's R-b

Changes in v2:
- New patch
---
 cmd/adc.c | 33 +++++----------------------------
 1 file changed, 5 insertions(+), 28 deletions(-)

diff --git a/cmd/adc.c b/cmd/adc.c
index 2d635acbd950..381961cf51a4 100644
--- a/cmd/adc.c
+++ b/cmd/adc.c
@@ -146,37 +146,14 @@ static int do_adc_scan(cmd_tbl_t *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
-static cmd_tbl_t cmd_adc_sub[] = {
-	U_BOOT_CMD_MKENT(list, 1, 1, do_adc_list, "", ""),
-	U_BOOT_CMD_MKENT(info, 2, 1, do_adc_info, "", ""),
-	U_BOOT_CMD_MKENT(single, 3, 1, do_adc_single, "", ""),
-	U_BOOT_CMD_MKENT(scan, 3, 1, do_adc_scan, "", ""),
-};
-
-static int do_adc(cmd_tbl_t *cmdtp, int flag, int argc,
-		  char *const argv[])
-{
-	cmd_tbl_t *c;
-
-	if (argc < 2)
-		return CMD_RET_USAGE;
-
-	/* Strip off leading 'adc' command argument */
-	argc--;
-	argv++;
-
-	c = find_cmd_tbl(argv[0], &cmd_adc_sub[0], ARRAY_SIZE(cmd_adc_sub));
-
-	if (c)
-		return c->cmd(cmdtp, flag, argc, argv);
-	else
-		return CMD_RET_USAGE;
-}
-
 static char adc_help_text[] =
 	"list - list ADC devices\n"
 	"adc info <name> - Get ADC device info\n"
 	"adc single <name> <channel> - Get Single data of ADC device channel\n"
 	"adc scan <name> [channel mask] - Scan all [or masked] ADC channels";
 
-U_BOOT_CMD(adc, 4, 1, do_adc, "ADC sub-system", adc_help_text);
+U_BOOT_CMD_WITH_SUBCMDS(adc, "ADC sub-system", adc_help_text,
+	U_BOOT_SUBCMD_MKENT(list, 1, 1, do_adc_list),
+	U_BOOT_SUBCMD_MKENT(info, 2, 1, do_adc_info),
+	U_BOOT_SUBCMD_MKENT(single, 3, 1, do_adc_single),
+	U_BOOT_SUBCMD_MKENT(scan, 3, 1, do_adc_scan));
-- 
2.17.1

[U-Boot] [PATCH v4 1/6] common: command: Fix command auto-completion

[Simon Glass]

Hi Boris,

On Mon, 3 Dec 2018 at 14:54, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> When auto-completing command arguments, the last argument is not
> necessarily the one we need to auto-complete. When the last character is
> a space, a tab or '\0' what we want instead is list all possible values,
> or if there's only one possible value, place this value on the command
> line instead of trying to suffix the last valid argument with missing
> chars.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> ---
> Changes in v4:
> -None
>
> Changes in v3:
> - Add Tom's R-b
>
> Changes in v2:
> - None
> ---
>  common/command.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

I wonder if you might be able to add a test for this into test/py/tests ?

It seems useful to ensure it doesn't break in future.

Regards,
Simon

[U-Boot] [PATCH v4 1/6] common: command: Fix command auto-completion

[Boris Brezillon]

Hi Simon,

On Mon, 10 Dec 2018 18:04:20 -0700
Simon Glass <sjg@chromium.org> wrote:

> Hi Boris,
> 
> On Mon, 3 Dec 2018 at 14:54, Boris Brezillon
> <boris.brezillon@bootlin.com> wrote:
> >
> > When auto-completing command arguments, the last argument is not
> > necessarily the one we need to auto-complete. When the last character is
> > a space, a tab or '\0' what we want instead is list all possible values,
> > or if there's only one possible value, place this value on the command
> > line instead of trying to suffix the last valid argument with missing
> > chars.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > Reviewed-by: Tom Rini <trini@konsulko.com>
> > ---
> > Changes in v4:
> > -None
> >
> > Changes in v3:
> > - Add Tom's R-b
> >
> > Changes in v2:
> > - None
> > ---
> >  common/command.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)  
> 
> I wonder if you might be able to add a test for this into test/py/tests ?

This is what I made, but I'm really bad at writing python scripts, so
don't hesitate to propose propose something else (especially for the
autocomp_command() method).

Regards,

Boris

--->8---
diff --git a/test/py/tests/test_autocomp.py b/test/py/tests/test_autocomp.py
new file mode 100644
index 000000000000..cb93cf7e8a87
--- /dev/null
+++ b/test/py/tests/test_autocomp.py
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Bootlin
+
+import pytest
+import u_boot_utils
+
+ at pytest.mark.buildconfigspec('auto_complete', 'cmd_memory', 'cmd_importenv')
+def test_env_print_autocomp(u_boot_console):
+    """Test that env print auto-completion works as expected."""
+
+    ram_base = u_boot_utils.find_ram_base(u_boot_console)
+    addr = '%08x' % ram_base
+    u_boot_console.run_command('mw.l ' + addr + ' 0x0')
+    u_boot_console.run_command('env import -d -b ' + addr)
+    u_boot_console.run_command('env set foo bar')
+    expected_response = 'foo'
+    response = u_boot_console.autocomp_command('printenv ', 'foo')
+    assert(expected_response in response)
diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
index 326b2ac51fbf..b8a2d0c84cda 100644
--- a/test/py/u_boot_console_base.py
+++ b/test/py/u_boot_console_base.py
@@ -137,6 +137,53 @@ class ConsoleBase(object):
             self.p.close()
         self.logstream.close()
 
+    def autocomp_command(self, cmd, expect):
+        """Try to auto-complete a command
+
+        The command is not executed, we just try to auto-complete it by
+        appending a \t at the end.
+
+        Args:
+            cmd: The command to auto-complete.
+            expect: The expected auto-completion.
+        """
+
+        if self.at_prompt and \
+                self.at_prompt_logevt != self.logstream.logfile.cur_evt:
+            self.logstream.write(self.prompt, implicit=True)
+
+        try:
+            self.at_prompt = False
+            cmd += '\t'
+            while cmd:
+                # Limit max outstanding data, so UART FIFOs don't overflow
+                chunk = cmd[:self.max_fifo_fill]
+                cmd = cmd[self.max_fifo_fill:]
+                self.p.send(chunk)
+                escchunk = chunk.replace('\t', '')
+                m = self.p.expect([escchunk] + self.bad_patterns)
+                if m != 0:
+                    self.at_prompt = False
+                    raise Exception('Bad pattern found on console: ' +
+                                    self.bad_pattern_ids[m - 1])
+
+            m = self.p.expect([expect])
+            if m != 0:
+                self.at_prompt = False
+                raise Exception('Bad pattern found on console: ' +
+                                self.bad_pattern_ids[m - 1])
+            self.at_prompt = True
+            self.at_prompt_logevt = self.logstream.logfile.cur_evt
+            # Only strip \r\n; space/TAB might be significant if testing
+            # indentation.
+            return self.p.after.strip('\r\n')
+        except Exception as ex:
+            self.log.error(str(ex))
+            self.cleanup_spawn()
+            raise
+        finally:
+            self.log.timestamp()
+
     def run_command(self, cmd, wait_for_echo=True, send_nl=True,
             wait_for_prompt=True):
         """Execute a command via the U-Boot console.

[U-Boot] [PATCH v4 1/6] common: command: Fix command auto-completion

[Simon Glass]

On Tue, 11 Dec 2018 at 04:24, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
>
> Hi Simon,
>
> On Mon, 10 Dec 2018 18:04:20 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Boris,
> >
> > On Mon, 3 Dec 2018 at 14:54, Boris Brezillon
> > <boris.brezillon@bootlin.com> wrote:
> > >
> > > When auto-completing command arguments, the last argument is not
> > > necessarily the one we need to auto-complete. When the last character is
> > > a space, a tab or '\0' what we want instead is list all possible values,
> > > or if there's only one possible value, place this value on the command
> > > line instead of trying to suffix the last valid argument with missing
> > > chars.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > ---
> > > Changes in v4:
> > > -None
> > >
> > > Changes in v3:
> > > - Add Tom's R-b
> > >
> > > Changes in v2:
> > > - None
> > > ---
> > >  common/command.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > I wonder if you might be able to add a test for this into test/py/tests ?
>
> This is what I made, but I'm really bad at writing python scripts, so
> don't hesitate to propose propose something else (especially for the
> autocomp_command() method).

That looks about right to me.

Can you send a separate patch, and cc Stephen Warren?

Regards,
Simon

[U-Boot] [U-Boot, v4, 3/6] common: command: Rework the 'cmd is repeatable' logic

[Tom Rini]

On Mon, Dec 03, 2018 at 10:54:20PM +0100, Boris Brezillon wrote:
> The repeatable property is currently attached to the main command and
> sub-commands have no way to change the repeatable value (the
> ->repeatable field in sub-command entries is ignored).
> 
> Replace the ->repeatable field by an extended ->cmd() hook (called
> ->cmd_rep()) which takes a new int pointer to store the repeatable cap
> of the command being executed.
> 
> With this trick, we can let sub-commands decide whether they are
> repeatable or not.
> 
> We also patch mmc and dtimg who are testing the ->repeatable field
> directly (they now use cmd_is_repeatable() instead), and fix the help
> entry manually since it doesn't use the U_BOOT_CMD() macro.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> ---
> Changes in v4:
> -None
> 
> Changes in v3:
> - Add Tom's R-b
> 
> Changes in v2:
> - New patch
> ---
>  cmd/dtimg.c       |  2 +-
>  cmd/help.c        |  2 +-
>  cmd/mmc.c         |  4 ++--
>  common/command.c  | 36 ++++++++++++++++++++++++++++----
>  include/command.h | 52 +++++++++++++++++++++++++++++++++++++++++++----
>  5 files changed, 84 insertions(+), 12 deletions(-)
> 
> diff --git a/cmd/dtimg.c b/cmd/dtimg.c
> index 65c8d101b98e..ae7d82f26dd2 100644
> --- a/cmd/dtimg.c
> +++ b/cmd/dtimg.c
> @@ -116,7 +116,7 @@ static int do_dtimg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  
>  	if (!cp || argc > cp->maxargs)
>  		return CMD_RET_USAGE;
> -	if (flag == CMD_FLAG_REPEAT && !cp->repeatable)
> +	if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp))
>  		return CMD_RET_SUCCESS;
>  
>  	return cp->cmd(cmdtp, flag, argc, argv);
> diff --git a/cmd/help.c b/cmd/help.c
> index 503fa632e74e..fa2010c67eb1 100644
> --- a/cmd/help.c
> +++ b/cmd/help.c
> @@ -29,7 +29,7 @@ U_BOOT_CMD(
>  
>  /* This does not use the U_BOOT_CMD macro as ? can't be used in symbol names */
>  ll_entry_declare(cmd_tbl_t, question_mark, cmd) = {
> -	"?",	CONFIG_SYS_MAXARGS,	1,	do_help,
> +	"?",	CONFIG_SYS_MAXARGS, cmd_always_repeatable,	do_help,
>  	"alias for 'help'",
>  #ifdef  CONFIG_SYS_LONGHELP
>  	""
> diff --git a/cmd/mmc.c b/cmd/mmc.c
> index 3920a1836a59..42e38900df12 100644
> --- a/cmd/mmc.c
> +++ b/cmd/mmc.c
> @@ -247,7 +247,7 @@ static int do_mmcrpmb(cmd_tbl_t *cmdtp, int flag,
>  
>  	if (cp == NULL || argc > cp->maxargs)
>  		return CMD_RET_USAGE;
> -	if (flag == CMD_FLAG_REPEAT && !cp->repeatable)
> +	if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp))
>  		return CMD_RET_SUCCESS;
>  
>  	mmc = init_mmc_device(curr_device, false);
> @@ -907,7 +907,7 @@ static int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  
>  	if (cp == NULL || argc > cp->maxargs)
>  		return CMD_RET_USAGE;
> -	if (flag == CMD_FLAG_REPEAT && !cp->repeatable)
> +	if (flag == CMD_FLAG_REPEAT && !cmd_is_repeatable(cp))
>  		return CMD_RET_SUCCESS;
>  
>  	if (curr_device < 0) {
> diff --git a/common/command.c b/common/command.c
> index e13cb47ac18b..19f0534a76ea 100644
> --- a/common/command.c
> +++ b/common/command.c
> @@ -501,6 +501,30 @@ void fixup_cmdtable(cmd_tbl_t *cmdtp, int size)
>  }
>  #endif
>  
> +int cmd_always_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
> +			  char * const argv[], int *repeatable)
> +{
> +	*repeatable = 1;
> +
> +	return cmdtp->cmd(cmdtp, flag, argc, argv);
> +}
> +
> +int cmd_never_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
> +			 char * const argv[], int *repeatable)
> +{
> +	*repeatable = 0;
> +
> +	return cmdtp->cmd(cmdtp, flag, argc, argv);
> +}
> +
> +int cmd_discard_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
> +			   char * const argv[])
> +{
> +	int repeatable;
> +
> +	return cmdtp->cmd_rep(cmdtp, flag, argc, argv, &repeatable);
> +}
> +
>  /**
>   * Call a command function. This should be the only route in U-Boot to call
>   * a command, so that we can track whether we are waiting for input or
> @@ -510,13 +534,15 @@ void fixup_cmdtable(cmd_tbl_t *cmdtp, int size)
>   * @param flag		Some flags normally 0 (see CMD_FLAG_.. above)
>   * @param argc		Number of arguments (arg 0 must be the command text)
>   * @param argv		Arguments
> + * @param repeatable	Can the command be repeated
>   * @return 0 if command succeeded, else non-zero (CMD_RET_...)
>   */
> -static int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +static int cmd_call(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
> +		    int *repeatable)
>  {
>  	int result;
>  
> -	result = (cmdtp->cmd)(cmdtp, flag, argc, argv);
> +	result = cmdtp->cmd_rep(cmdtp, flag, argc, argv, repeatable);
>  	if (result)
>  		debug("Command failed, result=%d\n", result);
>  	return result;
> @@ -553,12 +579,14 @@ enum command_ret_t cmd_process(int flag, int argc, char * const argv[],
>  
>  	/* If OK so far, then do the command */
>  	if (!rc) {
> +		int newrep;
> +
>  		if (ticks)
>  			*ticks = get_timer(0);
> -		rc = cmd_call(cmdtp, flag, argc, argv);
> +		rc = cmd_call(cmdtp, flag, argc, argv, &newrep);
>  		if (ticks)
>  			*ticks = get_timer(*ticks);
> -		*repeatable &= cmdtp->repeatable;
> +		*repeatable &= newrep;
>  	}
>  	if (rc == CMD_RET_USAGE)
>  		rc = cmd_usage(cmdtp);
> diff --git a/include/command.h b/include/command.h
> index 89efcecfa926..bb93f022c514 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -29,7 +29,16 @@
>  struct cmd_tbl_s {
>  	char		*name;		/* Command Name			*/
>  	int		maxargs;	/* maximum number of arguments	*/
> -	int		repeatable;	/* autorepeat allowed?		*/
> +					/*
> +					 * Same as ->cmd() except the command
> +					 * tells us if it can be repeated.
> +					 * Replaces the old ->repeatable field
> +					 * which was not able to make
> +					 * repeatable property different for
> +					 * the main command and sub-commands.
> +					 */
> +	int		(*cmd_rep)(struct cmd_tbl_s *cmd, int flags, int argc,
> +				   char * const argv[], int *repeatable);
>  					/* Implementation function	*/
>  	int		(*cmd)(struct cmd_tbl_s *, int, int, char * const []);
>  	char		*usage;		/* Usage message	(short)	*/
> @@ -60,6 +69,19 @@ int complete_subcmdv(cmd_tbl_t *cmdtp, int count, int argc,
>  
>  extern int cmd_usage(const cmd_tbl_t *cmdtp);
>  
> +/* Dummy ->cmd and ->cmd_rep wrappers. */
> +int cmd_always_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
> +			  char * const argv[], int *repeatable);
> +int cmd_never_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
> +			 char * const argv[], int *repeatable);
> +int cmd_discard_repeatable(cmd_tbl_t *cmdtp, int flag, int argc,
> +			   char * const argv[]);
> +
> +static inline bool cmd_is_repeatable(cmd_tbl_t *cmdtp)
> +{
> +	return cmdtp->cmd_rep == cmd_always_repeatable;
> +}
> +
>  #ifdef CONFIG_AUTO_COMPLETE
>  extern int var_complete(int argc, char * const argv[], char last_char, int maxv, char *cmdv[]);
>  extern int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp);
> @@ -188,16 +210,28 @@ int board_run_command(const char *cmdline);
>  #endif
>  
>  #ifdef CONFIG_CMDLINE
> +#define U_BOOT_CMDREP_MKENT_COMPLETE(_name, _maxargs, _cmd_rep,		\
> +				     _usage, _help, _comp)		\
> +		{ #_name, _maxargs, _cmd_rep, cmd_discard_repeatable,	\
> +		  _usage, _CMD_HELP(_help) _CMD_COMPLETE(_comp) }
> +
>  #define U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd,		\
>  				_usage, _help, _comp)			\
> -		{ #_name, _maxargs, _rep, _cmd, _usage,			\
> -			_CMD_HELP(_help) _CMD_COMPLETE(_comp) }
> +		{ #_name, _maxargs,					\
> +		 _rep ? cmd_always_repeatable : cmd_never_repeatable,	\
> +		 _cmd, _usage, _CMD_HELP(_help) _CMD_COMPLETE(_comp) }
>  
>  #define U_BOOT_CMD_COMPLETE(_name, _maxargs, _rep, _cmd, _usage, _help, _comp) \
>  	ll_entry_declare(cmd_tbl_t, _name, cmd) =			\
>  		U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd,	\
>  						_usage, _help, _comp);
>  
> +#define U_BOOT_CMDREP_COMPLETE(_name, _maxargs, _cmd_rep, _usage,	\
> +			       _help, _comp)				\
> +	ll_entry_declare(cmd_tbl_t, _name, cmd) =			\
> +		U_BOOT_CMDREP_MKENT_COMPLETE(_name, _maxargs, _cmd_rep,	\
> +					     _usage, _help, _comp)
> +
>  #else
>  #define U_BOOT_SUBCMD_START(name)	static cmd_tbl_t name[] = {};
>  #define U_BOOT_SUBCMD_END
> @@ -209,15 +243,25 @@ int board_run_command(const char *cmdline);
>  			_cmd(NULL, 0, 0, NULL);				\
>  		return 0;						\
>  	}
> +
> +#define U_BOOT_CMDREP_MKENT_COMPLETE(_name, _maxargs, _cmd_rep,		\
> +				     _usage, _help, _comp)		\
> +		{ #_name, _maxargs, 0 ? _cmd_rep : NULL, NULL, _usage,	\
> +			_CMD_HELP(_help) _CMD_COMPLETE(_comp) }
> +
>  #define U_BOOT_CMD_MKENT_COMPLETE(_name, _maxargs, _rep, _cmd, _usage,	\
>  				  _help, _comp)				\
> -		{ #_name, _maxargs, _rep, 0 ? _cmd : NULL, _usage,	\
> +		{ #_name, _maxargs, NULL, 0 ? _cmd : NULL, _usage,	\
>  			_CMD_HELP(_help) _CMD_COMPLETE(_comp) }
>  
>  #define U_BOOT_CMD_COMPLETE(_name, _maxargs, _rep, _cmd, _usage, _help,	\
>  			    _comp)				\
>  	_CMD_REMOVE(sub_ ## _name, _cmd)
>  
> +#define U_BOOT_CMDREP_COMPLETE(_name, _maxargs, _cmd_rep, _usage,	\
> +			       _help, _comp)				\
> +	_CMD_REMOVE(sub_ ## _name, _cmd_rep)
> +
>  #endif /* CONFIG_CMDLINE */
>  
>  #define U_BOOT_CMD(_name, _maxargs, _rep, _cmd, _usage, _help)		\

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190115/522c52a8/attachment.sig>

[U-Boot] [U-Boot, v4, 1/6] common: command: Fix command auto-completion

[Tom Rini]

On Mon, Dec 03, 2018 at 10:54:18PM +0100, Boris Brezillon wrote:

> When auto-completing command arguments, the last argument is not
> necessarily the one we need to auto-complete. When the last character is
> a space, a tab or '\0' what we want instead is list all possible values,
> or if there's only one possible value, place this value on the command
> line instead of trying to suffix the last valid argument with missing
> chars.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190115/d10d554c/attachment.sig>

[U-Boot] [U-Boot, v4, 2/6] common: command: Expose a generic helper to auto-complete sub commands

[Tom Rini]

On Mon, Dec 03, 2018 at 10:54:19PM +0100, Boris Brezillon wrote:

> Some commands have a table of sub-commands. With minor adjustments,
> complete_cmdv() is able to provide auto-completion for sub-commands
> (it's just about passing the table of commands instead of taking the
> global one).
> We rename this function into complete_subcmd() and implement
> complete_cmdv() as a wrapper around complete_subcmdv().
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190115/b4781c33/attachment.sig>

[U-Boot] [U-Boot, v4, 3/6] common: command: Rework the 'cmd is repeatable' logic

[Tom Rini]

On Mon, Dec 03, 2018 at 10:54:20PM +0100, Boris Brezillon wrote:

> The repeatable property is currently attached to the main command and
> sub-commands have no way to change the repeatable value (the
> ->repeatable field in sub-command entries is ignored).
> 
> Replace the ->repeatable field by an extended ->cmd() hook (called
> ->cmd_rep()) which takes a new int pointer to store the repeatable cap
> of the command being executed.
> 
> With this trick, we can let sub-commands decide whether they are
> repeatable or not.
> 
> We also patch mmc and dtimg who are testing the ->repeatable field
> directly (they now use cmd_is_repeatable() instead), and fix the help
> entry manually since it doesn't use the U_BOOT_CMD() macro.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190115/f87b515e/attachment.sig>

[U-Boot] [U-Boot, v4, 4/6] command: commands: Add macros to declare commands with subcmds

[Tom Rini]

On Mon, Dec 03, 2018 at 10:54:21PM +0100, Boris Brezillon wrote:

> Most cmd/xxx.c source files expose several commands through a single
> entry point. Some of them are doing the sub-command parsing manually in
> their do_<cmd>() function, others are declaring a table of sub-commands
> and then use find_cmd_tbl() to delegate the request to the sub command
> handler.
> 
> In either case, the amount of code to do that is not negligible and
> repetitive, not to mention that almost no commands are implementing
> the auto-completion hook, which means most u-boot commands lack
> auto-completion.
> 
> Provide several macros to easily define commands exposing sub-commands.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190115/f237b494/attachment.sig>

[U-Boot] [U-Boot, v4, 5/6] cmd: mtd: Use the subcmd infrastructure to declare mtd sub-commands

[Tom Rini]

On Mon, Dec 03, 2018 at 10:54:22PM +0100, Boris Brezillon wrote:

> It's way simpler this way, and we also gain auto-completion support for
> free (MTD name auto-completion has been added with mtd_name_complete())
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190115/0ed2f866/attachment.sig>

[U-Boot] [U-Boot, v4, 6/6] cmd: adc: Use the sub-command infrastructure

[Tom Rini]

On Mon, Dec 03, 2018 at 10:54:23PM +0100, Boris Brezillon wrote:

> And you get sub-command auto-completion for free.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190115/e4a15be3/attachment.sig>