[M] Change in coreboot[master]: drivers/phy/m88e1512: Provide functionality to customize LED status
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 3:
(4 comments)
File src/drivers/phy/m88e1512/chip.h:
https://review.coreboot.org/c/coreboot/+/69386/comment/961b08ad_4a9db253 PS2, Line 7: led_ctrl
something like that?
Yes, looks better IMHO.
File src/drivers/phy/m88e1512/m88e1512.h:
https://review.coreboot.org/c/coreboot/+/69386/comment/ab3794bf_a7df6539 PS3, Line 7: PHY_MDIO_PAGE_ANY_ADR_RE This name is a bit misleading. In the linked datasheet this register is called 'Page Address'. The 'Page Any' note just tries to make clear, that this register is available on 'any' page. In addition, all the registers accessed here are on the MDIO interface. So adding this to the register defines does not provide additional information but only make the name longer. What about: #define PHY_PAGE_REG 0x16
I am still undecided if we really need the page defines. It is just a 1:1 mapped number... What dou you think?
Accordingly, the LED contrl register would then be PHY_LED_FUNC_CTRL_REG
We could even get rid of the PHY prefix, but this is up to you to decide.
File src/drivers/phy/m88e1512/m88e1512.c:
https://review.coreboot.org/c/coreboot/+/69386/comment/308ef253_831e36d2 PS2, Line 22: dev->bus->dev->device
A guess the PCI path would be more helpfull here than the device ID, or?
Ack
File src/drivers/phy/m88e1512/m88e1512.c:
https://review.coreboot.org/c/coreboot/+/69386/comment/5500e4dc_7dd9ff8d PS3, Line 22: ... Just one '.' should be suitable here.
-
Werner Zeh (Code Review)