[PATCH 0/4] console: remove #ifdef CONFIG when it is possible

[PATCH 0/4] console: remove #ifdef CONFIG when it is possible

[Patrick Delaunay]

Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible

This patchset can applied on master branch after the serie [1]
(for the new order of test in function putc() and puts()
 done in "console: allow to record console output before ready"
)

I will push a separate serie to remove the remaining
#ifdef CONFIG_VIDCONSOLE_AS_LCD

And I remove the sandox code in [2] (with the associated tests
#ifdef CONFIG_SANDBOX)

[1] http://patchwork.ozlabs.org/project/uboot/list/?series=217079
    "log: don't build the trace buffer when log is not ready"

[2] http://patchwork.ozlabs.org/project/uboot/patch/20201127114927.2.Ida70f4fb1524187703e9240d63e436f8ae5adaab at changeid/
    "[2/2] console: sandbox: remove unnecessary sandbox code"



Patrick Delaunay (4):
  console: remove #ifdef CONFIG when it is possible
  console: add function console_devices_set
  console: remove #ifdef CONFIG_CONSOLE_RECORD
  console: add console_tstc_check helper function for CONSOLE_MUX

 common/console.c | 291 ++++++++++++++++++++++++++---------------------
 1 file changed, 164 insertions(+), 127 deletions(-)

-- 
2.17.1

[PATCH 1/4] console: remove #ifdef CONFIG when it is possible

[Patrick Delaunay]

Remove #ifdef or #ifndef for CONFIG when it is possible to simplify
the console.c code and respect the U-Boot coding rules.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 common/console.c | 149 +++++++++++++++++++----------------------------
 1 file changed, 61 insertions(+), 88 deletions(-)

diff --git a/common/console.c b/common/console.c
index c3d552bb3e..6fc3957024 100644
--- a/common/console.c
+++ b/common/console.c
@@ -44,14 +44,15 @@ static int on_console(const char *name, const char *value, enum env_op op,
 	case env_op_create:
 	case env_op_overwrite:
 
-#if CONFIG_IS_ENABLED(CONSOLE_MUX)
-		if (iomux_doenv(console, value))
-			return 1;
-#else
-		/* Try assigning specified device */
-		if (console_assign(console, value) < 0)
-			return 1;
-#endif
+		if (CONFIG_IS_ENABLED(CONSOLE_MUX)) {
+			if (iomux_doenv(console, value))
+				return 1;
+		} else {
+			/* Try assigning specified device */
+			if (console_assign(console, value) < 0)
+				return 1;
+		}
+
 		return 0;
 
 	case env_op_delete:
@@ -69,14 +70,13 @@ U_BOOT_ENV_CALLBACK(console, on_console);
 static int on_silent(const char *name, const char *value, enum env_op op,
 	int flags)
 {
-#if !CONFIG_IS_ENABLED(SILENT_CONSOLE_UPDATE_ON_SET)
-	if (flags & H_INTERACTIVE)
-		return 0;
-#endif
-#if !CONFIG_IS_ENABLED(SILENT_CONSOLE_UPDATE_ON_RELOC)
-	if ((flags & H_INTERACTIVE) == 0)
-		return 0;
-#endif
+	if (!CONFIG_IS_ENABLED(SILENT_CONSOLE_UPDATE_ON_SET))
+		if (flags & H_INTERACTIVE)
+			return 0;
+
+	if (!CONFIG_IS_ENABLED(SILENT_CONSOLE_UPDATE_ON_RELOC))
+		if ((flags & H_INTERACTIVE) == 0)
+			return 0;
 
 	if (value != NULL)
 		gd->flags |= GD_FLG_SILENT;
@@ -159,14 +159,13 @@ static bool console_dev_is_serial(struct stdio_dev *sdev)
 {
 	bool is_serial;
 
-#ifdef CONFIG_DM_SERIAL
-	if (sdev->flags & DEV_FLAGS_DM) {
+	if (IS_ENABLED(CONFIG_DM_SERIAL) && (sdev->flags & DEV_FLAGS_DM)) {
 		struct udevice *dev = sdev->priv;
 
 		is_serial = device_get_uclass_id(dev) == UCLASS_SERIAL;
-	} else
-#endif
-	is_serial = !strcmp(sdev->name, "serial");
+	} else {
+		is_serial = !strcmp(sdev->name, "serial");
+	}
 
 	return is_serial;
 }
@@ -350,13 +349,12 @@ int fgetc(int file)
 			if (console_tstc(file))
 				return console_getc(file);
 #endif
-#ifdef CONFIG_WATCHDOG
 			/*
 			 * If the watchdog must be rate-limited then it should
 			 * already be handled in board-specific code.
 			 */
-			 udelay(1);
-#endif
+			if (IS_ENABLED(CONFIG_WATCHDOG))
+				udelay(1);
 		}
 	}
 
@@ -406,10 +404,8 @@ int fprintf(int file, const char *fmt, ...)
 
 int getchar(void)
 {
-#ifdef CONFIG_DISABLE_CONSOLE
-	if (gd->flags & GD_FLG_DISABLE_CONSOLE)
+	if (IS_ENABLED(CONFIG_DISABLE_CONSOLE) && (gd->flags & GD_FLG_DISABLE_CONSOLE))
 		return 0;
-#endif
 
 	if (!gd->have_console)
 		return 0;
@@ -434,10 +430,8 @@ int getchar(void)
 
 int tstc(void)
 {
-#ifdef CONFIG_DISABLE_CONSOLE
-	if (gd->flags & GD_FLG_DISABLE_CONSOLE)
+	if (IS_ENABLED(CONFIG_DISABLE_CONSOLE) && (gd->flags & GD_FLG_DISABLE_CONSOLE))
 		return 0;
-#endif
 
 	if (!gd->have_console)
 		return 0;
@@ -485,10 +479,8 @@ static void print_pre_console_buffer(int flushpoint)
 	char buf_out[CONFIG_PRE_CON_BUF_SZ + 1];
 	char *buf_in;
 
-#ifdef CONFIG_SILENT_CONSOLE
-	if (gd->flags & GD_FLG_SILENT)
+	if (IS_ENABLED(CONFIG_SILENT_CONSOLE) && (gd->flags & GD_FLG_SILENT))
 		return;
-#endif
 
 	buf_in = map_sysmem(CONFIG_PRE_CON_BUF_ADDR, CONFIG_PRE_CON_BUF_SZ);
 	if (gd->precon_buf_idx > CONFIG_PRE_CON_BUF_SZ)
@@ -530,25 +522,20 @@ void putc(const char c)
 		return;
 	}
 #endif
-#ifdef CONFIG_DEBUG_UART
 	/* if we don't have a console yet, use the debug UART */
-	if (!(gd->flags & GD_FLG_SERIAL_READY)) {
+	if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags & GD_FLG_SERIAL_READY)) {
 		printch(c);
 		return;
 	}
-#endif
-#ifdef CONFIG_SILENT_CONSOLE
-	if (gd->flags & GD_FLG_SILENT) {
+
+	if (IS_ENABLED(CONFIG_SILENT_CONSOLE) && (gd->flags & GD_FLG_SILENT)) {
 		if (!(gd->flags & GD_FLG_DEVINIT))
 			pre_console_putc(c);
 		return;
 	}
-#endif
 
-#ifdef CONFIG_DISABLE_CONSOLE
-	if (gd->flags & GD_FLG_DISABLE_CONSOLE)
+	if (IS_ENABLED(CONFIG_DISABLE_CONSOLE) && (gd->flags & GD_FLG_DISABLE_CONSOLE))
 		return;
-#endif
 
 	if (!gd->have_console)
 		return pre_console_putc(c);
@@ -578,8 +565,8 @@ void puts(const char *s)
 		return;
 	}
 #endif
-#ifdef CONFIG_DEBUG_UART
-	if (!(gd->flags & GD_FLG_SERIAL_READY)) {
+
+	if (IS_ENABLED(CONFIG_DEBUG_UART) && !(gd->flags & GD_FLG_SERIAL_READY)) {
 		while (*s) {
 			int ch = *s++;
 
@@ -587,19 +574,15 @@ void puts(const char *s)
 		}
 		return;
 	}
-#endif
-#ifdef CONFIG_SILENT_CONSOLE
-	if (gd->flags & GD_FLG_SILENT) {
+
+	if (IS_ENABLED(CONFIG_SILENT_CONSOLE) && (gd->flags & GD_FLG_SILENT)) {
 		if (!(gd->flags & GD_FLG_DEVINIT))
 			pre_console_puts(s);
 		return;
 	}
-#endif
 
-#ifdef CONFIG_DISABLE_CONSOLE
-	if (gd->flags & GD_FLG_DISABLE_CONSOLE)
+	if (IS_ENABLED(CONFIG_DISABLE_CONSOLE) && (gd->flags & GD_FLG_DISABLE_CONSOLE))
 		return;
-#endif
 
 	if (!gd->have_console)
 		return pre_console_puts(s);
@@ -772,19 +755,19 @@ int console_assign(int file, const char *devname)
 /* return true if the 'silent' flag is removed */
 static bool console_update_silent(void)
 {
-#ifdef CONFIG_SILENT_CONSOLE
-	if (env_get("silent")) {
-		gd->flags |= GD_FLG_SILENT;
-	} else {
-		unsigned long flags = gd->flags;
+	unsigned long flags = gd->flags;
 
-		gd->flags &= ~GD_FLG_SILENT;
+	if (!IS_ENABLED(CONFIG_SILENT_CONSOLE))
+		return false;
 
-		return !!(flags & GD_FLG_SILENT);
+	if (env_get("silent")) {
+		gd->flags |= GD_FLG_SILENT;
+		return false;
 	}
-#endif
 
-	return false;
+	gd->flags &= ~GD_FLG_SILENT;
+
+	return !!(flags & GD_FLG_SILENT);
 }
 
 int console_announce_r(void)
@@ -843,12 +826,8 @@ int console_init_r(void)
 {
 	char *stdinname, *stdoutname, *stderrname;
 	struct stdio_dev *inputdev = NULL, *outputdev = NULL, *errdev = NULL;
-#ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE
 	int i;
-#endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */
-#if CONFIG_IS_ENABLED(CONSOLE_MUX)
 	int iomux_err = 0;
-#endif
 	int flushpoint;
 
 	/* update silent for env loaded from flash (initr_env) */
@@ -874,14 +853,14 @@ int console_init_r(void)
 		inputdev  = search_device(DEV_FLAGS_INPUT,  stdinname);
 		outputdev = search_device(DEV_FLAGS_OUTPUT, stdoutname);
 		errdev    = search_device(DEV_FLAGS_OUTPUT, stderrname);
-#if CONFIG_IS_ENABLED(CONSOLE_MUX)
-		iomux_err = iomux_doenv(stdin, stdinname);
-		iomux_err += iomux_doenv(stdout, stdoutname);
-		iomux_err += iomux_doenv(stderr, stderrname);
-		if (!iomux_err)
-			/* Successful, so skip all the code below. */
-			goto done;
-#endif
+		if (CONFIG_IS_ENABLED(CONSOLE_MUX)) {
+			iomux_err = iomux_doenv(stdin, stdinname);
+			iomux_err += iomux_doenv(stdout, stdoutname);
+			iomux_err += iomux_doenv(stderr, stderrname);
+			if (!iomux_err)
+				/* Successful, so skip all the code below. */
+				goto done;
+		}
 	}
 	/* if the devices are overwritten or not found, use default device */
 	if (inputdev == NULL) {
@@ -907,25 +886,22 @@ int console_init_r(void)
 		console_doenv(stdin, inputdev);
 	}
 
-#if CONFIG_IS_ENABLED(CONSOLE_MUX)
 done:
-#endif
 
-#ifndef CONFIG_SYS_CONSOLE_INFO_QUIET
-	stdio_print_current_devices();
-#endif /* CONFIG_SYS_CONSOLE_INFO_QUIET */
+	if (!IS_ENABLED(CONFIG_SYS_CONSOLE_INFO_QUIET))
+		stdio_print_current_devices();
+
 #ifdef CONFIG_VIDCONSOLE_AS_LCD
 	if (strstr(stdoutname, CONFIG_VIDCONSOLE_AS_NAME))
 		printf("Warning: Please change '%s' to 'vidconsole' in stdout/stderr environment vars\n",
 		       CONFIG_VIDCONSOLE_AS_NAME);
 #endif
 
-#ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE
-	/* set the environment variables (will overwrite previous env settings) */
-	for (i = 0; i < MAX_FILES; i++) {
-		env_set(stdio_names[i], stdio_devices[i]->name);
+	if (IS_ENABLED(CONFIG_SYS_CONSOLE_ENV_OVERWRITE)) {
+		/* set the environment variables (will overwrite previous env settings) */
+		for (i = 0; i < MAX_FILES; i++)
+			env_set(stdio_names[i], stdio_devices[i]->name);
 	}
-#endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */
 
 	gd->flags |= GD_FLG_DEVINIT;	/* device initialization completed */
 
@@ -956,18 +932,16 @@ int console_init_r(void)
 	else
 		flushpoint = PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL;
 
-#ifdef CONFIG_SPLASH_SCREEN
 	/*
 	 * suppress all output if splash screen is enabled and we have
 	 * a bmp to display. We redirect the output from frame buffer
 	 * console to serial console in this case or suppress it if
 	 * "silent" mode was requested.
 	 */
-	if (env_get("splashimage") != NULL) {
+	if (IS_ENABLED(CONFIG_SPLASH_SCREEN) && env_get("splashimage")) {
 		if (!(gd->flags & GD_FLG_SILENT))
 			outputdev = search_device (DEV_FLAGS_OUTPUT, "serial");
 	}
-#endif
 
 	/* Scan devices looking for input and output devices */
 	list_for_each(pos, list) {
@@ -1001,9 +975,8 @@ int console_init_r(void)
 #endif
 	}
 
-#ifndef CONFIG_SYS_CONSOLE_INFO_QUIET
-	stdio_print_current_devices();
-#endif /* CONFIG_SYS_CONSOLE_INFO_QUIET */
+	if (!IS_ENABLED(CONFIG_SYS_CONSOLE_INFO_QUIET))
+		stdio_print_current_devices();
 
 	/* Setting environment variables */
 	for (i = 0; i < MAX_FILES; i++) {
-- 
2.17.1

[PATCH 2/4] console: add function console_devices_set

[Patrick Delaunay]

Add a new function to access to console_devices only defined if
CONFIG_IS_ENABLED(CONSOLE_MUX).

This path allows to remove #if CONFIG_IS_ENABLED(CONSOLE_MUX)
in console_getc function.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 common/console.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/common/console.c b/common/console.c
index 6fc3957024..9a63eaa664 100644
--- a/common/console.c
+++ b/common/console.c
@@ -177,6 +177,11 @@ static struct stdio_dev *tstcdev;
 struct stdio_dev **console_devices[MAX_FILES];
 int cd_count[MAX_FILES];
 
+static void __maybe_unused console_devices_set(int file, struct stdio_dev *dev)
+{
+	console_devices[file][0] = dev;
+}
+
 /*
  * This depends on tstc() always being called before getchar().
  * This is guaranteed to be true because this routine is called
@@ -275,6 +280,11 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
 }
 #endif
 #else
+
+static void __maybe_unused console_devices_set(int file, struct stdio_dev *dev)
+{
+}
+
 static inline int console_getc(int file)
 {
 	return stdio_devices[file]->getc(stdio_devices[file]);
@@ -961,18 +971,14 @@ int console_init_r(void)
 	if (outputdev != NULL) {
 		console_setfile(stdout, outputdev);
 		console_setfile(stderr, outputdev);
-#if CONFIG_IS_ENABLED(CONSOLE_MUX)
-		console_devices[stdout][0] = outputdev;
-		console_devices[stderr][0] = outputdev;
-#endif
+		console_devices_set(stdout, outputdev);
+		console_devices_set(stderr, outputdev);
 	}
 
 	/* Initializes input console */
 	if (inputdev != NULL) {
 		console_setfile(stdin, inputdev);
-#if CONFIG_IS_ENABLED(CONSOLE_MUX)
-		console_devices[stdin][0] = inputdev;
-#endif
+		console_devices_set(stdin, inputdev);
 	}
 
 	if (!IS_ENABLED(CONFIG_SYS_CONSOLE_INFO_QUIET))
-- 
2.17.1

[PATCH 3/4] console: remove #ifdef CONFIG_CONSOLE_RECORD

[Patrick Delaunay]

Add helper functions to access to gd->console_out and gd->console_in
with membuff API and replace the #ifdef CONFIG_CONSOLE_RECORD test
by if (IS_ENABLED(CONFIG_CONSOLE_RECORD)) to respect the U-Boot
coding rule.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
# Conflicts:
#	common/console.c
---

 common/console.c | 86 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/common/console.c b/common/console.c
index 9a63eaa664..0b864444bb 100644
--- a/common/console.c
+++ b/common/console.c
@@ -88,6 +88,56 @@ static int on_silent(const char *name, const char *value, enum env_op op,
 U_BOOT_ENV_CALLBACK(silent, on_silent);
 #endif
 
+#ifdef CONFIG_CONSOLE_RECORD
+/* helper function: access to gd->console_out and gd->console_in */
+static void console_record_putc(const char c)
+{
+	if  (gd->console_out.start)
+		membuff_putbyte((struct membuff *)&gd->console_out, c);
+}
+
+static void console_record_puts(const char *s)
+{
+	if  (gd->console_out.start)
+		membuff_put((struct membuff *)&gd->console_out, s, strlen(s));
+}
+
+static int console_record_getc(void)
+{
+	if (!gd->console_in.start)
+		return -1;
+
+	return membuff_getbyte((struct membuff *)&gd->console_in);
+}
+
+static int console_record_tstc(void)
+{
+	if (gd->console_in.start) {
+		if (membuff_peekbyte((struct membuff *)&gd->console_in) != -1)
+			return 1;
+	}
+	return 0;
+}
+#else
+static void console_record_putc(char c)
+{
+}
+
+static void console_record_puts(const char *s)
+{
+}
+
+static int console_record_getc(void)
+{
+	return -1;
+}
+
+static int console_record_tstc(void)
+{
+	return 0;
+}
+#endif
+
 #if CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)
 /*
  * if overwrite_console returns 1, the stdin, stderr and stdout
@@ -420,15 +470,13 @@ int getchar(void)
 	if (!gd->have_console)
 		return 0;
 
-#ifdef CONFIG_CONSOLE_RECORD
-	if (gd->console_in.start) {
-		int ch;
+	if (IS_ENABLED(CONFIG_CONSOLE_RECORD)) {
+		int ch = console_record_getc();
 
-		ch = membuff_getbyte((struct membuff *)&gd->console_in);
 		if (ch != -1)
-			return 1;
+			return ch;
 	}
-#endif
+
 	if (gd->flags & GD_FLG_DEVINIT) {
 		/* Get from the standard input */
 		return fgetc(stdin);
@@ -445,12 +493,10 @@ int tstc(void)
 
 	if (!gd->have_console)
 		return 0;
-#ifdef CONFIG_CONSOLE_RECORD
-	if (gd->console_in.start) {
-		if (membuff_peekbyte((struct membuff *)&gd->console_in) != -1)
-			return 1;
-	}
-#endif
+
+	if (IS_ENABLED(CONFIG_CONSOLE_RECORD) && console_record_tstc())
+		return 1;
+
 	if (gd->flags & GD_FLG_DEVINIT) {
 		/* Test the standard input */
 		return ftstc(stdin);
@@ -521,10 +567,10 @@ void putc(const char c)
 {
 	if (!gd)
 		return;
-#ifdef CONFIG_CONSOLE_RECORD
-	if ((gd->flags & GD_FLG_RECORD) && gd->console_out.start)
-		membuff_putbyte((struct membuff *)&gd->console_out, c);
-#endif
+
+	if (IS_ENABLED(CONFIG_CONSOLE_RECORD) && (gd->flags & GD_FLG_RECORD))
+		console_record_putc(c);
+
 #ifdef CONFIG_SANDBOX
 	/* sandbox can send characters to stdout before it has a console */
 	if (!(gd->flags & GD_FLG_SERIAL_READY)) {
@@ -564,10 +610,10 @@ void puts(const char *s)
 {
 	if (!gd)
 		return;
-#ifdef CONFIG_CONSOLE_RECORD
-	if ((gd->flags & GD_FLG_RECORD) && gd->console_out.start)
-		membuff_put((struct membuff *)&gd->console_out, s, strlen(s));
-#endif
+
+	if (IS_ENABLED(CONFIG_CONSOLE_RECORD) && (gd->flags & GD_FLG_RECORD))
+		console_record_puts(s);
+
 #ifdef CONFIG_SANDBOX
 	/* sandbox can send characters to stdout before it has a console */
 	if (!(gd->flags & GD_FLG_SERIAL_READY)) {
-- 
2.17.1

[PATCH 4/4] console: add console_tstc_check helper function for CONSOLE_MUX

[Patrick Delaunay]

Add the helper function console_tstc_check() and replace the test
#if CONFIG_IS_ENABLED(CONSOLE_MUX) to a simple if to respect the
U-Boot coding rule.

No functional change.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 common/console.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/common/console.c b/common/console.c
index 0b864444bb..c570cd88cf 100644
--- a/common/console.c
+++ b/common/console.c
@@ -248,6 +248,12 @@ static int console_getc(int file)
 	return ret;
 }
 
+/*  Upper layer may have already called tstc(): check the saved result */
+static bool console_tstc_check(void)
+{
+	return !!tstcdev;
+}
+
 static int console_tstc(int file)
 {
 	int i, ret;
@@ -340,6 +346,11 @@ static inline int console_getc(int file)
 	return stdio_devices[file]->getc(stdio_devices[file]);
 }
 
+static bool console_tstc_check(void)
+{
+	return false;
+}
+
 static inline int console_tstc(int file)
 {
 	return stdio_devices[file]->tstc(stdio_devices[file]);
@@ -397,18 +408,19 @@ int fgetc(int file)
 		 */
 		for (;;) {
 			WATCHDOG_RESET();
-#if CONFIG_IS_ENABLED(CONSOLE_MUX)
-			/*
-			 * Upper layer may have already called tstc() so
-			 * check for that first.
-			 */
-			if (tstcdev != NULL)
-				return console_getc(file);
-			console_tstc(file);
-#else
-			if (console_tstc(file))
-				return console_getc(file);
-#endif
+			if (CONFIG_IS_ENABLED(CONSOLE_MUX)) {
+				/*
+				 * Upper layer may have already called tstc() so
+				 * check for that first.
+				 */
+				if (console_tstc_check())
+					return console_getc(file);
+				console_tstc(file);
+			} else {
+				if (console_tstc(file))
+					return console_getc(file);
+			}
+
 			/*
 			 * If the watchdog must be rate-limited then it should
 			 * already be handled in board-specific code.
-- 
2.17.1

[PATCH 1/4] console: remove #ifdef CONFIG when it is possible

[Simon Glass]

On Thu, 3 Dec 2020 at 02:20, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Remove #ifdef or #ifndef for CONFIG when it is possible to simplify
> the console.c code and respect the U-Boot coding rules.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  common/console.c | 149 +++++++++++++++++++----------------------------
>  1 file changed, 61 insertions(+), 88 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 2/4] console: add function console_devices_set

[Simon Glass]

On Thu, 3 Dec 2020 at 02:20, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Add a new function to access to console_devices only defined if
> CONFIG_IS_ENABLED(CONSOLE_MUX).
>
> This path allows to remove #if CONFIG_IS_ENABLED(CONSOLE_MUX)
> in console_getc function.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  common/console.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 3/4] console: remove #ifdef CONFIG_CONSOLE_RECORD

[Simon Glass]

Hi Patrick,

On Thu, 3 Dec 2020 at 02:20, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Add helper functions to access to gd->console_out and gd->console_in
> with membuff API and replace the #ifdef CONFIG_CONSOLE_RECORD test
> by if (IS_ENABLED(CONFIG_CONSOLE_RECORD)) to respect the U-Boot
> coding rule.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> # Conflicts:
> #       common/console.c
> ---
>
>  common/console.c | 86 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 66 insertions(+), 20 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

But see below

>
> diff --git a/common/console.c b/common/console.c
> index 9a63eaa664..0b864444bb 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -88,6 +88,56 @@ static int on_silent(const char *name, const char *value, enum env_op op,
>  U_BOOT_ENV_CALLBACK(silent, on_silent);
>  #endif
>
> +#ifdef CONFIG_CONSOLE_RECORD
> +/* helper function: access to gd->console_out and gd->console_in */
> +static void console_record_putc(const char c)
> +{
> +       if  (gd->console_out.start)
> +               membuff_putbyte((struct membuff *)&gd->console_out, c);
> +}
> +
> +static void console_record_puts(const char *s)
> +{
> +       if  (gd->console_out.start)
> +               membuff_put((struct membuff *)&gd->console_out, s, strlen(s));
> +}
> +
> +static int console_record_getc(void)
> +{
> +       if (!gd->console_in.start)
> +               return -1;
> +
> +       return membuff_getbyte((struct membuff *)&gd->console_in);
> +}
> +
> +static int console_record_tstc(void)
> +{
> +       if (gd->console_in.start) {
> +               if (membuff_peekbyte((struct membuff *)&gd->console_in) != -1)
> +                       return 1;
> +       }
> +       return 0;
> +}
> +#else
> +static void console_record_putc(char c)
> +{
> +}
> +
> +static void console_record_puts(const char *s)
> +{
> +}
> +
> +static int console_record_getc(void)
> +{
> +       return -1;
> +}
> +
> +static int console_record_tstc(void)
> +{
> +       return 0;
> +}
> +#endif
> +
>  #if CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)
>  /*
>   * if overwrite_console returns 1, the stdin, stderr and stdout
> @@ -420,15 +470,13 @@ int getchar(void)
>         if (!gd->have_console)
>                 return 0;
>
> -#ifdef CONFIG_CONSOLE_RECORD
> -       if (gd->console_in.start) {
> -               int ch;
> +       if (IS_ENABLED(CONFIG_CONSOLE_RECORD)) {
> +               int ch = console_record_getc();
>
> -               ch = membuff_getbyte((struct membuff *)&gd->console_in);
>                 if (ch != -1)
> -                       return 1;
> +                       return ch;
>         }
> -#endif
> +
>         if (gd->flags & GD_FLG_DEVINIT) {
>                 /* Get from the standard input */
>                 return fgetc(stdin);
> @@ -445,12 +493,10 @@ int tstc(void)
>
>         if (!gd->have_console)
>                 return 0;
> -#ifdef CONFIG_CONSOLE_RECORD
> -       if (gd->console_in.start) {
> -               if (membuff_peekbyte((struct membuff *)&gd->console_in) != -1)
> -                       return 1;
> -       }
> -#endif
> +
> +       if (IS_ENABLED(CONFIG_CONSOLE_RECORD) && console_record_tstc())
> +               return 1;
> +
>         if (gd->flags & GD_FLG_DEVINIT) {
>                 /* Test the standard input */
>                 return ftstc(stdin);
> @@ -521,10 +567,10 @@ void putc(const char c)
>  {
>         if (!gd)
>                 return;
> -#ifdef CONFIG_CONSOLE_RECORD
> -       if ((gd->flags & GD_FLG_RECORD) && gd->console_out.start)
> -               membuff_putbyte((struct membuff *)&gd->console_out, c);
> -#endif
> +
> +       if (IS_ENABLED(CONFIG_CONSOLE_RECORD) && (gd->flags & GD_FLG_RECORD))
> +               console_record_putc(c);

Given your functions above I wonder why you need the IS_ENABLED()
here? Maybe even move the gd-.flags check into those functions?

> +
>  #ifdef CONFIG_SANDBOX
>         /* sandbox can send characters to stdout before it has a console */
>         if (!(gd->flags & GD_FLG_SERIAL_READY)) {
> @@ -564,10 +610,10 @@ void puts(const char *s)
>  {
>         if (!gd)
>                 return;
> -#ifdef CONFIG_CONSOLE_RECORD
> -       if ((gd->flags & GD_FLG_RECORD) && gd->console_out.start)
> -               membuff_put((struct membuff *)&gd->console_out, s, strlen(s));
> -#endif
> +
> +       if (IS_ENABLED(CONFIG_CONSOLE_RECORD) && (gd->flags & GD_FLG_RECORD))
> +               console_record_puts(s);
> +
>  #ifdef CONFIG_SANDBOX
>         /* sandbox can send characters to stdout before it has a console */
>         if (!(gd->flags & GD_FLG_SERIAL_READY)) {
> --
> 2.17.1
>

Regards,
Simon

[PATCH 4/4] console: add console_tstc_check helper function for CONSOLE_MUX

[Simon Glass]

Hi Patrick,

On Thu, 3 Dec 2020 at 02:20, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Add the helper function console_tstc_check() and replace the test
> #if CONFIG_IS_ENABLED(CONSOLE_MUX) to a simple if to respect the
> U-Boot coding rule.
>
> No functional change.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  common/console.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/common/console.c b/common/console.c
> index 0b864444bb..c570cd88cf 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -248,6 +248,12 @@ static int console_getc(int file)
>         return ret;
>  }
>
> +/*  Upper layer may have already called tstc(): check the saved result */
> +static bool console_tstc_check(void)

This is checking if we actually have a device, right? I think
has_tstcdev() would be better.

Also could you add a comment in the testcdev variable declaration as
to what this variable actually is for?

> +{
> +       return !!tstcdev;
> +}
> +
>  static int console_tstc(int file)
>  {
>         int i, ret;
> @@ -340,6 +346,11 @@ static inline int console_getc(int file)
>         return stdio_devices[file]->getc(stdio_devices[file]);
>  }
>
> +static bool console_tstc_check(void)
> +{
> +       return false;
> +}
> +
>  static inline int console_tstc(int file)
>  {
>         return stdio_devices[file]->tstc(stdio_devices[file]);
> @@ -397,18 +408,19 @@ int fgetc(int file)
>                  */
>                 for (;;) {
>                         WATCHDOG_RESET();
> -#if CONFIG_IS_ENABLED(CONSOLE_MUX)
> -                       /*
> -                        * Upper layer may have already called tstc() so
> -                        * check for that first.
> -                        */
> -                       if (tstcdev != NULL)
> -                               return console_getc(file);
> -                       console_tstc(file);
> -#else
> -                       if (console_tstc(file))
> -                               return console_getc(file);
> -#endif
> +                       if (CONFIG_IS_ENABLED(CONSOLE_MUX)) {
> +                               /*
> +                                * Upper layer may have already called tstc() so
> +                                * check for that first.
> +                                */
> +                               if (console_tstc_check())
> +                                       return console_getc(file);
> +                               console_tstc(file);
> +                       } else {
> +                               if (console_tstc(file))
> +                                       return console_getc(file);
> +                       }
> +
>                         /*
>                          * If the watchdog must be rate-limited then it should
>                          * already be handled in board-specific code.
> --
> 2.17.1
>

Regards,
Simon

[PATCH 3/4] console: remove #ifdef CONFIG_CONSOLE_RECORD

[Patrick DELAUNAY]

On 12/12/20 4:39 PM, Simon Glass wrote:
> Hi Patrick,
>
> On Thu, 3 Dec 2020 at 02:20, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>> Add helper functions to access to gd->console_out and gd->console_in
>> with membuff API and replace the #ifdef CONFIG_CONSOLE_RECORD test
>> by if (IS_ENABLED(CONFIG_CONSOLE_RECORD)) to respect the U-Boot
>> coding rule.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
>> # Conflicts:
>> #       common/console.c
>> ---
>>
>>   common/console.c | 86 +++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 66 insertions(+), 20 deletions(-)
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> But see below
>
>> diff --git a/common/console.c b/common/console.c
>> index 9a63eaa664..0b864444bb 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -88,6 +88,56 @@ static int on_silent(const char *name, const char *value, enum env_op op,
>>   U_BOOT_ENV_CALLBACK(silent, on_silent);
>>   #endif
>>
>> +#ifdef CONFIG_CONSOLE_RECORD
>> +/* helper function: access to gd->console_out and gd->console_in */
>> +static void console_record_putc(const char c)
>> +{
>> +       if  (gd->console_out.start)
>> +               membuff_putbyte((struct membuff *)&gd->console_out, c);
>> +}
>> +
>> +static void console_record_puts(const char *s)
>> +{
>> +       if  (gd->console_out.start)
>> +               membuff_put((struct membuff *)&gd->console_out, s, strlen(s));
>> +}
>> +
>> +static int console_record_getc(void)
>> +{
>> +       if (!gd->console_in.start)
>> +               return -1;
>> +
>> +       return membuff_getbyte((struct membuff *)&gd->console_in);
>> +}
>> +
>> +static int console_record_tstc(void)
>> +{
>> +       if (gd->console_in.start) {
>> +               if (membuff_peekbyte((struct membuff *)&gd->console_in) != -1)
>> +                       return 1;
>> +       }
>> +       return 0;
>> +}
>> +#else
>> +static void console_record_putc(char c)
>> +{
>> +}
>> +
>> +static void console_record_puts(const char *s)
>> +{
>> +}
>> +
>> +static int console_record_getc(void)
>> +{
>> +       return -1;
>> +}
>> +
>> +static int console_record_tstc(void)
>> +{
>> +       return 0;
>> +}
>> +#endif
>> +
>>   #if CONFIG_IS_ENABLED(SYS_CONSOLE_IS_IN_ENV)
>>   /*
>>    * if overwrite_console returns 1, the stdin, stderr and stdout
>> @@ -420,15 +470,13 @@ int getchar(void)
>>          if (!gd->have_console)
>>                  return 0;
>>
>> -#ifdef CONFIG_CONSOLE_RECORD
>> -       if (gd->console_in.start) {
>> -               int ch;
>> +       if (IS_ENABLED(CONFIG_CONSOLE_RECORD)) {
>> +               int ch = console_record_getc();
>>
>> -               ch = membuff_getbyte((struct membuff *)&gd->console_in);
>>                  if (ch != -1)
>> -                       return 1;
>> +                       return ch;
>>          }
>> -#endif
>> +
>>          if (gd->flags & GD_FLG_DEVINIT) {
>>                  /* Get from the standard input */
>>                  return fgetc(stdin);
>> @@ -445,12 +493,10 @@ int tstc(void)
>>
>>          if (!gd->have_console)
>>                  return 0;
>> -#ifdef CONFIG_CONSOLE_RECORD
>> -       if (gd->console_in.start) {
>> -               if (membuff_peekbyte((struct membuff *)&gd->console_in) != -1)
>> -                       return 1;
>> -       }
>> -#endif
>> +
>> +       if (IS_ENABLED(CONFIG_CONSOLE_RECORD) && console_record_tstc())
>> +               return 1;
>> +
>>          if (gd->flags & GD_FLG_DEVINIT) {
>>                  /* Test the standard input */
>>                  return ftstc(stdin);
>> @@ -521,10 +567,10 @@ void putc(const char c)
>>   {
>>          if (!gd)
>>                  return;
>> -#ifdef CONFIG_CONSOLE_RECORD
>> -       if ((gd->flags & GD_FLG_RECORD) && gd->console_out.start)
>> -               membuff_putbyte((struct membuff *)&gd->console_out, c);
>> -#endif
>> +
>> +       if (IS_ENABLED(CONFIG_CONSOLE_RECORD) && (gd->flags & GD_FLG_RECORD))
>> +               console_record_putc(c);
> Given your functions above I wonder why you need the IS_ENABLED()
> here? Maybe even move the gd-.flags check into those functions?

In fact it is not needed.

I limit the difference to easy the review and to be coherent with other 
test on flags, for example:

 ??? if (IS_ENABLED(CONFIG_DISABLE_CONSOLE) && (gd->flags & 
GD_FLG_DISABLE_CONSOLE))

 ??? if (IS_ENABLED(CONFIG_SILENT_CONSOLE) && (gd->flags & GD_FLG_SILENT))

But you are right it is more elegant if I move the 2 tests in the helper 
function.

I will do it it in V2

Regards,

Patrick