Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(6 comments)
Some of the code I got ready from previous engineer, and I did not check for correctness. My code is only the fan control. Will fix remaining code.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 69: #define FAN_FOLLOW_STEP 0 : #define FAN_FOLLOW_INTERPOLATION 1
same here
You know, now that you highlighted it, I don't have code for this. I'll have to check why, if I just missed or if I just placed it here and need comment to explain what this was for.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 25: static void f81803a_pme_init(struct device *dev) : { : pnp_enter_conf_mode(dev); : pnp_write_config(dev, LDN_REG, F81803A_PME); : /* enable ERP function*/ : /* also set PSIN to generate PSOUT*/ : pnp_write_config(dev, PME_ERP_ENABLE_REG, ERP_ENABLE | ERP_PSOUT_EN); : pnp_exit_conf_mode(dev); : }
is flipping these two bits generally needed with this SIO chip or board-specific?
Apparently with any SIO of this family, if you're enabling PME functionality.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 40: or FDC
i'd guess that this SIO doesn't have a floppy controller, so the part of the comment about the FDC c […]
It does have watch dog timer, so I'll replace FDC with WDT.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 55:
looks like one tab too much; same for next line
will fix
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 61: /* TODO: Some of the 0x7f8 etc. values may not be correct. */
please check this and remove the comment after checking and if needed fixing
will do.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 67: //{ &ops, F81803A_WDT, PNP_IO0, 0x7f8 },
what about the WDT?
This SIO does have it...