| Message ID | 20140910134411.8128acce79f2423d4448e8f4@linux-foundation.org |
|---|---|
| State | Accepted |
| Headers | show |
Andrew, On Wed, Sep 10, 2014 at 1:44 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 10 Sep 2014 09:18:04 +0800 Chris Zhong <zyw@rock-chips.com> wrote: > >> 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 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. -Doug
On Wed, 10 Sep 2014 14:37:13 -0700 Doug Anderson <dianders@chromium.org> wrote: > Andrew, > > On Wed, Sep 10, 2014 at 1:44 PM, Andrew Morton > <akpm@linux-foundation.org> wrote: > > On Wed, 10 Sep 2014 09:18:04 +0800 Chris Zhong <zyw@rock-chips.com> wrote: > > > >> 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. > > 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. When one sees one driver doing something differently from the others one has to wonder "why" and "which one is better". If setting a dummy time is better then all drivers should do it, and I expect this could be done by rtc core at registration time.
Andrew, On Wed, Sep 10, 2014 at 3:08 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 10 Sep 2014 14:37:13 -0700 Doug Anderson <dianders@chromium.org> wrote: > >> Andrew, >> >> On Wed, Sep 10, 2014 at 1:44 PM, Andrew Morton >> <akpm@linux-foundation.org> wrote: >> > On Wed, 10 Sep 2014 09:18:04 +0800 Chris Zhong <zyw@rock-chips.com> wrote: >> > >> >> 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. > >> >> 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
diff -puN drivers/rtc/Kconfig~rtc-rk808-add-rtc-driver-for-rk808-fix drivers/rtc/Kconfig diff -puN drivers/rtc/Makefile~rtc-rk808-add-rtc-driver-for-rk808-fix drivers/rtc/Makefile diff -puN drivers/rtc/rtc-rk808.c~rtc-rk808-add-rtc-driver-for-rk808-fix drivers/rtc/rtc-rk808.c --- a/drivers/rtc/rtc-rk808.c~rtc-rk808-add-rtc-driver-for-rk808-fix +++ a/drivers/rtc/rtc-rk808.c @@ -326,14 +326,14 @@ static SIMPLE_DEV_PM_OPS(rk808_rtc_pm_op rk808_rtc_suspend, rk808_rtc_resume); /* 2014.1.1 12:00:00 Saturday */ -struct rtc_time tm_def = { - .tm_wday = 6, - .tm_year = 114, - .tm_mon = 0, - .tm_mday = 1, - .tm_hour = 12, - .tm_min = 0, - .tm_sec = 0, +static struct rtc_time tm_def = { + .tm_wday = 6, + .tm_year = 114, + .tm_mon = 0, + .tm_mday = 1, + .tm_hour = 12, + .tm_min = 0, + .tm_sec = 0, }; static int rk808_rtc_probe(struct platform_device *pdev)