Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 15:
(9 comments)
Patch Set 15:
Patch Set 11: Code-Review+1
(10 comments)
I like the idea. I've got a few comments about the code, though.
Hi, any comments on these? Several of them are still present in the latest patchset.
Sorry, slid under the radar.
https://review.coreboot.org/c/coreboot/+/33623/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33623/11//COMMIT_MSG@10 PS11, Line 10: onlF81803A
Looks like a few characters were eaten away
Oops... juggling too many things, it passed under the radar... will fix.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... File src/superio/fintek/common/fan_api_call.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/common/... PS11, Line 29: void set_fan(uint8_t fan, external_sensor sensor, temp_sensor_type stype,
I would suggest moving these printk's to the called functions, so that you can use __func__ instead. […]
Will investigate the possibility, not sure. Will try.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 7: This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : 96? I thought it was 76....
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 11: * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : This section is normally copied without change, so I did not try to adjust for 76 characters.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67: 127
I would suggest making this magic number a constant
127 is the maximum celsius value that can be output as a signed byte, The max percentile is actually 100. As I'm treating them as unsigned, I'm able to use 128... though I probably could use 120 and make it a magic number... that's the temperature at which you CPU will be damaged beyond repair.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 85: return HWM_STATUS_SUCCESS;
I would add this to the switch statement below.
Will do.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) {
This looks odd: is the first fan different?
Yes, fan 1 has a weight mechanism for adjusting for next fan speed. Basically the idea is to react more aggressively (normally CPU fan) based on how high another temperature (system, thermistor near the CPU, anything) is. This would be highly platform dependent, and by setting the weight temperature same as the control temperature I cancel the weight mechanism and make it work with any board. If a board wants to use the weight mechanism, they should implement it after calling the main HWM programming.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 161: :
Doesn't look like this line needs to be split at all
Will fix.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 228: if (rate_up < FAN_UP_RATE_DEFAULT) { : hwm_reg_modify(base_address, FAN_UP_RATE_REG, : shift, FAN_RATE_MASK, rate_up); : }
Should this only be done on the else path?
Yes, this is a trick. Basically testing for FAN_UP_RATE_JUMP, which require a different set of registers. It would be the same as: if (rate_up == FAN_UP_RATE_JUMP) Same trick below for rate down.