[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[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 <oe5hpm@oevsv.at>
---
 README       |    7 +++++++
 common/lcd.c |    9 ++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/README b/README
index 216f0c7..4792068 100644
--- a/README
+++ b/README
@@ -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
diff --git a/common/lcd.c b/common/lcd.c
index aa81522..eac1b87 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -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
 }
 
 /*----------------------------------------------------------------------*/
-- 
1.7.9.5

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[Gerhard Sittig]

On Thu, Mar 06, 2014 at 15:26 +0100, Hannes Petermaier wrote:
> 
> --- a/common/lcd.c
> +++ b/common/lcd.c
> @@ -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));

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

> @@ -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
-- 
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

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[Hannes Petermaier]

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

Hi Gerhard,
> On Thu, Mar 06, 2014 at 15:26 +0100, Hannes Petermaier wrote:
>> --- a/common/lcd.c
>> +++ b/common/lcd.c
>> @@ -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));
> 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.
>> @@ -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.
> 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 :-)

> virtually yours
> Gerhard Sittig
best regards,
Hannes

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[Hannes Petermaier]

Hi Gerhard,

On 2014-03-06 20:49, Gerhard Sittig wrote:
> 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 ?
> virtually yours
> Gerhard Sittig
best regards,
Hannes

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[Gerhard Sittig]

On Sat, Mar 08, 2014 at 20:46 +0100, Hannes Petermaier wrote:
> 
> Hi Gerhard,
> 
> On 2014-03-06 20:49, Gerhard Sittig wrote:
> >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 ?

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

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[Jeroen Hofstee]

On do, 2014-03-06 at 15:26 +0100, Hannes Petermaier wrote:
> - 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 <oe5hpm@oevsv.at>
> ---
>  

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

Regards,
Jeroen

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[Hannes Petermaier]

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

Jeroen Hofstee wrote:
> On do, 2014-03-06 at 15:26 +0100, Hannes Petermaier wrote:
>> - 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 <oe5hpm@oevsv.at>
>> ---
>>
>
> Perhaps I am missing something, but doesn't 'setenv stdout serial' not
> already do what you want to achieve?
>
> Regards,
> Jeroen
>

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[Anatolij Gustschin]

Hi,

On Thu,  6 Mar 2014 15:26:11 +0100
Hannes Petermaier <oe5hpm@oevsv.at> wrote:
...
> +		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

[U-Boot] [PATCH] lcd: Add support for CONFIG_LCD_NOSTDOUT

[Hannes Petermaier]

On 2014-08-11 15:52, Anatolij Gustschin wrote:
> Hi,
Hi,
sorry for my late response because i've been working on some other 
project for a couple of months.
>
> On Thu,  6 Mar 2014 15:26:11 +0100
> Hannes Petermaier <oe5hpm@oevsv.at> wrote:
> ...
>> +		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?
Yes - that works for me - so we can close the case.
Many thanks.
I will create some patch for the B&R boards.
>
> Best regards,
>
> Anatolij
best regards,
Hannes