[coreboot-gerrit] Change in coreboot[master]: siemens/mc_apl1: Activate PTN3460 eDP to LVDS bridge IC

[Werner Zeh (Code Review)]

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/Makefile.inc
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_panel.c
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_panel.h
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/mainboard.c
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/ptn3460.c
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.


-- 
To view, visit https://review.coreboot.org/19043
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8fa79bb608f1842f26c1af3d7bf4bb0513fa94d
Gerrit-PatchSet: 1
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Mario Scheithauer <mario.scheithauer at siemens.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Werner Zeh <werner.zeh at siemens.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes