[U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd parameters

[Igor Grinberg]

On 01/24/13 00:36, Jeroen Hofstee wrote:
> Hi Nikita,
> 
> On 01/21/2013 09:25 AM, Nikita Kiryanov wrote:
>> 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.
>>
> that seems to make sense...
>>>> +    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.
> You're driving only 24 bit panels, yet you want the software to drive 16 bit panels.
> Why not keep the software frame buffer at 32 bits?
>>
>>>> +    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.
> I can't understand how custom settings can be board specific, unless you
> mess with timer of course (but even then....).

I fail to understand which timer are you talking about...
Probably, you mean the DPLLs and the clocks?

-- 
Regards,
Igor.