Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(7 comments)
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
Opps.
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 […]
Will do.
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
Will do to all.
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 t […]
I'll not change these.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 91: fintek_boundaries_size
defines should be in uppercase
Agree, will do.
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 101: u8 sensor, u8 type
enum types?
Probably. Same for all other places you marked.
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. […]
That would be a user error, as there are explicitly 4 boundaries making 5 sections. Maybe a comment to avoid user error?