Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35482 )
Change subject: src/superio/nuvoton/nct5104d: soft reset GPIO ......................................................................
Patch Set 1:
(5 comments)
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
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
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); I'd suggest using pnp_set_logical_device here
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
https://review.coreboot.org/c/coreboot/+/35482/1/src/superio/nuvoton/nct5104... PS1, Line 143: reset_gpio(dev); when for example gpio bank 0 isn't enabled and bank 1 is enabled, bank 0 will also get reset. is this intended?