Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36093 )
Change subject: mb/lenovo/x200: Add ThinkPad X301 as a variant ......................................................................
Patch Set 3:
(8 comments)
Looks good!
https://review.coreboot.org/c/coreboot/+/36093/3/Documentation/mainboard/len... File Documentation/mainboard/lenovo/x301.md:
https://review.coreboot.org/c/coreboot/+/36093/3/Documentation/mainboard/len... PS3, Line 7: x301_kb_removed Add something more complete: "X301 with WSON8 chip replaces with SOIC8 chip"
https://review.coreboot.org/c/coreboot/+/36093/3/Documentation/mainboard/len... PS3, Line 20: Otherwise, it may be recommended to replace it with a : SOIC-8 one. You should mention that you might need to add the chip to the IFD VSCC list, https://review.coreboot.org/c/coreboot/+/27860
https://review.coreboot.org/c/coreboot/+/36093/3/Documentation/mainboard/len... PS3, Line 39: : [T420 / T520 / X220 / T420s / W520 common]: xx20_series.md Stale comment?
https://review.coreboot.org/c/coreboot/+/36093/3/Documentation/mainboard/len... File Documentation/mainboard/lenovo/x301_kb_removed.jpg:
PS3: Reduce image size to about 100KiB
https://review.coreboot.org/c/coreboot/+/36093/3/src/mainboard/lenovo/x200/M... File src/mainboard/lenovo/x200/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36093/3/src/mainboard/lenovo/x200/M... PS3, Line 16: variants/$(VARIANT_DIR)/dock.c I think defining a Kconfig in the mainboard Kconfig file "HAS_DOCK_CONNECTOR" and conditionally including it makes more sense.
https://review.coreboot.org/c/coreboot/+/36093/3/src/mainboard/lenovo/x200/M... PS3, Line 18: blc.c You might want to add your EDID string to this list to have a properly 'tuned' backlight pwm frequency. Typically 100-200Hz is fine for CCFL backlit panels, while with LED backlit you want it as high as possible without it flickering or producing noise. With LED backlight you can expect 300-1000Hz to work nicely.
https://review.coreboot.org/c/coreboot/+/36093/3/src/mainboard/lenovo/x200/b... File src/mainboard/lenovo/x200/board_info.txt:
https://review.coreboot.org/c/coreboot/+/36093/3/src/mainboard/lenovo/x200/b... PS3, Line 2: ThinkPad X200 baseboard Thinkpad X200/X200T/X200S/X301
https://review.coreboot.org/c/coreboot/+/36093/3/src/southbridge/intel/i8280... File src/southbridge/intel/i82801ix/nvs.h:
https://review.coreboot.org/c/coreboot/+/36093/3/src/southbridge/intel/i8280... PS3, Line 137: global_nvs_t This reminds me that this has to go to the sb dir.