From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nikita Kiryanov <nikita@compulab.co.il>
Date: Mon, 21 Jan 2013 10:25:20 +0200
Subject: [U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd
 parameters
In-Reply-To: <50FC5CEB.7060908@myspectrum.nl>
References: <1356246228-26732-1-git-send-email-nikita@compulab.co.il>
	<1356246228-26732-5-git-send-email-nikita@compulab.co.il>
	<50FC5CEB.7060908@myspectrum.nl>
Message-ID: <50FCFB70.5030800@compulab.co.il>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Jeroen,

On 01/20/2013 11:08 PM, Jeroen Hofstee wrote:
> On 12/23/2012 08:03 AM, Nikita Kiryanov wrote:
[...]
>> + * Returns -1 on failure, 0 on success.
>> + */
>> +static int parse_customlcd(char *custom_lcd_params)
>> +{
>> +    char params_cpy[160];
>> +    char *setting;
>> +
>> +    strncpy(params_cpy, custom_lcd_params, 160);
> I fail to understand why you want to copy this.

strtok modifies the string it operates on. The documentation for
getenv states that you must not modify the string it returns.

>> +    setting = strtok(params_cpy, ",");
>> +    while (setting) {
>> +        if (parse_setting(setting) < 0)
>> +            return -1;
>> +
>> +        setting = strtok(NULL, ",");
>> +    }
>> +
>> +    /* Currently we don't support changing this via custom lcd params */
>> +    panel_cfg.data_lines = LCD_INTERFACE_24_BIT;
>> +
> again, if you only support 24 panels, why not drive them as such?

Can you please elaborate on this comment? I'm not entirely sure what
inconsistencies you are referring to.

>> +    return 0;
>> +}
>> +
> Is above really board specific or should it be in omap_videomodes.c or
> whatever?

Well, most of it is parsing for a custom feature, so I would say this is
board specific.

>> +/*
>>    * env_parse_displaytype() - parse display type.
>>    *
>>    * Parses the environment variable "displaytype", which contains the
>> @@ -176,14 +378,19 @@ void lcd_ctrl_init(void *lcdbase)
>>   {
>>       struct dispc_regs *dispc = (struct dispc_regs *)OMAP3_DISPC_BASE;
>>       struct prcm *prcm = (struct prcm *)PRCM_BASE;
>> +    char *custom_lcd;
>>       char *displaytype = getenv("displaytype");
>>       if (displaytype == NULL)
>>           return;
>>       lcd_def = env_parse_displaytype(displaytype);
>> -    if (lcd_def == NONE)
>> -        return;
>> +    /* If we did not recognize the preset, check if it's an env
>> variable */
>> +    if (lcd_def == NONE) {
>> +        custom_lcd = getenv(displaytype);
>> +        if (custom_lcd == NULL || parse_customlcd(custom_lcd) < 0)
>> +            return;
>> +    }
>>       panel_cfg.frame_buffer = lcdbase;
>>       omap3_dss_panel_config(&panel_cfg);
>> @@ -204,7 +411,7 @@ void lcd_ctrl_init(void *lcdbase)
>>   void lcd_enable(void)
>>   {
>> -    if (lcd_def == DVI) {
>> +    if (lcd_def == DVI || lcd_def == DVI_CUSTOM) {
>>           gpio_direction_output(54, 0); /* Turn on DVI */
>>           omap3_dss_enable();
>>       }
> Regards,
> Jeroen
>


-- 
Regards,
Nikita.