[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 at 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 at st.com>
>> # Conflicts:
>> #       common/console.c
>> ---
>>
>>   common/console.c | 86 +++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 66 insertions(+), 20 deletions(-)
> Reviewed-by: Simon Glass <sjg at 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