Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48790 )
Change subject: superio/nuvoton/nct6793d: Add initial support ......................................................................
Patch Set 2: Code-Review+1
(6 comments)
There are a few differences in the datasheets (commented them inline in `nct6793d.h`). If you are sure that the NCT6793D datasheet is wrong, just mention that in the commit message. If not, better adapt the code.
https://review.coreboot.org/c/coreboot/+/48790/2/src/superio/nuvoton/nct6793... File src/superio/nuvoton/nct6793d/nct6793d.h:
https://review.coreboot.org/c/coreboot/+/48790/2/src/superio/nuvoton/nct6793... PS2, Line 17: /* BCLK, WDT2, WDT_MEM */ This seems to have changed, unless the datasheet is incomplete.
https://review.coreboot.org/c/coreboot/+/48790/2/src/superio/nuvoton/nct6793... PS2, Line 38: #define NCT6793D_PCHDSW ((3 << 8) | NCT6793D_DS) Reserved in the DS?
https://review.coreboot.org/c/coreboot/+/48790/2/src/superio/nuvoton/nct6793... PS2, Line 41: #define NCT6793D_DSDSS ((6 << 8) | NCT6793D_DS) Reserved in the DS?
https://review.coreboot.org/c/coreboot/+/48790/2/src/superio/nuvoton/nct6793... File src/superio/nuvoton/nct6793d/superio.c:
https://review.coreboot.org/c/coreboot/+/48790/2/src/superio/nuvoton/nct6793... PS2, Line 13: if (!dev->enabled) : return; : This function is only called for enabled devices.
https://review.coreboot.org/c/coreboot/+/48790/2/src/superio/nuvoton/nct6793... PS2, Line 48: pnp_alt_enable I wonder if it really needs the `alt` enable. Would be nice if you could compare superiotool dumps of cold boots with this set to `pnp_enable` and `pnp_alt_enable`.
https://review.coreboot.org/c/coreboot/+/48790/2/src/superio/nuvoton/nct6793... PS2, Line 60: 0x0ff8, }, The line breaks here seem unnecessary.