Clean up a few things in STDIO. Mostly, don't export structures directly, but introduce some kind of accessors if needed and remove dead code.
NOTE: I'm still working on the "compile tested on 2 different architectures" part. I'll keep you posted about that. I'd be glad for a review though. I'd hate to pull in logic errors, especially into such critical code. NOTE2: Cross-posting cover to DM list, so I get feedback from those guys.
Marek Vasut (6): stdio: dm: Murder dead code in console.c stdio: dm: Add accessors to stdio_devices[] stdio: dm: Make stdio_devices[] local stdio: dm: Add stdio_fd_to_name() call stdio: dm: Use stdio_fd_to_name() call to localize stdio_names stdio: dm: Optimize stdio_print_current_devices()
common/cmd_console.c | 8 ++--- common/cmd_terminal.c | 9 ++++-- common/console.c | 84 ++++++++++++++++++++++++++----------------------- common/fdt_support.c | 8 ++++- common/stdio.c | 46 +++++++++++++++++++++++++-- include/stdio_dev.h | 9 ++---- 6 files changed, 110 insertions(+), 54 deletions(-)
Cc: Wolfgang Denk wd@denx.de Cc: u-boot-dm@lists.denx.de
Signed-off-by: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de --- common/console.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/common/console.c b/common/console.c index 1177f7d..df03cef 100644 --- a/common/console.c +++ b/common/console.c @@ -694,11 +694,6 @@ done: } #endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */
-#if 0 - /* If nothing usable installed, use only the initial console */ - if ((stdio_devices[stdin] == NULL) && (stdio_devices[stdout] == NULL)) - return 0; -#endif return 0; }
@@ -767,12 +762,6 @@ int console_init_r(void) setenv(stdio_names[i], stdio_devices[i]->name); }
-#if 0 - /* If nothing usable installed, use only the initial console */ - if ((stdio_devices[stdin] == NULL) && (stdio_devices[stdout] == NULL)) - return 0; -#endif - return 0; }
Implement stdio_get_fd() and stdio_set_fd() calls to access currently assigned STDIO devices. This will in turn allow to make the stdio_devices[] local to stdio.c
Signed-off-by: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de --- common/stdio.c | 26 ++++++++++++++++++++++++++ include/stdio_dev.h | 2 ++ 2 files changed, 28 insertions(+)
diff --git a/common/stdio.c b/common/stdio.c index 1bf9ba0..6c8ac73 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -33,6 +33,7 @@ #if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C) #include <i2c.h> #endif +#include <errno.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -122,6 +123,31 @@ struct stdio_dev* stdio_get_by_name(const char *name) return NULL; }
+struct stdio_dev *stdio_get_fd(int ioe) +{ + switch (ioe) { + case stdin: + case stdout: + case stderr: + return stdio_devices[ioe]; + default: + return NULL; + } +} + +int stdio_set_fd(int ioe, struct stdio_dev *dev) +{ + switch (ioe) { + case stdin: + case stdout: + case stderr: + stdio_devices[ioe] = dev; + return 0; + default: + return -EINVAL; + } +} + struct stdio_dev* stdio_clone(struct stdio_dev *dev) { struct stdio_dev *_dev; diff --git a/include/stdio_dev.h b/include/stdio_dev.h index 23e0ee1..86b2a4f 100644 --- a/include/stdio_dev.h +++ b/include/stdio_dev.h @@ -97,6 +97,8 @@ int stdio_deregister(const char *devname); #endif struct list_head* stdio_get_list(void); struct stdio_dev* stdio_get_by_name(const char* name); +struct stdio_dev *stdio_get_fd(int ioe); +int stdio_set_fd(int ioe, struct stdio_dev *dev); struct stdio_dev* stdio_clone(struct stdio_dev *dev);
#ifdef CONFIG_ARM_DCC_MULTI
Use stdio_get_fd() and stdio_set_fd() instead of direct access to the array. This allows making stdio_devices[] local to stdio.c
Signed-off-by: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de --- common/cmd_console.c | 8 +++---- common/cmd_terminal.c | 9 ++++++-- common/console.c | 60 ++++++++++++++++++++++++++++++++----------------- common/fdt_support.c | 8 ++++++- common/stdio.c | 2 +- include/stdio_dev.h | 1 - 6 files changed, 58 insertions(+), 30 deletions(-)
diff --git a/common/cmd_console.c b/common/cmd_console.c index d8cad6b..343bc28 100644 --- a/common/cmd_console.c +++ b/common/cmd_console.c @@ -34,7 +34,7 @@ int do_coninfo (cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) int l; struct list_head *list = stdio_get_list(); struct list_head *pos; - struct stdio_dev *dev; + struct stdio_dev *dev, *sio;
/* Scan for valid output and input devices */
@@ -51,9 +51,9 @@ int do_coninfo (cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) (dev->flags & DEV_FLAGS_OUTPUT) ? 'O' : '.');
for (l = 0; l < MAX_FILES; l++) { - if (stdio_devices[l] == dev) { - printf ("%s ", stdio_names[l]); - } + sio = stdio_get_fd(l); + if (sio == dev) + printf("%s ", stdio_names[l]); } putc ('\n'); } diff --git a/common/cmd_terminal.c b/common/cmd_terminal.c index 7cc1a6c..ba34033 100644 --- a/common/cmd_terminal.c +++ b/common/cmd_terminal.c @@ -33,6 +33,7 @@ int do_terminal(cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) { int last_tilde = 0; struct stdio_dev *dev = NULL; + struct stdio_dev *sio;
if (argc < 1) return -1; @@ -46,12 +47,16 @@ int do_terminal(cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) printf("Entering terminal mode for port %s\n", dev->name); puts("Use '~.' to leave the terminal and get back to u-boot\n");
+ sio = stdio_get_dev(stdin); + if (!sio) + return -1; + while (1) { int c;
/* read from console and display on serial port */ - if (stdio_devices[0]->tstc()) { - c = stdio_devices[0]->getc(); + if (sio->tstc()) { + c = sio->getc(); if (last_tilde == 1) { if (c == '.') { putc(c); diff --git a/common/console.c b/common/console.c index df03cef..4a1938a 100644 --- a/common/console.c +++ b/common/console.c @@ -64,7 +64,7 @@ static int console_setfile(int file, struct stdio_dev * dev) }
/* Assign the new device (leaving the existing one started) */ - stdio_devices[file] = dev; + stdio_set_fd(file, dev);
/* * Update monitor functions @@ -170,27 +170,39 @@ static inline void console_doenv(int file, struct stdio_dev *dev) #else static inline int console_getc(int file) { - return stdio_devices[file]->getc(); + struct stdio_dev *dev = stdio_get_fd(file); + if (dev) + return dev->getc(); + return 0; }
static inline int console_tstc(int file) { - return stdio_devices[file]->tstc(); + struct stdio_dev *dev = stdio_get_fd(file); + if (dev) + return dev->tstc(); + return 0; }
static inline void console_putc(int file, const char c) { - stdio_devices[file]->putc(c); + struct stdio_dev *dev = stdio_get_fd(file); + if (dev) + return dev->putc(c); }
static inline void console_puts(int file, const char *s) { - stdio_devices[file]->puts(s); + struct stdio_dev *dev = stdio_get_fd(file); + if (dev) + return dev->puts(s); }
static inline void console_printdevs(int file) { - printf("%s\n", stdio_devices[file]->name); + struct stdio_dev *dev = stdio_get_fd(file); + if (dev) + printf("%s\n", dev->name); }
static inline void console_doenv(int file, struct stdio_dev *dev) @@ -592,27 +604,28 @@ int console_init_f(void) void stdio_print_current_devices(void) { #ifndef CONFIG_SYS_CONSOLE_INFO_QUIET + struct stdio_dev *sio; /* Print information */ puts("In: "); - if (stdio_devices[stdin] == NULL) { + sio = stdio_get_fd(stdin); + if (sio == NULL) puts("No input devices available!\n"); - } else { - printf ("%s\n", stdio_devices[stdin]->name); - } + else + printf("%s\n", sio->name);
puts("Out: "); - if (stdio_devices[stdout] == NULL) { + sio = stdio_get_fd(stdout); + if (sio == NULL) puts("No output devices available!\n"); - } else { - printf ("%s\n", stdio_devices[stdout]->name); - } + else + printf("%s\n", sio->name);
puts("Err: "); - if (stdio_devices[stderr] == NULL) { + sio = stdio_get_fd(stderr); + if (sio == NULL) puts("No error devices available!\n"); - } else { - printf ("%s\n", stdio_devices[stderr]->name); - } + else + printf("%s\n", sio->name); #endif /* CONFIG_SYS_CONSOLE_INFO_QUIET */ }
@@ -622,6 +635,7 @@ int console_init_r(void) { char *stdinname, *stdoutname, *stderrname; struct stdio_dev *inputdev = NULL, *outputdev = NULL, *errdev = NULL; + struct stdio_dev *sio; #ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE int i; #endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */ @@ -690,7 +704,9 @@ done: #ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE /* set the environment variables (will overwrite previous env settings) */ for (i = 0; i < 3; i++) { - setenv(stdio_names[i], stdio_devices[i]->name); + sio = stdio_get_fd(i); + if (sio) + setenv(stdio_names[i], sio->name); } #endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */
@@ -706,7 +722,7 @@ int console_init_r(void) int i; struct list_head *list = stdio_get_list(); struct list_head *pos; - struct stdio_dev *dev; + struct stdio_dev *dev, *sio;
#ifdef CONFIG_SPLASH_SCREEN /* @@ -759,7 +775,9 @@ int console_init_r(void)
/* Setting environment variables */ for (i = 0; i < 3; i++) { - setenv(stdio_names[i], stdio_devices[i]->name); + sio = stdio_get_fd(i); + if (sio) + setenv(stdio_names[i], sio->name); }
return 0; diff --git a/common/fdt_support.c b/common/fdt_support.c index 593f16c..7727359 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -97,7 +97,13 @@ int fdt_find_and_setprop(void *fdt, const char *node, const char *prop, #ifdef CONFIG_SERIAL_MULTI static void fdt_fill_multisername(char *sername, size_t maxlen) { - const char *outname = stdio_devices[stdout]->name; + struct stdio_dev *sio = stdio_get_fd(stdout); + const char *outname; + + if (!sio) + return; + + outname = sio->name;
if (strcmp(outname, "serial") > 0) strncpy(sername, outname, maxlen); diff --git a/common/stdio.c b/common/stdio.c index 6c8ac73..ba5e2fc 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -38,7 +38,7 @@ DECLARE_GLOBAL_DATA_PTR;
static struct stdio_dev devs; -struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL }; +static struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL }; char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" };
#if defined(CONFIG_SPLASH_SCREEN) && !defined(CONFIG_SYS_DEVICE_NULLDEV) diff --git a/include/stdio_dev.h b/include/stdio_dev.h index 86b2a4f..554708a 100644 --- a/include/stdio_dev.h +++ b/include/stdio_dev.h @@ -83,7 +83,6 @@ typedef struct { /* * VARIABLES */ -extern struct stdio_dev *stdio_devices[]; extern char *stdio_names[MAX_FILES];
/*
Dear Marek Vasut,
Use stdio_get_fd() and stdio_set_fd() instead of direct access to the array. This allows making stdio_devices[] local to stdio.c
Signed-off-by: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de
[...]
@@ -622,6 +635,7 @@ int console_init_r(void) { char *stdinname, *stdoutname, *stderrname; struct stdio_dev *inputdev = NULL, *outputdev = NULL, *errdev = NULL;
- struct stdio_dev *sio;
Self-review: NAK! This generates a warning.
Self-reply: V2 on the way.
#ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE int i; #endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */ @@ -690,7 +704,9 @@ done: #ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE /* set the environment variables (will overwrite previous env settings) */ for (i = 0; i < 3; i++) {
setenv(stdio_names[i], stdio_devices[i]->name);
sio = stdio_get_fd(i);if (sio)setenv(stdio_names[i], sio->name);#endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */
[...]
Best regards,
Dear Marek Vasut,
On 01.09.12 00:44, Marek Vasut wrote:
Use stdio_get_fd() and stdio_set_fd() instead of direct access to the array. This allows making stdio_devices[] local to stdio.c
Signed-off-by: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de
common/cmd_console.c | 8 +++---- common/cmd_terminal.c | 9 ++++++-- common/console.c | 60 ++++++++++++++++++++++++++++++++----------------- common/fdt_support.c | 8 ++++++- common/stdio.c | 2 +- include/stdio_dev.h | 1 - 6 files changed, 58 insertions(+), 30 deletions(-)
diff --git a/common/cmd_console.c b/common/cmd_console.c index d8cad6b..343bc28 100644 --- a/common/cmd_console.c +++ b/common/cmd_console.c @@ -34,7 +34,7 @@ int do_coninfo (cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) int l; struct list_head *list = stdio_get_list(); struct list_head *pos;
- struct stdio_dev *dev;
struct stdio_dev *dev, *sio;
/* Scan for valid output and input devices */
@@ -51,9 +51,9 @@ int do_coninfo (cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) (dev->flags & DEV_FLAGS_OUTPUT) ? 'O' : '.');
for (l = 0; l < MAX_FILES; l++) {
if (stdio_devices[l] == dev) {printf ("%s ", stdio_names[l]);}
sio = stdio_get_fd(l);if (sio == dev)printf("%s ", stdio_names[l]);diff --git a/common/cmd_terminal.c b/common/cmd_terminal.c index 7cc1a6c..ba34033 100644 --- a/common/cmd_terminal.c +++ b/common/cmd_terminal.c @@ -33,6 +33,7 @@ int do_terminal(cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) { int last_tilde = 0; struct stdio_dev *dev = NULL;
struct stdio_dev *sio;
if (argc < 1) return -1;
@@ -46,12 +47,16 @@ int do_terminal(cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) printf("Entering terminal mode for port %s\n", dev->name); puts("Use '~.' to leave the terminal and get back to u-boot\n");
sio = stdio_get_dev(stdin);
if (!sio)
return -1;while (1) { int c;
/* read from console and display on serial port */
if (stdio_devices[0]->tstc()) {c = stdio_devices[0]->getc();
if (sio->tstc()) {c = sio->getc(); if (last_tilde == 1) { if (c == '.') { putc(c);diff --git a/common/console.c b/common/console.c index df03cef..4a1938a 100644 --- a/common/console.c +++ b/common/console.c @@ -64,7 +64,7 @@ static int console_setfile(int file, struct stdio_dev * dev) }
/* Assign the new device (leaving the existing one started) */
stdio_devices[file] = dev;
stdio_set_fd(file, dev);/*
- Update monitor functions
@@ -170,27 +170,39 @@ static inline void console_doenv(int file, struct stdio_dev *dev) #else static inline int console_getc(int file) {
- return stdio_devices[file]->getc();
- struct stdio_dev *dev = stdio_get_fd(file);
- if (dev)
return dev->getc();- return 0;
}
static inline int console_tstc(int file) {
- return stdio_devices[file]->tstc();
- struct stdio_dev *dev = stdio_get_fd(file);
- if (dev)
return dev->tstc();- return 0;
}
static inline void console_putc(int file, const char c) {
- stdio_devices[file]->putc(c);
- struct stdio_dev *dev = stdio_get_fd(file);
- if (dev)
return dev->putc(c);
return is not required here ..
}
static inline void console_puts(int file, const char *s) {
- stdio_devices[file]->puts(s);
- struct stdio_dev *dev = stdio_get_fd(file);
- if (dev)
return dev->puts(s);
.. and here
}
static inline void console_printdevs(int file) {
- printf("%s\n", stdio_devices[file]->name);
- struct stdio_dev *dev = stdio_get_fd(file);
- if (dev)
printf("%s\n", dev->name);}
static inline void console_doenv(int file, struct stdio_dev *dev) @@ -592,27 +604,28 @@ int console_init_f(void) void stdio_print_current_devices(void) { #ifndef CONFIG_SYS_CONSOLE_INFO_QUIET
- struct stdio_dev *sio; /* Print information */ puts("In: ");
- if (stdio_devices[stdin] == NULL) {
- sio = stdio_get_fd(stdin);
- if (sio == NULL) puts("No input devices available!\n");
- } else {
printf ("%s\n", stdio_devices[stdin]->name);- }
else
printf("%s\n", sio->name);puts("Out: ");
- if (stdio_devices[stdout] == NULL) {
- sio = stdio_get_fd(stdout);
Isn't sio still set properly here ...
- if (sio == NULL) puts("No output devices available!\n");
- } else {
printf ("%s\n", stdio_devices[stdout]->name);- }
else
printf("%s\n", sio->name);puts("Err: ");
- if (stdio_devices[stderr] == NULL) {
- sio = stdio_get_fd(stderr);
.. and here?
- if (sio == NULL)
Side note: in your newly introduced checks you use just
if (sio) { ...
Why check explicitly for NULL here? I personally favor 'if (!sio)'.
puts("No error devices available!\n");
- } else {
printf ("%s\n", stdio_devices[stderr]->name);- }
- else
printf("%s\n", sio->name);#endif /* CONFIG_SYS_CONSOLE_INFO_QUIET */ }
@@ -622,6 +635,7 @@ int console_init_r(void) { char *stdinname, *stdoutname, *stderrname; struct stdio_dev *inputdev = NULL, *outputdev = NULL, *errdev = NULL;
- struct stdio_dev *sio;
#ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE int i; #endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */ @@ -690,7 +704,9 @@ done: #ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE /* set the environment variables (will overwrite previous env settings) */ for (i = 0; i < 3; i++) {
setenv(stdio_names[i], stdio_devices[i]->name);
sio = stdio_get_fd(i);if (sio)setenv(stdio_names[i], sio->name);#endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */
@@ -706,7 +722,7 @@ int console_init_r(void) int i; struct list_head *list = stdio_get_list(); struct list_head *pos;
- struct stdio_dev *dev;
- struct stdio_dev *dev, *sio;
#ifdef CONFIG_SPLASH_SCREEN /* @@ -759,7 +775,9 @@ int console_init_r(void)
/* Setting environment variables */ for (i = 0; i < 3; i++) {
setenv(stdio_names[i], stdio_devices[i]->name);
sio = stdio_get_fd(i);if (sio)setenv(stdio_names[i], sio->name);}
return 0;
diff --git a/common/fdt_support.c b/common/fdt_support.c index 593f16c..7727359 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -97,7 +97,13 @@ int fdt_find_and_setprop(void *fdt, const char *node, const char *prop, #ifdef CONFIG_SERIAL_MULTI static void fdt_fill_multisername(char *sername, size_t maxlen) {
- const char *outname = stdio_devices[stdout]->name;
struct stdio_dev *sio = stdio_get_fd(stdout);
const char *outname;
if (!sio)
return;outname = sio->name;
if (strcmp(outname, "serial") > 0) strncpy(sername, outname, maxlen);
diff --git a/common/stdio.c b/common/stdio.c index 6c8ac73..ba5e2fc 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -38,7 +38,7 @@ DECLARE_GLOBAL_DATA_PTR;
static struct stdio_dev devs; -struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL }; +static struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL }; char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" };
#if defined(CONFIG_SPLASH_SCREEN) && !defined(CONFIG_SYS_DEVICE_NULLDEV) diff --git a/include/stdio_dev.h b/include/stdio_dev.h index 86b2a4f..554708a 100644 --- a/include/stdio_dev.h +++ b/include/stdio_dev.h @@ -83,7 +83,6 @@ typedef struct { /*
- VARIABLES
*/ -extern struct stdio_dev *stdio_devices[]; extern char *stdio_names[MAX_FILES];
/*
Best regards
Andreas Bießmann
Dear Andreas Bießmann,
Dear Marek Vasut,
Heh, this Dear $recipient became really popular :-)
[...]
return is not required here ..
}
static inline void console_puts(int file, const char *s) {
- stdio_devices[file]->puts(s);
- struct stdio_dev *dev = stdio_get_fd(file);
- if (dev)
return dev->puts(s);.. and here
Thanks!
}
static inline void console_printdevs(int file) {
- printf("%s\n", stdio_devices[file]->name);
- struct stdio_dev *dev = stdio_get_fd(file);
- if (dev)
printf("%s\n", dev->name);}
static inline void console_doenv(int file, struct stdio_dev *dev)
@@ -592,27 +604,28 @@ int console_init_f(void)
void stdio_print_current_devices(void) { #ifndef CONFIG_SYS_CONSOLE_INFO_QUIET
struct stdio_dev *sio;
/* Print information */ puts("In: ");
- if (stdio_devices[stdin] == NULL) {
sio = stdio_get_fd(stdin);
if (sio == NULL)
puts("No input devices available!\n");
- } else {
printf ("%s\n", stdio_devices[stdin]->name);- }
else
printf("%s\n", sio->name);puts("Out: ");
- if (stdio_devices[stdout] == NULL) {
- sio = stdio_get_fd(stdout);
Isn't sio still set properly here ...
It is, but it still can be NULL. Also notice the argument differs, first it's stdin, then stdout and lastly stderr
if (sio == NULL)
puts("No output devices available!\n");
- } else {
printf ("%s\n", stdio_devices[stdout]->name);- }
else
printf("%s\n", sio->name);puts("Err: ");
- if (stdio_devices[stderr] == NULL) {
- sio = stdio_get_fd(stderr);
.. and here?
- if (sio == NULL)
Side note: in your newly introduced checks you use just
if (sio) { ...
Why check explicitly for NULL here? I personally favor 'if (!sio)'.
Compat reason really ... I didn't wanted to introduce more change than necessary ... incremental patching can be done indeed ;-)
puts("No error devices available!\n");
- } else {
printf ("%s\n", stdio_devices[stderr]->name);- }
- else
printf("%s\n", sio->name);
Thanks for the review :)
Best regards
Andreas Bießmann
Dear Marek Vasut,
In message 201209020202.51744.marex@denx.de you wrote:
Dear Andreas Bießmann,
Dear Marek Vasut,
Heh, this Dear $recipient became really popular :-)
If somebody can recommend a more clever rule than "%(friendly {from})" for my nmh replcomps file I would be all too happy! [I've been looking for a way to insert some mapping from some address list here, but failed so far...]
Best regards,
Wolfgang Denk
Add stdio_fd_to_name() function, which convers the STDIO FD number to a proper name.
Signed-off-by: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de --- common/stdio.c | 12 ++++++++++++ include/stdio_dev.h | 1 + 2 files changed, 13 insertions(+)
diff --git a/common/stdio.c b/common/stdio.c index ba5e2fc..0e917fd 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -166,6 +166,18 @@ struct stdio_dev* stdio_clone(struct stdio_dev *dev) return _dev; }
+const char *stdio_fd_to_name(int ioe) +{ + switch (ioe) { + case stdin: + case stdout: + case stderr: + return stdio_names[ioe]; + default: + return NULL; + } +} + int stdio_register (struct stdio_dev * dev) { struct stdio_dev *_dev; diff --git a/include/stdio_dev.h b/include/stdio_dev.h index 554708a..7299735 100644 --- a/include/stdio_dev.h +++ b/include/stdio_dev.h @@ -98,6 +98,7 @@ struct list_head* stdio_get_list(void); struct stdio_dev* stdio_get_by_name(const char* name); struct stdio_dev *stdio_get_fd(int ioe); int stdio_set_fd(int ioe, struct stdio_dev *dev); +const char *stdio_fd_to_name(int ioe); struct stdio_dev* stdio_clone(struct stdio_dev *dev);
#ifdef CONFIG_ARM_DCC_MULTI
The stdio_names variable is no longer exported. Now it's properly constified and accessed via stdio_fd_to_name call.
Signed-off-by: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de --- common/cmd_console.c | 2 +- common/console.c | 4 ++-- common/stdio.c | 6 +++++- include/stdio_dev.h | 5 ----- 4 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/common/cmd_console.c b/common/cmd_console.c index 343bc28..d5ffc69 100644 --- a/common/cmd_console.c +++ b/common/cmd_console.c @@ -53,7 +53,7 @@ int do_coninfo (cmd_tbl_t * cmd, int flag, int argc, char * const argv[]) for (l = 0; l < MAX_FILES; l++) { sio = stdio_get_fd(l); if (sio == dev) - printf("%s ", stdio_names[l]); + printf("%s ", stdio_fd_to_name(l)); } putc ('\n'); } diff --git a/common/console.c b/common/console.c index 4a1938a..22516a5 100644 --- a/common/console.c +++ b/common/console.c @@ -706,7 +706,7 @@ done: for (i = 0; i < 3; i++) { sio = stdio_get_fd(i); if (sio) - setenv(stdio_names[i], sio->name); + setenv(stdio_fd_to_name(i), sio->name); } #endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */
@@ -777,7 +777,7 @@ int console_init_r(void) for (i = 0; i < 3; i++) { sio = stdio_get_fd(i); if (sio) - setenv(stdio_names[i], sio->name); + setenv(stdio_fd_to_name(i), sio->name); }
return 0; diff --git a/common/stdio.c b/common/stdio.c index 0e917fd..64ee94a 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -39,7 +39,11 @@ DECLARE_GLOBAL_DATA_PTR;
static struct stdio_dev devs; static struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL }; -char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" }; +static const char * const stdio_names[MAX_FILES] = { + "stdin", + "stdout", + "stderr" +};
#if defined(CONFIG_SPLASH_SCREEN) && !defined(CONFIG_SYS_DEVICE_NULLDEV) #define CONFIG_SYS_DEVICE_NULLDEV 1 diff --git a/include/stdio_dev.h b/include/stdio_dev.h index 7299735..4e4d7d5 100644 --- a/include/stdio_dev.h +++ b/include/stdio_dev.h @@ -81,11 +81,6 @@ typedef struct { } video_ext_t;
/* - * VARIABLES - */ -extern char *stdio_names[MAX_FILES]; - -/* * PROTOTYPES */ int stdio_register (struct stdio_dev * dev);
Rework the function to be more compact. This results in a minor code size reduction.
Signed-off-by: Marek Vasut marex@denx.de Cc: Wolfgang Denk wd@denx.de --- common/console.c | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/common/console.c b/common/console.c index 22516a5..9558abe 100644 --- a/common/console.c +++ b/common/console.c @@ -604,28 +604,27 @@ int console_init_f(void) void stdio_print_current_devices(void) { #ifndef CONFIG_SYS_CONSOLE_INFO_QUIET + int i; struct stdio_dev *sio; + const struct { + const int fd; + const char * const name; + const char * const type; + } sp[MAX_FILES] = { + { stdin, "In: ", "input", }, + { stdout, "Out: ", "output", }, + { stderr, "Err: ", "error", }, + }; + /* Print information */ - puts("In: "); - sio = stdio_get_fd(stdin); - if (sio == NULL) - puts("No input devices available!\n"); - else - printf("%s\n", sio->name); - - puts("Out: "); - sio = stdio_get_fd(stdout); - if (sio == NULL) - puts("No output devices available!\n"); - else - printf("%s\n", sio->name); - - puts("Err: "); - sio = stdio_get_fd(stderr); - if (sio == NULL) - puts("No error devices available!\n"); - else - printf("%s\n", sio->name); + for (i = 0; i < MAX_FILES; i++) { + sio = stdio_get_fd(sp[i].fd); + if (sio) + printf("%s%s\n", sp[i].name, sio->name); + else + printf("%sNo %s devices available!\n", + sp[i].name, sp[i].type); + } #endif /* CONFIG_SYS_CONSOLE_INFO_QUIET */ }
Dear Marek Vasut,
In message 1346453055-30888-1-git-send-email-marex@denx.de you wrote:
Clean up a few things in STDIO. Mostly, don't export structures directly, but introduce some kind of accessors if needed and remove dead code.
NOTE: I'm still working on the "compile tested on 2 different architectures" part. I'll keep you posted about that. I'd be glad for a review though. I'd hate to pull in logic errors, especially into such critical code. NOTE2: Cross-posting cover to DM list, so I get feedback from those guys.
Marek Vasut (6): stdio: dm: Murder dead code in console.c stdio: dm: Add accessors to stdio_devices[] stdio: dm: Make stdio_devices[] local stdio: dm: Add stdio_fd_to_name() call stdio: dm: Use stdio_fd_to_name() call to localize stdio_names stdio: dm: Optimize stdio_print_current_devices()
I can't make heads nor tails from this patch series.
1) It was posted to the U-Boot list, but all patches carry a "dm:" in the subject, which does not appear to make sense to me, as at least some of the changes have no relation to DM work at all.
2) It appears this might be a RFC series, so why isn't it maked as such in the Subject: ?
3) It appears that some code gets added - what is the impact of these changes on the memory footprint?
4) Besides the dead code removal - what exactly is the purpose of these patches?
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
Dear Marek Vasut,
In message 1346453055-30888-1-git-send-email-marex@denx.de you wrote:
Clean up a few things in STDIO. Mostly, don't export structures directly, but introduce some kind of accessors if needed and remove dead code.
NOTE: I'm still working on the "compile tested on 2 different architectures"
part. I'll keep you posted about that. I'd be glad for a review though. I'd hate to pull in logic errors, especially into such critical code.NOTE2: Cross-posting cover to DM list, so I get feedback from those guys.
Marek Vasut (6): stdio: dm: Murder dead code in console.c stdio: dm: Add accessors to stdio_devices[] stdio: dm: Make stdio_devices[] local stdio: dm: Add stdio_fd_to_name() call stdio: dm: Use stdio_fd_to_name() call to localize stdio_names stdio: dm: Optimize stdio_print_current_devices()
I can't make heads nor tails from this patch series.
- It was posted to the U-Boot list, but all patches carry a "dm:" in the subject, which does not appear to make sense to me, as at least some of the changes have no relation to DM work at all.
They very distantly are. I really needed to clean up the STDIO a bit to familiarize myself with the code I'm soon going to break.
But all in all, I think exporting structures for others to access them as they wish isn't the best of ideas. Therefore I encapsulated these into the file and added accessors. The direction these patches take with STDIO and console.c stuff in U-Boot is such that applying proper encapsulation will allow easier conversion to the driver model stuff later. Yet I'm getting there with really small steps as I need to be very careful here.
- It appears this might be a RFC series, so why isn't it maked as such in the Subject: ?
It's not RFC, why would it be RFC? I'm still working on the "NOTE" part though.
- It appears that some code gets added - what is the impact of these changes on the memory footprint?
So far I tested this on M28:
Before: text data bss dec hex filename 415705 7688 288708 712101 adda5 ./u-boot 11754 788 12 12554 310a ./spl/u-boot-spl
After: text data bss dec hex filename 415590 7676 288700 711966 add1e ./u-boot 11794 788 12 12594 3132 ./spl/u-boot-spl
As you can see, the SPL grows a bit, yet U-Boot shrunk. Tested with Debian GCC 4.7.1 .
- Besides the dead code removal - what exactly is the purpose of these patches?
Mostly see 1).
Best regards,
Wolfgang Denk
Best regards, Marek Vasut
Dear Marek,
In message 201209011619.06260.marex@denx.de you wrote:
NOTE: I'm still working on the "compile tested on 2 different architectures"
...
But all in all, I think exporting structures for others to access them as they wish isn't the best of ideas. Therefore I encapsulated these into the file and added accessors. The direction these patches take with STDIO and console.c stuff in U-Boot is such that applying proper encapsulation will allow easier conversion to the driver model stuff later. Yet I'm getting there with really small steps as I need to be very careful here.
Then please drop the "dm:" part from the commit messages.
- It appears this might be a RFC series, so why isn't it maked as such in the Subject: ?
It's not RFC, why would it be RFC? I'm still working on the "NOTE" part though.
Well, either a patch is ready for submitting (which includes being compile-clean), or it is not - in which case it might still be good enough as a RFC.
You claim these patches are not ready yet, but they are not RFC either. What are they then?
- Besides the dead code removal - what exactly is the purpose of these patches?
Mostly see 1).
Then please document this in the commit message(s).
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
Dear Marek,
In message 201209011619.06260.marex@denx.de you wrote:
NOTE: I'm still working on the "compile tested on 2 different architectures"
...
But all in all, I think exporting structures for others to access them as they wish isn't the best of ideas. Therefore I encapsulated these into the file and added accessors. The direction these patches take with STDIO and console.c stuff in U-Boot is such that applying proper encapsulation will allow easier conversion to the driver model stuff later. Yet I'm getting there with really small steps as I need to be very careful here.
Then please drop the "dm:" part from the commit messages.
I'd like to use it to track what's gonna end up in our patchdrop for the university, hope it's not a problem.
It appears this might be a RFC series, so why isn't it maked as
such in the Subject: ?
It's not RFC, why would it be RFC? I'm still working on the "NOTE" part though.
Well, either a patch is ready for submitting (which includes being compile-clean), or it is not - in which case it might still be good enough as a RFC.
You claim these patches are not ready yet, but they are not RFC either. What are they then?
Something inbetween, still building. Yet, it does seem to be going well.
Besides the dead code removal - what exactly is the purpose of
these patches?
Mostly see 1).
Then please document this in the commit message(s).
Every patch has it's proper commit message (but 1/6, which is obvious).
Best regards,
Wolfgang Denk
Best regards, Marek Vasut
Dear Marek Vasut,
In message 201209011857.18390.marex@denx.de you wrote:
I'd like to use it to track what's gonna end up in our patchdrop for the university, hope it's not a problem.
Please feel free to add a line to the commit message body, but please keep the Subject: clean.
You claim these patches are not ready yet, but they are not RFC either. What are they then?
Something inbetween, still building. Yet, it does seem to be going well.
If not clean, then it's RFC. Please mark as such when posting.
- Besides the dead code removal - what exactly is the purpose of these patches?
Mostly see 1).
Then please document this in the commit message(s).
Every patch has it's proper commit message (but 1/6, which is obvious).
Hm... it didn't become clear to me...
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
Dear Marek Vasut,
In message 201209011857.18390.marex@denx.de you wrote:
I'd like to use it to track what's gonna end up in our patchdrop for the university, hope it's not a problem.
Please feel free to add a line to the commit message body, but please keep the Subject: clean.
OK
You claim these patches are not ready yet, but they are not RFC either. What are they then?
Something inbetween, still building. Yet, it does seem to be going well.
If not clean, then it's RFC. Please mark as such when posting.
ARM just finished building, I'll have V2 for one of the patches, the rest seems OK.
Besides the dead code removal - what exactly is the purpose of
these patches?
Mostly see 1).
Then please document this in the commit message(s).
Every patch has it's proper commit message (but 1/6, which is obvious).
Hm... it didn't become clear to me...
Hm... I'll review and repost.
Yet, it still bothers me to see SPL grow by a few bytes. On the other hand, uboot shrunk:
1316 Configuring for omap5_evm board... 1317 text data bss dec hex filename 1318 - 173007 4312 201912 379231 5c95f ./u-boot 1319 - 32046 1216 197508 230770 38572 ./spl/u-boot-spl 1320 + 172912 4300 201900 379112 5c8e8 ./u-boot 1321 + 32092 1216 197508 230816 385a0 ./spl/u-boot-spl
798 Configuring for da850evm - Board: da850evm, Options: MAC_ADDR_IN_SPIFLASH 799 text data bss dec hex filename 800 - 153161 3476 54260 210897 337d1 ./u-boot 801 - 13867 1228 76 15171 3b43 ./spl/u-boot-spl 802 + 153046 3464 54252 210762 3374a ./u-boot 803 + 13907 1228 76 15211 3b6b ./spl/u-boot-spl
1280 Configuring for devkit8000 board... 1281 text data bss dec hex filename 1282 - 272855 7880 216436 497171 79613 ./u-boot 1283 - 42601 1840 198020 242461 3b31d ./spl/u-boot-spl 1284 + 272744 7868 216424 497036 7958c ./u-boot 1285 + 42647 1840 198020 242507 3b34b ./spl/u-boot-spl
etc.
Best regards,
Wolfgang Denk
Best regards, Marek Vasut
participants (3)
-
Andreas Bießmann -
Marek Vasut -
Wolfgang Denk