Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
My real concern was having fan_control.h accepted, because it introduces a new API.
Only had a brief look at the patch, but wanted to get some feedback soonish, so here we go:
For the SIO chips of other vendors (mostly some ITE ones and I think one Nuvoton one) the configuration of the fan control/HWM part is done via device tree settings (look for the chip.h file in the SIO folders). The problem with the configuration via device tree settings is that the code can't tell apart if a setting isn't configured or if it is set to zero. I'm not opposed to do the configuration of the HWM by function calls in ramstage instead of device tree settings, since this would work around that device tree setting issue. For assigning IO ports and IRQ and that sort of stuff, the device tree works rather well, but at least some parts of the configuration of the HWM does indeed feel a bit shoehorned in to the device tree.
The discussion device tree setting vs. doing things in code recently also started in the context of the FSP-S settings, although the problem there is a bit different; the bigger issue there is that some FSP settings simply aren't hooked up to the device tree setting infrastructure and to a lesser extend that this is mostly unnecessary code (well, the FSP contains much more boilerplate compared to the device tree settings stuff).
For the SIO HWM part, I'd like to make sure that we won't end up with multiple non-devicetree API concepts; this getting too inconsistent for different SIOs HWMs would be my only serious concern here. Since the SIO configuration is rather board-specific and a board doesn't have a huge variety of SIOs (sure, some variants of one boards have 2 different SIO chips, but this is no blocker for me), it doesn't need to be one exact API that is used in the first and final form for all SIOs, but the general concept of how things are done should be similar and clear.
Is there a patch that shows how this new API is used?
Might be a good idea to have a discussion about the device tree setting vs. function calls in ramstage on the mailing list, but I don't expect too much opposition against this.
TL;DR since I'm not sure if this sounded a bit too negative: I do like the idea, but want to make sure that the change will improve the architecture and won't be a pain to maintain in the future.