Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... File src/superio/fintek/f81803a/fan_control.c:
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 38: read_and_or_write hwm_reg_modify maybe? i find the the current function name a bit misleading on multiple levels; first i wondered when it'll do reads or writes and then i wondered why it says and_or, but there's no and or or parameter
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 44: use_mask = mask << shift; : use_value = (value & mask) << shift; the assignments can be put in the same line as the variable declaration. that would save one line and make the code a little easier to read (at least for me)
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 51: static inline void set_prog_sel(u16 address, u8 value) what does this function do? a short comment would probably make this clearer. does this select/switch register banks?
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 57: static int check_value_seq(u8 *values, u8 count) a short comment on what this function does would also be good here
https://review.coreboot.org/#/c/33623/5/src/superio/fintek/f81803a/fan_contr... PS5, Line 232: 0 0 or 1? in other places this function was called with last parameter being 1 and afterwards again with last parameter 0