| Message ID | 1394115971-22079-1-git-send-email-oe5hpm@oevsv.at |
|---|---|
| State | Deferred |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
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
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
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
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
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
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 >
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
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
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 } /*----------------------------------------------------------------------*/
- 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(-)