Re: [PATCH v10 1/3] RTC: RK808: add RTC driver for RK808

Andrew,

On Wed, Sep 10, 2014 at 3:08 PM, Andrew Morton
[off-list ref] wrote:

On Wed, 10 Sep 2014 14:37:13 -0700 Doug Anderson [off-list ref] wrote:

quoted

Andrew,

On Wed, Sep 10, 2014 at 1:44 PM, Andrew Morton
[off-list ref] wrote:

quoted

On Wed, 10 Sep 2014 09:18:04 +0800 Chris Zhong [off-list ref] wrote:

quoted

Adding RTC driver for supporting RTC device present inside RK808 PMIC.

...

+     ret = rtc_valid_tm(&tm);
+     if (ret) {
+             dev_warn(&pdev->dev, "invalid date/time and init time\n");
+             rk808_rtc_set_time(&pdev->dev, &tm_def);
+     }

This is somewhat unusual.  Most drivers will emit a warning and give up
when they find the time is wrong.  Why is this driver different and is
this desirable behaviour?

When you say "give up", what does that mean?  I assume the driver
should keep initting, right?  Then the user can go in and set a time
later...

I think I was misreading current drivers a bit.  rtc-cmos.c will go in
and set a dummy time but regular low-level drivers don't sanity-check
the time at all at setup time.

quoted

I did test things with just removing this chunk of code.  You get some
yells at bootup if you put a bogus time in there:

[    2.987590] rk808-rtc rk808-rtc: invalid date/time and init time
[    3.013148] rk808-rtc rk808-rtc: rtc core: registered rk808-rtc as rtc0
[    4.586115] rk808-rtc rk808-rtc: hctosys: invalid date/time

...but if you later set a valid time then everything is fine.  That
seems reasonable behavior to me, so I guess we could just remove this
whole chunk?  It appears that after a normal bootup the date/time is
something valid.

hm.  Having an invalid time is perhaps better than having a valid but
incorrect time.

OK.  It's an easy patch to remove this.  Chris: do you want to test
that too and post it, or do you want me to?  Basically just remove:

  rk808_rtc_set_time(&pdev->dev, &tm_def);

...and of course then remove the tm_def structure and take braces off
the "if".  I think everything works if you do that.  We need to make
sure to base on the patch Andrew talked about about making the
structure static (the new patch would remove the structure completely,
but nice not to get a merge conflict).

-Doug