[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