Maciej Matuszczyk 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 50:
(19 comments)
https://review.coreboot.org/c/coreboot/+/35864/7//COMMIT_MSG Commit Message:
PS7:
Please obey the 72 chars per line rule of Git.
Done
https://review.coreboot.org/c/coreboot/+/35864/7//COMMIT_MSG@9 PS7, Line 9: - It can be 100 % Open Source.
Please clarify what `It` is in this context. I suppose `coreboot` if […]
Done
https://review.coreboot.org/c/coreboot/+/35864/7//COMMIT_MSG@12 PS7, Line 12: black on 1400x1050 IPS display[1]. Display works fine on Linux. I don't why it appears like that.
Please mention that this is with native graphics init. […]
Yes it is
https://review.coreboot.org/c/coreboot/+/35864/40//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35864/40//COMMIT_MSG@23 PS40, Line 23: Maccraft
Done
Done
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. […]
Done
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.
Yep. […]
Yes. Happens regardless of this setting
https://review.coreboot.org/c/coreboot/+/35864/43/Documentation/mainboard/le... File Documentation/mainboard/lenovo/r60.md:
PS43:
Well it is. […]
Done
https://review.coreboot.org/c/coreboot/+/35864/16/src/mainboard/lenovo/t60/K... File src/mainboard/lenovo/t60/Kconfig:
https://review.coreboot.org/c/coreboot/+/35864/16/src/mainboard/lenovo/t60/K... PS16, Line 43: default "variants/t60/overridetree.cb" if BOARD_LENOVO_R60 : # R60 uses same files as T60 except gpio.c : default "variants/$(CONFIG_VARIANT_DIR)/overridetree.cb"
just make it explicit: default "variant/t60/overridetree. […]
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/35864/1/src/mainboard/lenovo/t60/va... File src/mainboard/lenovo/t60/variants/r60/gpio.c:
https://review.coreboot.org/c/coreboot/+/35864/1/src/mainboard/lenovo/t60/va... PS1, Line 82: * SMB_3B_EN */
According to my schematics, GPIO34 is output, and connects to SMB_3B_EN through R814
Done
https://review.coreboot.org/c/coreboot/+/35864/1/src/mainboard/lenovo/t60/va... PS1, Line 102: .gpio33 = GPIO_LEVEL_LOW, : .gpio35 = GPIO_LEVEL_HIGH,
oops
Done
https://review.coreboot.org/c/coreboot/+/35864/16/src/mainboard/lenovo/t60/v... File src/mainboard/lenovo/t60/variants/r60/gpio.c:
https://review.coreboot.org/c/coreboot/+/35864/16/src/mainboard/lenovo/t60/v... PS16, Line 1: /* : * This file is part of the coreboot project. : * : * Copyri
neither of these variant/*/gpio.c files are linked...
Done
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 […]
Done
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... PS20, Line 30: .gpio21 = GPIO_MODE_GPIO, /* LCD_PRESENCE */
IMO this would decrease readibility to someone else. […]
Done
https://review.coreboot.org/c/coreboot/+/35864/20/src/mainboard/lenovo/t60/v... PS20, Line 30: .gpio21 = GPIO_MODE_GPIO, /* LCD_PRESENCE */
just #if. […]
Done
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?
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/35864/10/src/mainboard/lenovo/t60/v... File src/mainboard/lenovo/t60/variants/t60/gpio.c:
PS10:
Did you want to change this file?
Done
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.
Done