On 17/07/2019 16:54:54+0800, Ran Bi wrote:

quoted

quoted

+
+/* we map HW YEAR 0 to 1968 not 1970 because 2000 is the leap year */
+#define RTC_MIN_YEAR		1968
+#define RTC_BASE_YEAR		1900
+#define RTC_MIN_YEAR_OFFSET	(RTC_MIN_YEAR - RTC_BASE_YEAR)

Do not do that. If this RTC range starts in 200, ths is what the driver
has to support, you should not care about dates before 2000. Note that
the RTC core can still properly shift the range if it is absolutely
necessary.

Do we need to care about default alarm date 1970-01-01? Or can I just
set it to 2000-01-01?

You never have to set a default value. It doesn't add any value versus
an unknown value.

quoted

quoted

+
+static inline u32 rtc_readl(struct mt2712_rtc *rtc, u32 reg)

Please use a more descriptive prefix than just rtc_.

Do you mean it's better to use prefix "mt2712_rtc_"?

Yes.

quoted

quoted

+
+	/*
+	 * register status was not correct,
+	 * need set time and alarm to default
+	 */
+	if (p1 != RTC_POWERKEY1_KEY || p2 != RTC_POWERKEY2_KEY
+	    || !valid_rtc_time(rtc)) {
+		reset_rtc_time(rtc);

Do not do that. This is valuable information. If the time is invalid,
report it as such in read_time and read_alarm. Resetting the time here
will lead to more issues later (i.e. userspace is not able to know
whether the time is set correctly or not).

When RTC's power run out, RTC will lost it's registers value and time
data at next boot up. We even cannot know what the date and time it
shows. We want to check this state here and set a default RTC date. Do
you think it's no need here and the date should be set by system?

If I understand correctly, the POWERKEY register will lose their value.
This means that you know that the time is incorrect. instead of setting
it to a default value and losing that valuable information, simply check
for that in read_time and return EINVAL in that case. then on the next
set_time invocation, you can set the POWERKEY registers and set the time
to a known value.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com