Patrick Georgi 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:
(21 comments)
https://review.coreboot.org/c/coreboot/+/35864/40//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35864/40//COMMIT_MSG@2 PS40, Line 2: Maccraft
Please use your real name or a pseudonym.
Ack
https://review.coreboot.org/c/coreboot/+/35864/40//COMMIT_MSG@23 PS40, Line 23: Maccraft
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/35864/23/.tmpconfig.lintmiTEi9 File .tmpconfig.lintmiTEi9:
PS23:
Get rid of this.
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 16: #
Should I include link or credit the creator? […]
Done
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... File Documentation/mainboard/lenovo/r60.md:
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... PS23, Line 1: ThinkPad
Thinkpad
Done
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... PS23, Line 3: 
I'm still not convinced this image is useful.
Done
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... PS23, Line 4: : Untested on boards with external Radeon graphics adapter. If you have such board, proceed at your own risk and document if it does work. It should work, but no one tested it yet.
You're saying everything twice here...
Done
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... PS23, Line 11: The flash IC is located at the bottom center of the mainboard. Access to the flash chip is blocked by the magnesium frame, so you need to disassemble the entire laptop and remove the mainboard.
Note: a Picture of the flash IC's location would be useful. […]
Ack
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... PS23, Line 18: I
This is documentation, not a blog.
Done
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... PS23, Line 17: : I have flashed it correctly using this method: : https://gist.github.com/ArthurHeymans/c5ef494ada01af372735f237f6c6adbe : In config use R60 board instead of X60
something like "This [method](https://.... […]
Done
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... PS23, Line 24: After installing coreboot all lenovo bios flashing restrictions are lifted. : Flashrom is able to flash updated coreboot.rom without any problems.
I'd just state that by default coreboot does not feature flash restrictions like the vendor firmware […]
Done
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... PS23, Line 25: updated coreboot.rom
"any rom"
Done
https://review.coreboot.org/c/coreboot/+/35864/26/Documentation/mainboard/le... File Documentation/mainboard/lenovo/r60.md:
https://review.coreboot.org/c/coreboot/+/35864/26/Documentation/mainboard/le... PS26, Line 22: installation
configuration
Done
https://review.coreboot.org/c/coreboot/+/35864/43/Documentation/mainboard/le... File Documentation/mainboard/lenovo/r60.md:
https://review.coreboot.org/c/coreboot/+/35864/43/Documentation/mainboard/le... PS43, Line 28: - Suspend and resume.
In GNU/Linux?
Done
https://review.coreboot.org/c/coreboot/+/35864/43/Documentation/mainboard/le... PS43, Line 30: - GRUB2 and SeaBIOS payloads.
What versions?
Done
https://review.coreboot.org/c/coreboot/+/35864/43/Documentation/mainboard/le... PS43, Line 27: - Intel WiFi card. : - Suspend and resume. : - Native graphics initialization. Both legacy VGA and linear framebuffer work. : - GRUB2 and SeaBIOS payloads. : - Reflashing with flashrom (use flashrom-git as of 17.09.2019). : - 2G+1G memory configuration working.
Please remove the dots/periods at the end.
Done
https://review.coreboot.org/c/coreboot/+/35864/43/Documentation/mainboard/le... PS43, Line 36: Linux
Linux 5. […]
Done
https://review.coreboot.org/c/coreboot/+/35864/43/Documentation/mainboard/le... PS43, Line 37: picture
Please link to the picture.
Done
https://review.coreboot.org/c/coreboot/+/35864/23/Documentation/mainboard/le... File Documentation/mainboard/lenovo/r60.png:
PS23:
2M is way too large. […]
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 27: config DEVICETREE : string : default "variants/$(CONFIG_VARIANT_DIR)/devicetree.cb"
This makes no sense if you use override trees.
Done
https://review.coreboot.org/c/coreboot/+/35864/40/src/mainboard/lenovo/t60/g... File src/mainboard/lenovo/t60/gpio.c:
https://review.coreboot.org/c/coreboot/+/35864/40/src/mainboard/lenovo/t60/g... PS40, Line 30: .gpio19 = GPIO_MODE_GPIO, /* GBE_RS# */
These comments only add what gpio does what if it isn't native function. […]
Ack