lcd: Add support for CONFIG_LCD_NOSTDOUT

PATCH New Hannes Petermaier 5 participants 9 messages Mar 6, 2014 – Feb 4, 2015

Share & tools

Hannes Petermaier

author

2014-03-06 14:26 UTC #0 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

- Adds support for CONFIG_LCD_NOSTDOUT, which prevents switching stdout to the LCD screen, usefull in case when only lcd_puts(...), lcd_printf(...) is used for displaying status informations.

Signed-off-by Hannes Petermaier <[email protected]>

---
 README       |    7 +++++++
 common/lcd.c |    9 ++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

README +7 -0

index 216f0c7..4792068 100644
@@ -1702,6 +1702,13 @@ CBFS (Coreboot Filesystem) support
 		Normally display is black on white background; define
 		CONFIG_SYS_WHITE_ON_BLACK to get it inverted.
 
+		CONFIG_LCD_NOSTDOUT
+		Normally 'stdout' is redirected to LCD-screen after
+		initialization. Define CONFIG_LCD_NOSTDOUT to avoid this.
+		Useful in case where only lcd_puts(...), lcd_printf(...)
+		functions of the framework are used and 'normal' u-boot
+		console remains e.g. on serial-line.
+
 		CONFIG_LCD_ALIGNMENT
 
 		Normally the LCD is page-aligned (tyically 4KB). If this is

common/lcd.c +6 -3

index aa81522..eac1b87 100644
@@ -400,12 +400,12 @@ __weak int lcd_get_size(int *line_length)
 
 int drv_lcd_init(void)
 {
-	struct stdio_dev lcddev;
-	int rc;
-
 	lcd_base = (void *) gd->fb_base;
 
 	lcd_init(lcd_base);		/* LCD initialization */
+#ifndef CONFIG_LCD_NOSTDOUT
+	struct stdio_dev lcddev;
+	int rc;
 
 	/* Device initialization */
 	memset(&lcddev, 0, sizeof(lcddev));

@@ -419,6 +419,9 @@ int drv_lcd_init(void)
 	rc = stdio_register(&lcddev);
 
 	return (rc == 0) ? 1 : rc;
+#else
+	return 0;
+#endif
 }
 
 /*----------------------------------------------------------------------*/

Signature · 1 line

1.7.9.5

Gerhard Sittig

2014-03-06 19:49 UTC #1 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

Quoted 9 lines · Hannes Petermaier

common/lcd.c +0 -2

@@ -400,12 +400,12 @@ __weak int lcd_get_size(int *line_length)
 
 int drv_lcd_init(void)
 {
-	struct stdio_dev lcddev;
-	int rc;

common/lcd.c __weak int lcd_get_size(int *line_length) · line 411

Quoted 10 lines

common/lcd.c +3 -1

-
 	lcd_base = (void *) gd->fb_base;
 
 	lcd_init(lcd_base);		/* LCD initialization */
+#ifndef CONFIG_LCD_NOSTDOUT
+	struct stdio_dev lcddev;
+	int rc;
 
 	/* Device initialization */
 	memset(&lcddev, 0, sizeof(lcddev));

What do language lawyers say about declarations after instructions within blocks? This looks somewhat fishy.

common/lcd.c int drv_lcd_init(void) · line 425

Quoted 8 lines

common/lcd.c +3 -0

@@ -419,6 +419,9 @@ int drv_lcd_init(void)
 	rc = stdio_register(&lcddev);
 
 	return (rc == 0) ? 1 : rc;
+#else
+	return 0;
+#endif
 }

This (continuation from the above #ifndef) somehow reads like inverted logic. It appears like "ifdef NOSTDOUT" is a shortcut, not a strict alternative as the patch suggests.

In general U-Boot tries to get away from the multitude of ifdefs where possible. I'm afraid adding a new one needs a very good reason to get perceived as welcome.

virtually yours
Gerhard Sittig

Signature · 3 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

Hannes Petermaier

2014-03-06 20:28 UTC #2 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

On 2014-03-06 20:49, Gerhard Sittig wrote:

Hi Gerhard,

Quoted 17 lines · Hannes Petermaier

Quoted 16 lines

common/lcd.c +3 -3

@@ -400,12 +400,12 @@ __weak int lcd_get_size(int *line_length)
 
  int drv_lcd_init(void)
  {
-	struct stdio_dev lcddev;
-	int rc;
-
  	lcd_base = (void *) gd->fb_base;
 
  	lcd_init(lcd_base);		/* LCD initialization */
+#ifndef CONFIG_LCD_NOSTDOUT
+	struct stdio_dev lcddev;
+	int rc;

Quoted 4 lines

common/lcd.c __weak int lcd_get_size(int *line_length) · line 411
Quoted 2 lines
common/lcd.c +0 -0
  	/* Device initialization */
  	memset(&lcddev, 0, sizeof(lcddev));
What do language lawyers say about declarations after instructions within blocks? This looks somewhat fishy.

Lawyers say, that compiler must not say warnings if the option ist turned on, so it is necessary to do this so.

Quoted 11 lines

common/lcd.c int drv_lcd_init(void) · line 425
Quoted 8 lines
common/lcd.c +3 -0
@@ -419,6 +419,9 @@ int drv_lcd_init(void)
  	rc = stdio_register(&lcddev);
 
  	return (rc == 0) ? 1 : rc;
+#else
+	return 0;
+#endif
  }
This (continuation from the above #ifndef) somehow reads like inverted logic. It appears like "ifdef NOSTDOUT" is a shortcut, not a strict alternative as the patch suggests.

Yes, in fact - this is inverted logic.
Reason for this is, if someone doesn't want this feature, he/she simply doesn't set the #define.
All other applications which are using this code have to do nothing at all to be compatible in the future.
So for my opinion thats a good way to do so.

Quoted 3 lines

In general U-Boot tries to get away from the multitude of ifdefs where possible. I'm afraid adding a new one needs a very good reason to get perceived as welcome.

Okay, i didn't knew about this fact.
I have to check "how early" lcd_drv_init is called and if at this time reading environment variables are allready accessible, so it would be possible to make the behavior environment dependent. Maybe this would also cancel discussion about inverted logic :-)

Quoted 2 lines

virtually yours
Gerhard Sittig

best regards,
Hannes

Hannes Petermaier

2014-03-08 19:46 UTC #3 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

In reply to Gerhard Sittig

Hi Gerhard,

Quoted 3 lines · Gerhard Sittig

In general U-Boot tries to get away from the multitude of ifdefs where possible. I'm afraid adding a new one needs a very good reason to get perceived as welcome.

By the way i've found a passage within README, which tells to do so as id id:

'* If you modify existing code, make sure that your new code does not
add to the memory footprint of the code ;-) Small is beautiful!

When adding new features, these should compile conditionally only (using #ifdef), and the resulting code with the new feature disabled must not need more memory than the old code without your modification.
'
whats your opinion about this ?

Quoted 2 lines

virtually yours
Gerhard Sittig

best regards,
Hannes

Gerhard Sittig

2014-03-10 19:44 UTC #4 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

Quoted 13 lines · Hannes Petermaier

Hi Gerhard,
Quoted 3 lines · Gerhard Sittig
In general U-Boot tries to get away from the multitude of ifdefs where possible. I'm afraid adding a new one needs a very good reason to get perceived as welcome.
By the way i've found a passage within README, which tells to do so as id id:
'* If you modify existing code, make sure that your new code does not
add to the memory footprint of the code ;-) Small is beautiful!

Quoted 6 lines

When adding new features, these should compile conditionally only (using #ifdef), and the resulting code with the new feature disabled must not need more memory than the old code without your modification.
'
whats your opinion about this ?

This policy may not apply here. It's applicable to "larger" chunks of new code, like complete drivers or complex features. Not everyone wants support for unused features in their executables that need to fit in constrained resources. That's well understood.

Your case is different. There is a feature, it's already present and small and unconditional. And your change even shortcuts this small portion. I guess that adding another compiler switch for this may not be appropriate.

But let's see what others have to say. I'm not an expert in policy.

virtually yours
Gerhard Sittig

Signature · 3 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

Jeroen Hofstee

2014-05-03 18:34 UTC #5 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

In reply to Hannes Petermaier

Quoted 7 lines · Hannes Petermaier

- Adds support for CONFIG_LCD_NOSTDOUT, which prevents switching stdout to the LCD screen, usefull in case when only lcd_puts(...), lcd_printf(...) is used for displaying status informations.
Signed-off-by Hannes Petermaier <[email protected]>
---

Perhaps I am missing something, but doesn't 'setenv stdout serial' not already do what you want to achieve?

Regards,
Jeroen

Hannes Petermaier

2014-05-05 06:21 UTC #6 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

Hi Jeroen,

many thanks for answer.

Unfortunately no.

The LCD-framework does overrule this environment settings. I guess the reason for this behaviour is, that environment ist loaded _before_ the lcd-driver is initialized.

best regards,
hannes

Quoted 15 lines · Jeroen Hofstee

Quoted 7 lines · Hannes Petermaier
- Adds support for CONFIG_LCD_NOSTDOUT, which prevents switching stdout to the LCD screen, usefull in case when only lcd_puts(...), lcd_printf(...) is used for displaying status informations.
Signed-off-by Hannes Petermaier <[email protected]>
---
Perhaps I am missing something, but doesn't 'setenv stdout serial' not already do what you want to achieve?
Regards,
Jeroen

Anatolij Gustschin

2014-08-11 13:52 UTC #7 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

In reply to Hannes Petermaier

Hi,

On Thu, 6 Mar 2014 15:26:11 +0100
Hannes Petermaier <[email protected]> wrote:
...

README CBFS (Coreboot Filesystem) support · line 1711

Quoted 7 lines

README +7 -0

+		CONFIG_LCD_NOSTDOUT
+		Normally 'stdout' is redirected to LCD-screen after
+		initialization. Define CONFIG_LCD_NOSTDOUT to avoid this.
+		Useful in case where only lcd_puts(...), lcd_printf(...)
+		functions of the framework are used and 'normal' u-boot
+		console remains e.g. on serial-line.
+

this console redirection to lcd can be disabled by defining

#define CONFIG_SYS_CONSOLE_IS_IN_ENV
#define CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE

in the board config file and adding below function to the board code:

int overwrite_console(void)

{
        return 1;
}

Can you please try it instead of this patch?

Best regards,

Anatolij

Hannes Petermaier

2015-02-04 09:05 UTC #8 lore raw

PATCH 1/1 lcd: Add support for CONFIG_LCD_NOSTDOUT

Quoted 1 line · Anatolij Gustschin

Hi,

Hi,
sorry for my late response because i've been working on some other project for a couple of months.

Quoted 19 lines

On Thu, 6 Mar 2014 15:26:11 +0100
Hannes Petermaier <[email protected]> wrote:
...
README CBFS (Coreboot Filesystem) support · line 1711
Quoted 7 lines
README +7 -0
+		CONFIG_LCD_NOSTDOUT
+		Normally 'stdout' is redirected to LCD-screen after
+		initialization. Define CONFIG_LCD_NOSTDOUT to avoid this.
+		Useful in case where only lcd_puts(...), lcd_printf(...)
+		functions of the framework are used and 'normal' u-boot
+		console remains e.g. on serial-line.
+
this console redirection to lcd can be disabled by defining
#define CONFIG_SYS_CONSOLE_IS_IN_ENV
#define CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE
in the board config file and adding below function to the board code:

Quoted 6 lines

int overwrite_console(void)
{
         return 1;
}
Can you please try it instead of this patch?

Yes - that works for me - so we can close the case. Many thanks.
I will create some patch for the B&R boards.

Quoted 4 lines

Best regards,
Anatolij

best regards,
Hannes