Yuchen Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46395 )
Change subject: soc/mediatek/mt8192: add rtc MT6359P driver ......................................................................
Patch Set 52:
(29 comments)
https://review.coreboot.org/c/coreboot/+/46395/51//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46395/51//COMMIT_MSG@8 PS51, Line 8:
Since it's not trivial to me, could you mention how the code is "refactored" (by moving between file […]
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... File src/soc/mediatek/common/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 103: 0
Does 0 mean success or failure here?
0 here means failure. This is consistent in the rtc coreboot driver
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... File src/soc/mediatek/common/rtc_osc_init.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 4: #include <soc/rtc.h>
rtc.h should come before rtc_common. […]
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 9: diff1, diff2
diff_left, diff_right
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 10: = 0
No need to initialize this and "middle".
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 20: 0x%x
%#x
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 34:
Remove extra blank line.
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 44: if (val > RTC_FQMTR_LOW_BASE) : diff1 = val - RTC_FQMTR_LOW_BASE; : else : diff1 = RTC_FQMTR_LOW_BASE - val;
This can be simplified to […]
Ack
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 56: :
One space after ":"
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/common/rt... PS51, Line 69: C
Please be consistent with the case (C or c).
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/in... File src/soc/mediatek/mt8173/include/soc/rtc.h:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/in... PS51, Line 111: fail
failed
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/in... PS51, Line 122: fail
failed
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/rt... File src/soc/mediatek/mt8173/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/rt... PS51, Line 87: .
No period for consistency with comments above.
Ack
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/rt... PS51, Line 87: i
I
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8173/rt... PS51, Line 87: W
w
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/rtc.h:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/in... PS51, Line 219: fail
failed
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/in... PS51, Line 230: fail
failed
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/rt... File src/soc/mediatek/mt8183/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8183/rt... PS51, Line 228: /* in recovery mode, We need 20ms delay for register setting. */
Same.
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/rtc.h:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/in... PS51, Line 14: E
Could you add a "," for all the enums in this file?
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/in... PS51, Line 89: to low
low
Ack
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/in... PS51, Line 89: lower leakage current hw design change
What does that mean?
Ack
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... File src/soc/mediatek/mt8192/rtc.c:
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 21: (unsigned int)
Do we need the cast? Same below. […]
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 46: /* Unlock for reload */
Move to its own line.
Ack
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 67: 0x%x
%#x
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 136: out !!
out!
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 220: u8
If this is boolean, please use native type such as int.
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 241: i
I
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 241: W
w
Done
https://review.coreboot.org/c/coreboot/+/46395/51/src/soc/mediatek/mt8192/rt... PS51, Line 319: 0x%x
%#x […]
Done