Attention is currently required from: Hung-Te Lin, Jarried Lin, Paul Menzel, Yidi Lin.
Yu-Ping Wu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/85978?usp=email )
Change subject: soc/mediatek/mt8196: Add RTC driver ......................................................................
Patch Set 5:
(2 comments)
File src/soc/mediatek/mt8196/include/soc/rtc.h:
https://review.coreboot.org/c/coreboot/+/85978/comment/64bf6fe8_c5a12033?usp... : PS1, Line 18: RTC_BBPU_PWREN = BIT(0),
The new version of the header file mt6685_RTC_HW.H deletes the same macro definition as rtc_common. […]
Sorry, but I'm not quite convinced of that approach. The duplicate constants are one problem, but the underlying problem is the organization of the headers. In PS5 the `RTC_*` constants are still spread in rtc.h and mt6685_rtc_hw.h in MT8196's code (not considering the common rtc_reg_common.h), and it's unclear to me what the difference of them is. I asked about the distinction of rtc.h, mt6685_rtc_hw.h and mt6685_rtc.h, but you didn't answer my question yet.
In PS5 you removed the duplicate constants in mt6685_rtc_hw.h. However that leaves mt6685_rtc_hw.h in an incomplete state (for example it contains `RTC_BBPU_PWREN` but not `RTC_BBPU`), so that approach is not ideal.
Considering the limited time we have, I'd like to propose a simplest approach: following what we did for existing SoCs.
- Remove mt6685_hw.h, mt6685_rtc_hw.h and mt6685_rtc.h and keep only rtc.h. Move constants from these headers to rtc.h if they are used. - Keep only required constants in rtc.h. Please make sure we don't have duplicate constants in rtc.h, rtc_common.h and rtc_reg_common.h.
Then we may consider refactoring in future patches.
File src/soc/mediatek/mt8196/mt6685_rtc.c:
https://review.coreboot.org/c/coreboot/+/85978/comment/3ac1ac4a_18306f6e?usp... : PS2, Line 358: rtc_init_after_recovery
move to above line
Done