Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 2:
(2 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 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
I'll not change these.
I'd be ok with that, since i don't see a better, but still easily readable way, right now. also since this is the return value and not another parameter of the functions, it doesn't make things too inconsistent IMHO, so yeah, just keep this
https://review.coreboot.org/#/c/33623/2/src/superio/fintek/common/fan_contro... PS2, Line 105: u8 *boundaries, u8 *sections
That would be a user error, as there are explicitly 4 boundaries making 5 sections. […]
I'd definitely like to have a comment stating this; without a comment this is too easy to use wrongly. not in this patch, but in the patch using the code: the calling code should probably explicitly define the arrays passed to this function with the boundary and section size as lengths, so no array[], but array[length] instead