Attention is currently required from: Mario Scheithauer.
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69386 )
Change subject: drivers/phy/m88e1512: Provide functionality to customize LED status ......................................................................
Patch Set 2:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/69386/comment/ccc3653a_2e1aa198 PS2, Line 14: https://www.marvell.com/content/dam/marvell/en/public-collateral/phys-transc... I have pushed this datasheet to archive.org: https://web.archive.org/web/20221109080111/https://www.marvell.com/content/d...
Let's use this link here instead of the direct Marvell link to provide long-time stability.
File src/drivers/phy/m88e1512/chip.h:
https://review.coreboot.org/c/coreboot/+/69386/comment/37f1c434_0bee4db4 PS2, Line 6: led_customize Maybe 'configure_leds' instead? And since this is just a flag, should it be bool then?
https://review.coreboot.org/c/coreboot/+/69386/comment/71a5c1e7_7024434b PS2, Line 7: led_ctrl The LED control register includes a LED[2] control field as well. As this aims to be a generic driver I would vote for adding full access to all the LED control fields via devicetree.
And for the sake of readability I would split it into three different fields here, one for each LED.
File src/drivers/phy/m88e1512/m88e1512.c:
https://review.coreboot.org/c/coreboot/+/69386/comment/5c41f463_f230e46d PS2, Line 17: dev->bus->dev To increase readability a dedicated device pointer (something like parent or similar) would be helpful here. Then you can simply do 'parent->device' or ' mdio_ops->read(parent, ...)
https://review.coreboot.org/c/coreboot/+/69386/comment/8757f012_b8949443 PS2, Line 22: dev->bus->dev->device A guess the PCI path would be more helpfull here than the device ID, or?
https://review.coreboot.org/c/coreboot/+/69386/comment/89b06fc4_fb41a1c7 PS2, Line 35: Select page back to 0. 'Switch back to page 0.'