Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 4:
(4 comments)
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
Will do the first element of enum. Not sure which one I missed, there were so many... could you please explicit. I'm also missing code for interpolation, which is set at reset. I need to at least give the option to disable it, even if no one uses it.
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
will do.
https://review.coreboot.org/#/c/33623/4/src/superio/fintek/common/fan_contro... PS4, Line 115: fintek_sections_size
upper case
Will do.
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. […]
Will do.
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
Weird, I thought I fixed it. will do.