stdio: Clean up

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 <[email protected]>

Cc Wolfgang Denk <[email protected]>

common/console.c +0 -0

 #ifdef CONFIG_SYS_CONSOLE_ENV_OVERWRITE
 	int i;
 #endif /* CONFIG_SYS_CONSOLE_ENV_OVERWRITE */

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 <[email protected]>

Cc Wolfgang Denk <[email protected]>

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

common/cmd_console.c +4 -4

index d8cad6b..343bc28 100644
@@ -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');
 	}

common/cmd_terminal.c +7 -2

index 7cc1a6c..ba34033 100644
@@ -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);

common/console.c +7 -3

index df03cef..4a1938a 100644
@@ -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)

common/console.c +6 -2

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

common/console.c +10 -6

 		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;

common/fdt_support.c +7 -1

index 593f16c..7727359 100644
@@ -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);

common/stdio.c +1 -1

index 6c8ac73..ba5e2fc 100644
@@ -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)

include/stdio_dev.h +0 -0

index 86b2a4f..554708a 100644

Dear Marek Vasut,

return is not required here ..

Quoted 5 lines

}

static inline void console_puts(int file, const char *s) {

common/console.c static inline void console_doenv(int file, struct stdio_dev *dev) · line 198

Quoted 4 lines

common/console.c +3 -1

-	stdio_devices[file]->puts(s);
+	struct stdio_dev *dev = stdio_get_fd(file);
+	if (dev)
+		return dev->puts(s);

.. and here

Quoted 37 lines

}

static inline void console_printdevs(int file)
{

common/console.c +8 -5

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

common/console.c int console_init_f(void) · line 617

Quoted 4 lines

common/console.c +1 -1

 	puts("Out:   ");
 
-	if (stdio_devices[stdout] == NULL) {
+	sio = stdio_get_fd(stdout);

Isn't sio still set properly here ...

Quoted 10 lines

common/console.c +3 -3

+	if (sio == NULL)
 
 		puts("No output devices available!\n");
 
-	} else {
-		printf ("%s\n", stdio_devices[stdout]->name);
-	}
+	else
+		printf("%s\n", sio->name);
 

common/console.c int console_init_f(void) · line 624

Quoted 4 lines

common/console.c +1 -1

 	puts("Err:   ");
 
-	if (stdio_devices[stderr] == NULL) {
+	sio = stdio_get_fd(stderr);

.. and here?

Review

Quoted 1 line

(quoted diff) +1 -0

+	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)'.

common/console.c int console_init_f(void) · line 628

Quoted 7 lines

common/console.c +2 -3

 		puts("No error devices available!\n");
 
-	} else {
-		printf ("%s\n", stdio_devices[stderr]->name);
-	}
+	else
+		printf("%s\n", sio->name);

Best regards

Andreas Bie?mann

Dear Andreas Bie?mann,

Quoted 1 line

Dear Marek Vasut,

Heh, this Dear $recipient became really popular :-)

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

Dear Marek Vasut,

Quoted 12 lines · In message <[email protected]> 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.

Quoted 7 lines

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

Quoted 3 lines

Quoted 3 lines

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.

Quoted 2 lines

2) 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.

Quoted 2 lines

4) Besides the dead code removal - what exactly is the purpose of these patches?

Mostly see 1).

Dear Marek,

Quoted 2 lines · In message <[email protected]> you wrote:

Quoted 2 lines

Quoted 2 lines

NOTE: I'm still working on the "compile tested on 2 different architectures"

...

Quoted 7 lines

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.

Quoted 6 lines

Quoted 3 lines

2) 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?

Quoted 5 lines

Quoted 3 lines

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

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

4) 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).

Dear Marek Vasut,

Quoted 2 lines · In message <[email protected]> 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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

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