Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 19:
(42 comments)
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
Oops... juggling too many things, it passed under the radar... will fix.
Done
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,
Will investigate the possibility, not sure. Will try.
Done.
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
This was originally part of mainboard, using the API I defined previously. […]
Done.
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
Actually nothing creative... Mostly copy/paste with small changes. […]
Done
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
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
PS11:
Minor: some split lines seem to fit on the current 96 character limit without splitting
Done.
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. :
No, we've changed the maximum line length to 96 characters. […]
Done.
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.
Done.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 67: 127
I agree... I was just saying that the define will be 120...
Done.
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 85: return HWM_STATUS_SUCCESS;
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 119: if (fan == FIRST_FAN) {
Will do.
Done
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... PS11, Line 161: :
Will fix.
Done
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); : }
I was thinking more o n the line of making it explicit instead of using the trick. […]
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 21: static char msg_err_invalid[] = "Error: invalid";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 22: static char msg_err_wrong_order[] = "Error: wrong order,";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 23: static char msg_err_fan[] = "fan";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 24: static char msg_err_temp_source[] = "temperature source";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 25: static char msg_err_type[] = "type";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 26: static char msg_err_mode[] = "mode";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 27: static char msg_err_rate[] = "change rate";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 28: static char msg_err_frequency[] = "frequency";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 29: static char msg_err_temp_sensor[] = "temperature sensor";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 30: static char msg_err_bondary[] = "boundary";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 31: static char msg_err_section[] = "section";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 32: static char no_msg[] = "";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 49: static char* get_msg(int err)
"foo* bar" should be "foo *bar"
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 52: while(msg_table[i].selection) {
space required before the open parenthesis '('
Done
https://review.coreboot.org/c/coreboot/+/33623/16/src/superio/fintek/f81803a... PS16, Line 53: if (msg_table[i].selection == err)
suspect code indent for conditional statements (16, 16)
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 21: static char msg_err_invalid[] = "Error: invalid";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 22: static char msg_err_wrong_order[] = "Error: wrong order,";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 23: static char msg_err_fan[] = "fan";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 24: static char msg_err_temp_source[] = "temperature source";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 25: static char msg_err_type[] = "type";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 26: static char msg_err_mode[] = "mode";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 27: static char msg_err_rate[] = "change rate";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 28: static char msg_err_frequency[] = "frequency";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 29: static char msg_err_temp_sensor[] = "temperature sensor";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 30: static char msg_err_bondary[] = "boundary";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 31: static char msg_err_section[] = "section";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 32: static char no_msg[] = "";
static char array declaration should probably be static const char
Done
https://review.coreboot.org/c/coreboot/+/33623/17/src/superio/fintek/f81803a... PS17, Line 53: if (msg_table[i].selection == err)
Will fix.
Done
https://review.coreboot.org/c/coreboot/+/33623/18/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/c/coreboot/+/33623/18/src/superio/fintek/f81803a... PS18, Line 52: while(msg_table[i].selection) {
space required before the open parenthesis '('
Done