Werner Zeh has posted comments on this change. ( https://review.coreboot.org/19043 )
Change subject: siemens/mc_apl1: Activate PTN3460 eDP to LVDS bridge IC ......................................................................
Patch Set 1:
(8 comments)
Let's get rid of lcd_panel.c and lcd_panel.h, they are not needed in your case.
https://review.coreboot.org/#/c/19043/1/src/mainboard/siemens/mc_apl1/Makefi... File src/mainboard/siemens/mc_apl1/Makefile.inc:
PS1, Line 11: ramstage-y += lcd_panel.c I would drop lcd_panel.c completely while moving the init function for ptn3460 to mainboard_final.
https://review.coreboot.org/#/c/19043/1/src/mainboard/siemens/mc_apl1/lcd_pa... File src/mainboard/siemens/mc_apl1/lcd_panel.c:
PS1, Line 34: strcpy(blockname, "hwinfo.hex"); You can skip this step as you only have one hwinfo block. You can simply use ptn3460_init("hwinfo.hex"). Thats it!
Line 38: printk(BIOS_ERR, "LCD: Setup PTN with status 0x%x\n", status);
Isn’t the status now 0?
If the status is _not_ 0, there was an error. This needs to be shown on the console with BIOS_ERR level. If status is 0, then the level would be BIOS_INFO.
https://review.coreboot.org/#/c/19043/1/src/mainboard/siemens/mc_apl1/lcd_pa... File src/mainboard/siemens/mc_apl1/lcd_panel.h:
PS1, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2014-2017 Siemens AG : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ : : #ifndef _LCD_PANEL_H_ : #define _LCD_PANEL_H_ : : /* This GPIOs are used for LCD panel type encoding. */ : #define LCD_TYPE_GPIO_BIT0 40 : #define LCD_TYPE_GPIO_BIT1 41 : #define LCD_TYPE_GPIO_BIT2 42 : #define LCD_TYPE_GPIO_BIT3 43 : : #define LCD_PANEL_TYPE_10_INCH 4 : #define LCD_PANEL_TYPE_12_INCH 7 : #define LCD_PANEL_TYPE_15_INCH 6 : #define LCD_PANEL_TYPE_19_INCH 1 : #define LCD_PANEL_TYPE_EDID 15 : : int setup_lcd_panel(void); : : #endif /* _LCD_PANEL_H_ */ Drop this file completely.
https://review.coreboot.org/#/c/19043/1/src/mainboard/siemens/mc_apl1/mainbo... File src/mainboard/siemens/mc_apl1/mainboard.c:
PS1, Line 120: setup_lcd_panel(); I would completely remove lcd_panel.{c,h} and move the call to ptn3460_init() to mainbaord_final. As you only have one possible LCD panel, there is no need for a panel selection at all.
https://review.coreboot.org/#/c/19043/1/src/mainboard/siemens/mc_apl1/ptn346... File src/mainboard/siemens/mc_apl1/ptn3460.c:
PS1, Line 75: s skip the s
PS1, Line 92: cfg.t3_timing = 5; Please increase this value to 10 resulting in 500 ms backlight delay. There are LCD panels out there which might need this longer time to avoid flicker.
Line 127: int ptn3460_write_edid(u8 edid_num, u8 *data)
How do you know the data is right length?
Actually this function is responsible for writing a complete EDID data set to the PTN and the length of the EDID data set is fixed to 128 bytes per definition. So the idea here is: the caller needs to ensure that the pointer passed in points to a proper buffer.