diff mbox series

[3/4] console: remove #ifdef CONFIG_CONSOLE_RECORD

Message ID 20201203102027.3.I3e15d39becc5c180a3be52571bb570e29c6b4cd0@changeid
State Superseded
Delegated to: Tom Rini
Headers show
Series console: remove #ifdef CONFIG when it is possible | expand

Commit Message

Patrick DELAUNAY Dec. 3, 2020, 9:20 a.m. UTC
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(-)

Comments

Simon Glass Dec. 12, 2020, 3:39 p.m. UTC | #1
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
Patrick DELAUNAY Dec. 15, 2020, 2:47 p.m. UTC | #2
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
diff mbox series

Patch

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)) {