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

Nikita Kiryanov
2013-01-21T08:25:20Z

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.