Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33421 )
Change subject: util/superiotool/nuvoton.c: add NCT5539D register dump ......................................................................
Patch Set 9:
(31 comments)
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c File util/superiotool/nuvoton.c:
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@587 PS5, Line 587: 0x20,0x21 ID registers don't have to be included here, since they're already used to select this chip's data structures in superiotool. doen't hurt to include them though.
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@587 PS5, Line 587: 0x07 LDN select register not really needed here
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@600 PS5, Line 600: 0x00 hm, the state of bits in this register where register 0xe0 is 0 is dependent on the external circuitry. since it's not only the input register, i don't have a too strong opinion on this one; however the default pin direction is input and in this case the register state is NANA
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@600 PS5, Line 600: 0x00 NANA
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@600 PS5, Line 600: 0x00 NANA
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@600 PS5, Line 600: 0x00 see my comment on 0xe1
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@601 PS5, Line 601: WDT3, WDT3 is mentioned twice; remove one
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@603 PS5, Line 603: 0x00 NANA
https://review.coreboot.org/#/c/33421/5/util/superiotool/nuvoton.c@603 PS5, Line 603: 0x00 same comment as in LDN 7
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c File util/superiotool/nuvoton.c:
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@587 PS9, Line 587: 0x07 LDN select register not needed here
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@587 PS9, Line 587: 0x20,0x21 chip id register not needed, since this is used to select this chip
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@602 PS9, Line 602: 0x00 NANA
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@602 PS9, Line 602: 0x00 NANA
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@602 PS9, Line 602: 0x00 when register 0xe0 is in its default state, this is rather NANA
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@602 PS9, Line 602: 0x00 see comment on LDN7 0xe1
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@603 PS9, Line 603: WDT3, WDT3 mentioned twice; remove one
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@605 PS9, Line 605: 0x00 see comment on LDN7 0xe1
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@605 PS9, Line 605: 0x00 NANA
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@608 PS9, Line 608: 0x00 NANA
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@608 PS9, Line 608: 0x00 see comment on LDN7 0xe1
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@608 PS9, Line 608: 0x00 see comment on LDN7 0xe1
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@608 PS9, Line 608: 0x00 NANA
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@608 PS9, Line 608: 0x00 see comment on LDN7 0xe1
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@608 PS9, Line 608: 0x00 NANA
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@608 PS9, Line 608: 0x00 see comment on LDN7 0xe1
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@608 PS9, Line 608: 0x00 NANA
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@613 PS9, Line 613: {0x30,0x60,0x61,0x70,0xe0,0xe1,0xe2,0xe3,0xe4,0xe5,0xe6,0xe7,0xec,0xed,0xf0,0xf1,0xf2,0xf6,0xf7,0xf8,0xf9, : 0xfa,0xfb,0xfc,EOT}, : {0x00,0x00,0x00,0x00,0x7f,0x7f,0x7f,0x7f,0x7f,0xa8,0x08,0x7f, : 0x00,0x81,0x00,0x00,0x00,0x00,0x87,0x47,0x00,0x00,0x00,0x02,EOT}}, inconsistent formatting
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@626 PS9, Line 626: Register remove "Register"?
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@629 PS9, Line 629: Register remove "Register"?
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@632 PS9, Line 632: Register remove "Register"?
https://review.coreboot.org/#/c/33421/9/util/superiotool/nuvoton.c@637 PS9, Line 637: 0xa0 this seems to be a bit odd to me; i'd have expected 0x00 here. did you try reading this from the chip on the mainboard and if so what value did you get there?