Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 4:
(4 comments)
I'd prefer if you assign the 0 explicitly to the first enum element, since the enum values get written to hardware registers. The effect should be the same, but just to make clear that the actual values matter and they aren't only used as symbols. There are also some comments from patch set 2 not addressed. Haven't looked closely at fan_control.c yet, but from what I saw using the macros I suggested improved the readability quite a bit
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 107: fintek_boundaries_size upper case
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 115: fintek_sections_size upper case
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 125: int set_sections(u16 base_address, u8 fan, u8 *boundaries, u8 *sections); maybe add a comment right above this function prototype regarding the array sizes. it is written above in detail, but a short pointer to that would be good to have
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/f81803a/superio.c File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/f81803a/superio.c... PS4, Line 55: whitespace issue here and on next line