Attention is currently required from: Rory Liu, Paul Menzel, Zhuohao Lee. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61267 )
Change subject: drivers/net/r8168: Add ASPM control mechanism ......................................................................
Patch Set 5: Code-Review+2
(4 comments)
File src/drivers/net/chip.h:
https://review.coreboot.org/c/coreboot/+/61267/comment/654c169a_1b995310 PS4, Line 36: bool enable_aspm;
Should the level be part of the variable name?
Done
File src/drivers/net/r8168.c:
https://review.coreboot.org/c/coreboot/+/61267/comment/3b2f9a17_cbbd9cef PS4, Line 252: struct drivers_net_config *config = dev->chip_info; : if (!config || !config->enable_aspm) : return;
Seeing the function name, I’d move this outside the function before calling it.
Done
https://review.coreboot.org/c/coreboot/+/61267/comment/bab96d51_4ef7b184 PS4, Line 256: printk(BIOS_INFO, "rtl: enable_aspm_L1.2\n");
Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/61267/comment/40535509_5f81d3d1 PS4, Line 363: /* Enable ASPM_L1.2 */
Comment not needed, as the function name says the same.
Done