From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jeroen Hofstee <jeroen@myspectrum.nl>
Date: Wed, 23 Jan 2013 23:36:23 +0100
Subject: [U-Boot] [PATCH 4/5] cm-t35: add support for user defined lcd
 parameters
In-Reply-To: <50FCFB70.5030800@compulab.co.il>
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> <50FCFB70.5030800@compulab.co.il>
Message-ID: <510065E7.50507@myspectrum.nl>
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 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....).

Regards,
Jeroen