Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31616 )
Change subject: superio/ite/common: add option for enabling 5 FANs ......................................................................
Patch Set 2:
(8 comments)
Thanks for your patch.
I'm not sure about some of the added #if in `env_ctrl.h`. Do you want to keep it from compiling unnecessary code?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h File src/superio/ite/common/env_ctrl.h:
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@60 PS2, Line 60: */ Lot's of things here might be unused in one case or another, no need to comment.
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@64 PS2, Line 64: #if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) As the definition doesn't change for x <= 3, we don't need the #if, I suppose?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@66 PS2, Line 66: ({ \ Why putting it into a code-block `{}`?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@67 PS2, Line 67: x Please put parameters always in parentheses `(x)`, they may use operators with lower precedence than what follows here.
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.h@19... PS2, Line 196: #define ITE_EC_FAN_CTL_DELTA_TEMP_INTRVL(c) ((c) & 0x1f) same definition as below?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.c File src/superio/ite/common/env_ctrl.c:
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.c@19... PS2, Line 196: (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_NO_ONOFF) && Please align this more clearly. e.g. tab + 4 spaces in this line and tab + 5 spaces in the next
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl.c@21... PS2, Line 219: if (IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_FAN16_CONFIG) I thought we fixed this by now. There might be more patches lurking on Gerrit...
Can you please split this into a separate patch so we can merge the fix fast?
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl_chip... File src/superio/ite/common/env_ctrl_chip.h:
https://review.coreboot.org/#/c/31616/2/src/superio/ite/common/env_ctrl_chip... PS2, Line 114: #if IS_ENABLED(CONFIG_SUPERIO_ITE_ENV_CTRL_5FANS) No need to guard this, imho.