Hi Patrick,
On Thu, 3 Dec 2020 at 02:20, Patrick Delaunay <patrick.delau...@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.delau...@st.com>
> # Conflicts:
> # common/console.c
> ---
>
> common/console.c | 86 +++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 66 insertions(+), 20 deletions(-)
Reviewed-by: Simon Glass <s...@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
