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/mainbo... 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/ptn346... 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/ptn346... 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
-
Mario Scheithauer (Code Review)