Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35864 )
Change subject: mb/lenovo/{t60,r60}: Add ThinkPad R60 support as variant board ......................................................................
Patch Set 20:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35864/20/Documentation/mainboard/le... File Documentation/mainboard/lenovo/r60.md:
https://review.coreboot.org/c/coreboot/+/35864/20/Documentation/mainboard/le... PS20, Line 2: : https://imgur.com/a/0wpMGsm Please remove links to image and include directly. I don't think this one is very useful?
https://review.coreboot.org/c/coreboot/+/35864/20/Documentation/mainboard/le... PS20, Line 36: - Several pixel black bar at the left side of screen. Doesn't appear in Linux. See picture. Does this also happen in native res instead of textmode?
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/M... File src/mainboard/lenovo/t60/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/M... PS20, Line 16: subdirs-y += variants/r60/ : subdirs-y += variants/t60/ : subdirs-y += variants/z61t/ Only add the dir you want to link.
IMO it just make it explicit which file you want to link instead of going through this variant dir Makefile.
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... File src/mainboard/lenovo/t60/variants/r60/gpio.c:
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... PS20, Line 30: .gpio21 = GPIO_MODE_GPIO, /* LCD_PRESENCE */ If only this and gpio24 are different, I think you can just use the preprocessor to do that and keep a single file
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... File src/mainboard/lenovo/t60/variants/t60/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... PS20, Line 1: bootblock-y += gpio.c why?
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... File src/mainboard/lenovo/t60/variants/t60/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... PS20, Line 2: ## This file is part of the coreboot project. this file is unused.
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... File src/mainboard/lenovo/t60/variants/t60/gpio.c:
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... PS20, Line 1: /* The gpio.c file in the basedir is still there.