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

[Mario Scheithauer (Code Review)]

Mario Scheithauer 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 2:

(16 comments)

PTN3460 is also included in another Siemens mainboard and we should alter this. But I would do this in a following patch.
Thanks all for the review!

https://review.coreboot.org/#/c/19043/1//COMMIT_MSG
Commit Message:

PS1, Line 13: ata_
> EDID?
Done


https://review.coreboot.org/#/c/19043/1/src/mainboard/siemens/mc_apl1/mainboard.c
File src/mainboard/siemens/mc_apl1/mainboard.c:

PS1, Line 116: t status;
> Maybe move that into the function?
Done


PS1, Line 120:  * ptn3460_init() 
> I would completely remove lcd_panel.{c,h} and move the call to ptn3460_init
Changed to direct call of ptn3460_init("hwinfo.hex")


https://review.coreboot.org/#/c/19043/1/src/mainboard/siemens/mc_apl1/ptn3460.c
File src/mainboard/siemens/mc_apl1/ptn3460.c:

PS1, Line 24: function 
> function
Done


Line 28:  * @return CB_SUCCESS on success or error code.
> We have CB_SUCCESS and CB_ERROR defined in `src/include/types.h`.
Done


PS1, Line 44:  size
> sizeof(edid_data)
changed to sizeof(edid_data)


PS1, Line 61: Her
> is
Done


PS1, Line 72: PTN_CONFIG_OFF
> This is 0x19 bytes == 24 bytes.  You are smashing your stack since sizeof(s
changed to sizeof(cfg) here and in the following


PS1, Line 75: 
> skip the s
Done


PS1, Line 92: /* 250 ms from LVD
> Please increase this value to 10 resulting in 500 ms backlight delay. There
Done


PS1, Line 85: 
            : 	/* No clock spreading, 300 mV LVDS swing. */
            : 	cfg.lvds_interface_ctrl2 = 0x03;
            : 	/* No LVDS signal swap. */
            : 	cfg.lvds_interface_ctrl3 = 0x00;
            : 	/* Delay T2 (VDD to LVDS active) by 16 ms. */
            : 	cfg.t2_delay = 1;
            : 	/* 250 ms from LVDS to backlight active. */
            : 	cfg.t3_timing = 10;
            : 	/* 1 second re-power delay. */
            : 	cfg.t12_timing = 20;
            : 	/* 150 ms backlight off to LVDS inactive. */
            : 	cfg.t4_timing = 3;
            : 	/* Delay T5 (LVDS to VDD inactive) by 16 ms. */
            : 	cfg.t5_delay = 1;
            : 	/* Enable backlight con
> I guess this could be made configurable in the devicetree in the future?
Yes, you are right. I would do this as part of the PTN driver implementation in a further patch.


PS1, Line 106: PTN_CONFI
> Cast not needed here as you already made it a uint8_t type.
Yes, i2c_write_bytes isn’t available.


PS1, Line 141: atus);
> Extended Display Identification Data (EDID)
I changed this into function description.


PS1, Line 154: 
> DisplayPort
Done


https://review.coreboot.org/#/c/19043/1/src/mainboard/siemens/mc_apl1/ptn3460.h
File src/mainboard/siemens/mc_apl1/ptn3460.h:

PS1, Line 28: N	0x
> Aren't you assuming this is ptn_3460_config? If so, then this should be siz
deleted the define


PS1, Line 90: id(u8 
> This should be const char *.
Done


-- 
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: 2
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: Mario Scheithauer <mario.scheithauer at siemens.com>
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