Attention is currently required from: Hung-Te Lin, Jarried Lin.
Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/86017?usp=email )
Change subject: soc/mediatek/mt8196: Add thermal driver ......................................................................
Patch Set 8: -Code-Review
(4 comments)
File src/soc/mediatek/mt8196/thermal.c:
https://review.coreboot.org/c/coreboot/+/86017/comment/339c1b36_2dbe623d?usp... : PS8, Line 38: */
To avoid duplication, the comment should just be in one file?
Please remove the duplicate comments in thermal_internal.h.
https://review.coreboot.org/c/coreboot/+/86017/comment/66ae941f_f2c7b6e7?usp... : PS8, Line 163: printk(BIOS_INFO, "%s: msr_raw=%u, temp_mc=%d\n", __func__, msr_raw, temp_mc);
Should be a debug message, or rewritten?
Please change to `BIOS_DEBUG`
https://review.coreboot.org/c/coreboot/+/86017/comment/1ca3aa62_ce7c38e7?usp... : PS8, Line 170: uint32_t i;
size_t?
I'd prefer using `int` for array index. `size_t` is for representing the size of a C object.
Either way, `i`, `j`, `s_index` should all be changed to a native type.
https://review.coreboot.org/c/coreboot/+/86017/comment/e1bcf8c4_c3778267?usp... : PS8, Line 271: printk(BIOS_INFO, "%s\n", __func__);
Looks like a debug message.
Can we remove this log?
-
Yu-Ping Wu (Code Review)