Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33623 )
Change subject: superio/fintek: Add f81803a ......................................................................
Patch Set 5:
(4 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 44: use_mask = mask << shift; : use_value = (value & mask) << shift;
the assignments can be put in the same line as the variable declaration. […]
Not sure if I understand what you're saying. I do believe that even though 1 line longer, it's clearer this way.
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. […]
Yes, registers 96 through 9e have 2 banks. Will add comment.
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
Will do.
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 wi […]
I try to exit pointing to bank 0 (default). But sometimes I have to access bank 1.