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