Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35849 )
Change subject: src/superio/nuvoton/nct5104d: assign IO port range to control GPIO ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... File src/superio/nuvoton/nct5104d/superio.c:
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 127: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg & 0xFE);
this should be a pnp_set_enable call; this duplicates functionality that could be done with a device […]
With a device tree that bit is always set to "1", but the goal of that function is not to enable WDT. That's why there is additional variable 'enable_wdt1' in device tree, by which user really decides if WDT needs to be enabled or disabled.
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 152: io_base_address = pnp_read_config(dev, 0x61); : io_base_address |= (pnp_read_config(dev, 0x60) << 8);
use pnp_read_iobase instead
In entire file, functions from device/pnp.h are used. Therefore, if I want to include device/pnp_ops.h (which contains pnp_read_iobase function), there are conflicts because both files now contain functions with the same names. To avoid that I prefer still use device/pnp.h and just add pnp_read_iobase function to it. Is that will be ok?
https://review.coreboot.org/c/coreboot/+/35849/1/src/superio/nuvoton/nct5104... PS1, Line 163: reg = pnp_read_config(dev, 0x30); : pnp_write_config(dev, 0x30, reg | 0x02);
this also does what pnp_set_enable does
pnp_set_enable modifies CR30.0 bit. I need to set CR30.1 bit to enable GPIO Base Address mode