| Message ID | 20201203102027.3.I3e15d39becc5c180a3be52571bb570e29c6b4cd0@changeid |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | console: remove #ifdef CONFIG when it is possible | expand |
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
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 --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)) {
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(-)