| Message ID | 20141027154031.4492ea11d401045ca04a3ff8@linux-foundation.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Oct 27, 2014 at 03:40:31PM -0700, Andrew Morton wrote: > On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold <johan@kernel.org> wrote: > > > Add new property "ti,system-power-controller" to register the RTC as a > > power-off handler. > > > > Some RTC IP revisions can control an external PMIC via the pmic_power_en > > pin, which can be configured to transition to OFF on ALARM2 events and > > back to ON on subsequent ALARM (wakealarm) events. > > > > This is based on earlier work by Colin Foe-Parker and AnilKumar Ch. [1] > > > > [1] https://www.mail-archive.com/linux-omap@vger.kernel.org/msg82127.html > > > > Tested-by: Felipe Balbi <balbi@ti.com> > > Signed-off-by: Johan Hovold <johan@kernel.org> > > --- > > > > Changes since v2: > > - add two-second delay to allow alarm to trigger before returning > > hmpf. As this sentence is below the ^--- it doesn't get into the > changelog. We usually don't keep the patch-revision change log in the commit message (e.g. put in the cover letter). But in general, how do you want to handle updates to a single patch in a series you already have in your tree? Do you prefer a proper incremental-fix patch (with commit message), just an updated single patch, or a resend of the whole series? > > ... > > > > +static void omap_rtc_power_off(void) > > +{ > > + struct omap_rtc *rtc = omap_rtc_power_off_rtc; > > + struct rtc_time tm; > > + unsigned long now; > > + u32 val; > > + > > + /* enable pmic_power_en control */ > > + val = rtc_readl(rtc, OMAP_RTC_PMIC_REG); > > + rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN); > > + > > + /* set alarm two seconds from now */ > > + omap_rtc_read_time_raw(rtc, &tm); > > + bcd2tm(&tm); > > + rtc_tm_to_time(&tm, &now); > > + rtc_time_to_tm(now + 2, &tm); > > + > > + if (tm2bcd(&tm) < 0) { > > + dev_err(&rtc->rtc->dev, "power off failed\n"); > > + return; > > + } > > + > > + rtc_wait_not_busy(rtc); > > + > > + rtc_write(rtc, OMAP_RTC_ALARM2_SECONDS_REG, tm.tm_sec); > > + rtc_write(rtc, OMAP_RTC_ALARM2_MINUTES_REG, tm.tm_min); > > + rtc_write(rtc, OMAP_RTC_ALARM2_HOURS_REG, tm.tm_hour); > > + rtc_write(rtc, OMAP_RTC_ALARM2_DAYS_REG, tm.tm_mday); > > + rtc_write(rtc, OMAP_RTC_ALARM2_MONTHS_REG, tm.tm_mon); > > + rtc_write(rtc, OMAP_RTC_ALARM2_YEARS_REG, tm.tm_year); > > + > > + /* > > + * enable ALARM2 interrupt > > + * > > + * NOTE: this fails on AM3352 if rtc_write (writeb) is used > > + */ > > + val = rtc_read(rtc, OMAP_RTC_INTERRUPTS_REG); > > + rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG, > > + val | OMAP_RTC_INTERRUPTS_IT_ALARM2); > > + > > + mdelay(2000); > > And it is uncommented. > > How on earth is a reader to know why this is here? The comment above the function reads: * The RTC can be used to control an external PMIC via the pmic_power_en pin, * which can be configured to transition to OFF on ALARM2 events. * * Notes: * The two-second alarm offset is the shortest offset possible as the alarm * registers must be set before the next timer update and the offset * calculation is too heavy for everything to be done within a single access * period (~15us). So it's effect is at least fairly obvious and documented. > I can do this > > --- a/drivers/rtc/rtc-omap.c~rtc-omap-add-support-for-pmic_power_en-v3-fix > +++ a/drivers/rtc/rtc-omap.c > @@ -417,6 +417,7 @@ static void omap_rtc_power_off(void) > rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG, > val | OMAP_RTC_INTERRUPTS_IT_ALARM2); > > + /* Allow alarm to trigger before returning */ > mdelay(2000); > } Looks good, and I should have put something like that there nonetheless. > But it doesn't explain *why* we want the alarm to trigger before > returning. Should we really require every power-off handler to document arch behaviour (even if its inconsistent and currently undocumented); in this case that some arches return to user-space where we may oops if called from process 0 (e.g. systemd, but not if using sysvinit)? Johan
On Tue, 28 Oct 2014 09:36:33 +0100 Johan Hovold <johan@kernel.org> wrote: > > But it doesn't explain *why* we want the alarm to trigger before > > returning. > > Should we really require every power-off handler to document arch > behaviour (even if its inconsistent and currently undocumented); in > this case that some arches return to user-space where we may oops if > called from process 0 (e.g. systemd, but not if using sysvinit)? The kernel really doesn't have a problem related to excessive amounts of useful code comments :( The bottom line is: did I provide a reader with the ability to understand why the code is this way? If "no" then improvements beckon. This does look like one code site where an elaborate explanation is warranted. There's no way in which a reader can get to your above paragraph from the current rtc-omap.c.
On Tue, Oct 28, 2014 at 02:18:05PM -0700, Andrew Morton wrote: > On Tue, 28 Oct 2014 09:36:33 +0100 Johan Hovold <johan@kernel.org> wrote: > > > > But it doesn't explain *why* we want the alarm to trigger before > > > returning. > > > > Should we really require every power-off handler to document arch > > behaviour (even if its inconsistent and currently undocumented); in > > this case that some arches return to user-space where we may oops if > > called from process 0 (e.g. systemd, but not if using sysvinit)? > > The kernel really doesn't have a problem related to excessive amounts > of useful code comments :( > > The bottom line is: did I provide a reader with the ability to > understand why the code is this way? If "no" then improvements beckon. > > This does look like one code site where an elaborate explanation is > warranted. There's no way in which a reader can get to your above > paragraph from the current rtc-omap.c. I agree with you that I should add a comment on why that mdelay is there to make it perfectly clear and obvious that it's purpose is to let the alarm trigger, which in turn causes the pmic to power off the system. It is a power-off handler, and any power-off handler should not return until it has at least attempted to power off the system. In this case, that mandates a two-second delay. So far, so good. I don't agree with you that every power-off handler should document what happens if it fails to power-off the system. That is, to describe what arches will happily return to user space, and under what conditions (e.g. what init system is used) that this will cause a panic. If anything that belongs in arch code or kernel/reboot.c, and is also something that is likely to change over time (consider the power-off-handler call chains). Johan
On Tue, Oct 28, 2014 at 09:36:33AM +0100, Johan Hovold wrote: > On Mon, Oct 27, 2014 at 03:40:31PM -0700, Andrew Morton wrote: > > On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold <johan@kernel.org> wrote: > But in general, how do you want to handle updates to a single patch in a > series you already have in your tree? Do you prefer a proper > incremental-fix patch (with commit message), just an updated single > patch, or a resend of the whole series? How should I best send the updated patch? Can you just replace the current three incremental patches: rtc-omap-add-support-for-pmic_power_en.patch rtc-omap-add-support-for-pmic_power_en-v3.patch rtc-omap-add-support-for-pmic_power_en-v3-fix.patch that you have in your tree, with a single new v4 which adds a more elaborate comment? Thanks, Johan
On Wed, 29 Oct 2014 13:50:05 +0100 Johan Hovold <johan@kernel.org> wrote: > On Tue, Oct 28, 2014 at 09:36:33AM +0100, Johan Hovold wrote: > > On Mon, Oct 27, 2014 at 03:40:31PM -0700, Andrew Morton wrote: > > > On Mon, 27 Oct 2014 09:09:28 +0100 Johan Hovold <johan@kernel.org> wrote: > > > But in general, how do you want to handle updates to a single patch in a > > series you already have in your tree? Do you prefer a proper > > incremental-fix patch (with commit message), just an updated single > > patch, or a resend of the whole series? > > How should I best send the updated patch? Can you just replace the > current three incremental patches: > > rtc-omap-add-support-for-pmic_power_en.patch > rtc-omap-add-support-for-pmic_power_en-v3.patch > rtc-omap-add-support-for-pmic_power_en-v3-fix.patch > > that you have in your tree, with a single new v4 which adds a more > elaborate comment? Yep, that works. I'll almost always turn the new patch into an incremental, mainly so that I (and others) can see what was changed.
--- a/drivers/rtc/rtc-omap.c~rtc-omap-add-support-for-pmic_power_en-v3-fix +++ a/drivers/rtc/rtc-omap.c @@ -417,6 +417,7 @@ static void omap_rtc_power_off(void) rtc_writel(rtc, OMAP_RTC_INTERRUPTS_REG, val | OMAP_RTC_INTERRUPTS_IT_ALARM2); + /* Allow alarm to trigger before returning */ mdelay(2000); }