Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )
Change subject: superio/nuvoton/nct5104d: Add soft reset GPIO functionality ......................................................................
Patch Set 3:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@7 PS1, Line 7: src/
Remove `src/` from the prefix.
Done
https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@11 PS1, Line 11: GPIOS
GPIOs
Done
https://review.coreboot.org/c/coreboot/+/35482/1//COMMIT_MSG@11 PS1, Line 11: uknown
missing an n
Done
https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/nct5104d.h:
https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... PS1, Line 24: #define LDN_SELECT_CR07 0x07
unneeded; see my comments on the other file
Done
https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... PS1, Line 42: #define NCT5104D_GPIO 0x07 /* GPIO In-Out configuration */
already defined in line 50
Done
https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... PS1, Line 113: pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO);
just add the LDN 0xf to the device's devicetree then
Done in a subsequent patch
https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... PS1, Line 117: pnp_write_config(dev, LDN_SELECT_CR07, NCT5104D_GPIO_PP_OD);
same here
Done
https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... PS2, Line 23: nt gpio0_enabled = 0; : int gpio1_enabled = 0; : int gpio6_enabled = 0;
this just duplicates information from the devicetree
Done
https://review.coreboot.org/c/coreboot/+/35482/2/src/superio/nuvoton/nct5104... PS2, Line 142: : if (gpio0_enabled) : pnp_write_config(dev, NCT5104D_GPIO0_PP_OD, 0xFF); : : if (gpio1_enabled) : pnp_write_config(dev, NCT5104D_GPIO1_PP_OD, 0xFF); : : if (gpio6_enabled) : pnp_write_config(dev, NCT5104D_GPIO6_PP_OD, 0xFF);
Yes, they do. We want to reset only enabled gpio banks.
Done