Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 15:
(6 comments)
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 30: fan_temp_source temp_source, fan_type ftype, : fan_mode fmode, fan_pwm_freq fan_freq, : fan_rate_up rate_up, fan_rate_down rate_down, : fan_follow follow, u8 *boundaries, u8 *sections
Maybe turn all of this into a structure? Doesn't it seem a little silly to pass all of this?
This was originally part of mainboard, using the API I defined previously. Felix asked to have it moved to SIO code, which I did... but did not thought about converting to a structure. Will do, it's a good idea.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/acpi/superio.asl:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 5: Copyright (C) 2013 secunet Security Networks AG
We're going to be moving all of the copyright lines out of the source code and into an authors file. […]
Actually nothing creative... Mostly copy/paste with small changes. The HWM API was creative, the ACPI code was not.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 26: 0x01
Also not a big deal, but this column could be aligned with the column below.
Will do.
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 67: 127
Angel's saying that you should turn it into a #define. We don't want a magic number.
I agree... I was just saying that the define will be 120...
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) {
Add that as a comment please?
Will do.
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); : }
add that as a comment?
I was thinking more o n the line of making it explicit instead of using the trick. This way no comment will be needed.