| Message ID | 1259068720-26113-1-git-send-email-nsekhar@ti.com |
|---|---|
| State | Accepted |
| Headers | show |
Hello, On Tue, Nov 24, 2009 at 18:48:40, Nori, Sekhar wrote: > The rtc-omap driver currently hardcodes the RTC to be > not capable of wakeup events. On the DA850/OMAP-L138 > SoC, the RTC is a wake up source from its "deep sleep" > mode. > > Let platform data set the wakeup capability flag instead. > > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> Any comments on this patch? Thanks, Sekhar
On Tue, 5 Jan 2010 13:46:17 +0530 "Nori, Sekhar" <nsekhar@ti.com> wrote: > > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> > > Any comments on this patch? please let me know if you get the Acked-by from the author.
Hi David, George, On Thu, Jan 21, 2010 at 05:07:13, Alessandro Zummo wrote: > On Tue, 5 Jan 2010 13:46:17 +0530 > "Nori, Sekhar" <nsekhar@ti.com> wrote: > > > > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > > > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com> > > > > Any comments on this patch? > > please let me know if you get the Acked-by from > the author. Would you please Ack this patch? The patch basically lets the platform data set the device wake-up capability flag for OMAP RTC instead of always hard-coding it to zero in RTC driver. This should not break any existing platforms since device_register() initializes this flag to zero anyway. Here is the link to the patch: http://patchwork.ozlabs.org/patch/39207/ Thanks, Sekhar
On Saturday 23 January 2010, Nori, Sekhar wrote: > Hi David, George, > > Would you please Ack this patch? The patch basically > lets the platform data set the device wake-up capability > flag for OMAP RTC instead of always hard-coding it to zero > in RTC driver. > > This should not break any existing platforms since > device_register() initializes this flag to zero anyway. > > Here is the link to the patch: > > http://patchwork.ozlabs.org/patch/39207/ This just deletes the device_init_wakeup() call and the comment describing how/why to use another setting. Where is the code to replace it and *set* those flags to a nonzero default, so the device can be woken up? That is, to achieve what $SUBJECT says this achieves?? The $SUBJECT is inaccurate. The patch description also has it wrong ... what I think you're trying to do is change the model for how the wakeup capability is configured. That's fine, and probably overdue. So I think maybe this needs changing. Minimally te code comment; maybe add a block noting chip-specific issues, where the OMAP1 chips can initialize their platform devices approppriately IFF their board is wired right, but OMAP-L138 doesn't have that issue (and so can unilaterally device_init_wakeup to true in its chip init logic, for all boards). And to be paired with a patch which makes the L138 boards behave right... maybe that chip init code already does that, if so mention that in the patch comments so we're not left guessing. I'll be glad to see this driver finally serving as a wakeup event source. I don't recall seeing OMAP1 boards wired so that it could do that; else this would have issued those events a *long* time ago! - Dave > > Thanks, > Sekhar > >
On Saturday 23 January 2010, David Brownell wrote: > So I think maybe this needs changing. Minimally te > code comment; maybe add a block noting chip-specific > issues, where the OMAP1 chips can initialize their > platform devices approppriately IFF their board is > wired right, but OMAP-L138 doesn't have that issue > (and so can unilaterally device_init_wakeup to true > in its chip init logic, for all boards). And likewise the header comment at top of file, which talks about how wiring RTC_WAKE_INT is a board-specific issue. Which is true for OMAP1, but not for L138 ...
Hi Dave, Thanks for the review and apologize for the late response. I was out for a while. On Sat, Jan 23, 2010 at 22:37:25, David Brownell wrote: > On Saturday 23 January 2010, Nori, Sekhar wrote: > > Hi David, George, > > > > Would you please Ack this patch? The patch basically > > lets the platform data set the device wake-up capability > > flag for OMAP RTC instead of always hard-coding it to zero > > in RTC driver. > > > > This should not break any existing platforms since > > device_register() initializes this flag to zero anyway. > > > > Here is the link to the patch: > > > > http://patchwork.ozlabs.org/patch/39207/ > > This just deletes the device_init_wakeup() call and the > comment describing how/why to use another setting. > > Where is the code to replace it and *set* those flags > to a nonzero default, so the device can be woken up? > That is, to achieve what $SUBJECT says this achieves?? That code is present in board code after the platform device is registered. For OMAP-L1 it is done in da8xx_register_rtc() in arch/arm/mach-davinci/devices-da8xx.c http://git.kernel.org/?p=linux/kernel/git/khilman/linux-davinci.git;a=blob;f=arch/arm/mach-davinci/devices-da8xx.c;h=0a96791d3b0f6dd8238c4e0826f3f452b3905477;hb=887f8d18de5efbeafc76e5c509fd53f9018992fa#l584 > > The $SUBJECT is inaccurate. The patch description > also has it wrong ... what I think you're trying to > do is change the model for how the wakeup capability > is configured. That's fine, and probably overdue. Right, I see that now. > > So I think maybe this needs changing. Minimally te > code comment; maybe add a block noting chip-specific > issues, where the OMAP1 chips can initialize their > platform devices approppriately IFF their board is > wired right, but OMAP-L138 doesn't have that issue > (and so can unilaterally device_init_wakeup to true > in its chip init logic, for all boards). > And likewise the header comment at top of file, > which talks about how wiring RTC_WAKE_INT is a > board-specific issue. Which is true for OMAP1, > but not for L138 ... Okay. > > And to be paired with a patch which makes the L138 > boards behave right... maybe that chip init code > already does that, if so mention that in the patch > comments so we're not left guessing. Okay. That code is already in Kevin's tree. I will refer to that in patch description. > > I'll be glad to see this driver finally serving as > a wakeup event source. I don't recall seeing OMAP1 > boards wired so that it could do that; else this > would have issued those events a *long* time ago! Yes, and it was fun seeing the rtc wake-up the OMAP-L138 from suspend. Thanks, Sekhar
On Friday 29 January 2010, Nori, Sekhar wrote: > > Hi Dave, > > Thanks for the review and apologize for the late response. > I was out for a while. Heh, my mailboxes are way overloaded ... I'm starting to catch up, be glad your note wasn't too deeply buried! > > So I think maybe this needs changing. Minimally ... > > Okay. > > ... > > I'll be glad to see this driver finally serving as > > a wakeup event source. I don't recall seeing OMAP1 > > boards wired so that it could do that; else this > > would have issued those events a *long* time ago! > > Yes, and it was fun seeing the rtc wake-up the OMAP-L138 > from suspend. Yeah, that's always a good milestone. RTC wakeup *SHOULD* be something almost every system can do, from suspend states. But it can get messy. Fortunately OMAP doesn't have to cope with buggy ACPI BIOS code, or broken-by-design hardware. ;) - Dave
diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c index 0587d53..ac9d55b 100644 --- a/drivers/rtc/rtc-omap.c +++ b/drivers/rtc/rtc-omap.c @@ -400,16 +400,11 @@ static int __init omap_rtc_probe(struct platform_device *pdev) /* BOARD-SPECIFIC CUSTOMIZATION CAN GO HERE: * - * - Boards wired so that RTC_WAKE_INT does something, and muxed - * right (W13_1610_RTC_WAKE_INT is the default after chip reset), - * should initialize the device wakeup flag appropriately. - * * - Boards wired so RTC_ON_nOFF is used as the reset signal, * rather than nPWRON_RESET, should forcibly enable split * power mode. (Some chip errata report that RTC_CTRL_SPLIT * is write-only, and always reads as zero...) */ - device_init_wakeup(&pdev->dev, 0); if (new_ctrl & (u8) OMAP_RTC_CTRL_SPLIT) pr_info("%s: split power mode\n", pdev->name);