Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 15:
(7 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?
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
This was simply copied by the previous engineer from another fintek SIO... […]
We're going to be moving all of the copyright lines out of the source code and into an authors file. You can add a copyright if you've done anything creative, in updating the file, but I don't know that it matters.
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.
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....
No, we've changed the maximum line length to 96 characters.
I don't think Angel was talking about the license. That should stay as it is. But if you look at lines 38 & 39, that could be combines.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67: 127
127 is the maximum celsius value that can be output as a signed byte, The max percentile is actually […]
Angel's saying that you should turn it into a #define. We don't want a magic number.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) {
Yes, fan 1 has a weight mechanism for adjusting for next fan speed. […]
Add that as a comment please?
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); : }
Yes, this is a trick. […]
add that as a comment?