Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(39 comments)
haven't had a closer look at fan_control.c yet, since some changes to the header files will simplify the code a bit
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... File src/superio/fintek/common/fan_control.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 26: _ stray underscore
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 23: #define EXTERNAL_SENSOR1 1 : #define EXTERNAL_SENSOR2 2 : #define EXTERNAL_SENSOR3 3 : #define EXTERNAL_SENSOR_4 4 enums instead of the defines? since the numbers matter here, add an explicit assignment to the first element of the enum
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 27: #define TEMP_SENSOR_THERMISTOR 0 : #define TEMP_SENSOR_BJT 1 : #define TEMP_SENSOR_DEFAULT 2 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 31: #define FAN_TYPE_PWM_PUSH_PULL 0 : #define FAN_TYPE_DAC_POWER 1 : #define FAN_TYPE_PWM_OPEN_DRAIN 2 : #define FAN_TYPE_SET_BY_STRAP 3 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 37: #define FAN_MODE_AUTO_RPM 0 : #define FAN_MODE_AUTO_PWM_DAC 1 : #define FAN_MODE_MANUAL_RPM 2 : #define FAN_MODE_MANUAL_PWM_DAC 3 : #define FAN_MODE_DEFAULT 4 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 42: : #define FAN_PWM_FREQ_23500 0 : #define FAN_PWM_FREQ_11750 1 : #define FAN_PWM_FREQ_5875 2 : #define FAN_PWM_FREQ_220 3 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 48: #define FAN_TEMP_PECI 0 : #define FAN_TEMP_EXTERNAL_1 1 : #define FAN_TEMP_EXTERNAL_2 2 : #define FAN_TEMP_TSI 4 : #define FAN_TEMP_MXM 5 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 54: #define FAN_UP_RATE_2HZ 0 : #define FAN_UP_RATE_5HZ 1 : #define FAN_UP_RATE_10HZ 2 : #define FAN_UP_RATE_20HZ 3 : #define FAN_UP_RATE_DEFAULT 4 : #define FAN_UP_RATE_JUMP 8 same here; last one needs additional explicit value assignment
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 61: #define FAN_DOWN_RATE_2HZ 0 : #define FAN_DOWN_RATE_5HZ 1 : #define FAN_DOWN_RATE_10HZ 2 : #define FAN_DOWN_RATE_20HZ 3 : #define FAN_DOWN_RATE_DEFAULT 4 : #define FAN_DOWN_RATE_SAME_AS_UP 5 : #define FAN_DOWN_RATE_JUMP 8 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 69: #define FAN_FOLLOW_STEP 0 : #define FAN_FOLLOW_INTERPOLATION 1 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 71: : #define HWM_STATUS_SUCCESS 0 : #define HWM_STATUS_INVALID_FAN -1 : #define HWM_STATUS_INVALID_TEMP_SOURCE -2 : #define HWM_STATUS_INVALID_TYPE -3 : #define HWM_STATUS_INVALID_MODE -4 : #define HWM_STATUS_INVALID_RATE -5 : #define HWM_STATUS_INVALID_FREQUENCY -6 : #define HWM_STATUS_INVALID_TEMP_SENSOR -7 : #define HWM_STATUS_INVALID_BOUNDARY_VALUE -8 : #define HWM_STATUS_INVALID_SECTION_VALUE -9 : #define HWM_STATUS_BOUNDARY_WRONG_ORDER -20 : #define HWM_STATUS_SECTIONS_WRONG_ORDER -21 : #define HWM_STATUS_WARNING_SENSOR_DISCONECTED 1 : #define HWM_STATUS_WARNING_FAN_NOT_PWM 2 enum would also be nice here, but assigning negative values to enum elements is probably something that's not explicitly defined in the C specification. or at least i'd be rather cautious about this
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 91: fintek_boundaries_size defines should be in uppercase
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 99: fintek_sections_size same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 101: u8 sensor, u8 type enum types?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 102: u8 source enum type?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 103: u8 type, u8 mode enum types?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 104: u8 frequency enum type?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 105: u8 *boundaries, u8 *sections hm, this might cause nasty bugs when the caller passes a pointer to a too short array. u8 boundaries[static fintek_boundaries_size] might be worth a try; not sure if this is in the c standard we're currently using though. a struct probably won't work well here, since you can't easily iterate over the struct members
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 106: u8 rate_up, : u8 rate_down enum types?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a.h File src/superio/fintek/f81803a/f81803a.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a.h... PS2, Line 35: alignment
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a.h... PS2, Line 37: /* Global Control Registers */ some additional whitespace issues below
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... File src/superio/fintek/f81803a/f81803a_hwm.h:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 21: #define TP_SENSOR1_TYPE_SHIFT 1 : #define TP_SENSOR2_TYPE_SHIFT 2 TP_SENSOR_TYPE_SHIFT(x)? see my comment below
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 28: #define TP_EXTERNAL_SENSOR2_OPEN 0x04 : #define TP_EXTERNAL_SENSOR1_OPEN 0x02 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 32: #define FAN1_TYPE_SHIFT 0 : #define FAN2_TYPE_SHIFT 2 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 36: #define FAN1_MODE_SHIFT 0 : #define FAN2_MODE_SHIFT 4 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 39: #define FAN1_FREQ_SEL_ADD_SHIFT 3 /* FUNC_PROG_SEL = 1 */ : #define FAN2_FREQ_SEL_ADD_SHIFT 4 /* FUNC_PROG_SEL = 1 */ same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 42: #define FAN1_UP_RATE_SHIFT 0 : #define FAN2_UP_RATE_SHIFT 2 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 46: #define FAN1_DOWN_RATE_SHIFT 0 : #define FAN2_DOWN_RATE_SHIFT 2 same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 54: #define FAN1_BOUND_TEMP1 0xa6 : #define FAN1_BOUND_TEMP2 0xa7 : #define FAN1_BOUND_TEMP3 0xa8 : #define FAN1_BOUND_TEMP4 0xa9 #define FAN_BOUND_TEMP(fan, temp) (0xa6 + (0x10 * (fan)) + (temp)) would make the code using this a bit more elegant. would be 0-indexed though instead of the current indices
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 58: #define FAN1_SECTION_SPEED1 0xaa : #define FAN1_SECTION_SPEED2 0xab : #define FAN1_SECTION_SPEED3 0xac : #define FAN1_SECTION_SPEED4 0xad : #define FAN1_SECTION_SPEED5 0xae same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 63: #define FAN2_BOUND_TEMP1 0xb6 : #define FAN2_BOUND_TEMP2 0xb7 : #define FAN2_BOUND_TEMP3 0xb8 : #define FAN2_BOUND_TEMP4 0xb9 see comment above
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 67: #define FAN2_SECTION_SPEED1 0xba : #define FAN2_SECTION_SPEED2 0xbb : #define FAN2_SECTION_SPEED3 0xbc : #define FAN2_SECTION_SPEED4 0xbd : #define FAN2_SECTION_SPEED5 0xbe same here
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 72: #define FAN_DETAIL_SKIP (FAN2_BOUND_TEMP1 - FAN1_BOUND_TEMP1) not needed with my suggestion for this
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/f81803a_h... PS2, Line 74: #define FAN1_TMP_MAPPING 0xaf : #define FAN2_TMP_MAPPING 0xbf seem comment above
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c File src/superio/fintek/f81803a/superio.c:
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 25: static void f81803a_pme_init(struct device *dev) : { : pnp_enter_conf_mode(dev); : pnp_write_config(dev, LDN_REG, F81803A_PME); : /* enable ERP function*/ : /* also set PSIN to generate PSOUT*/ : pnp_write_config(dev, PME_ERP_ENABLE_REG, ERP_ENABLE | ERP_PSOUT_EN); : pnp_exit_conf_mode(dev); : } is flipping these two bits generally needed with this SIO chip or board-specific?
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 40: or FDC i'd guess that this SIO doesn't have a floppy controller, so the part of the comment about the FDC can be removed
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 55: looks like one tab too much; same for next line
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 61: /* TODO: Some of the 0x7f8 etc. values may not be correct. */ please check this and remove the comment after checking and if needed fixing
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/f81803a/superio.c... PS2, Line 67: //{ &ops, F81803A_WDT, PNP_IO0, 0x7f8 }, what about the WDT?