Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803A ......................................................................
Patch Set 11: Code-Review+1
(10 comments)
I like the idea. I've got a few comments about the code, though.
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
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.
Maybe make this function return the error code as well?
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 67: Minor: previous line uses tabs here, this one uses spaces
https://review.coreboot.org/c/coreboot/+/33623/11/src/superio/fintek/f81803a... File src/superio/fintek/f81803a/f81803a.h:
PS11: Minor consistency issue: There are some hex values with lowercase letters, and others with uppercase letters.
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
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
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.
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?
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
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?