On Wed, 2008-04-09 at 00:48 -0700, David Brownell wrote:
> On Tuesday 08 April 2008, Zhao Yakui wrote:
> > + if (t->enabled)
> > + if (cmos->wake_on)
> > + cmos->wake_on(dev);
>
> That looks ugly (bracketing) ... and suspicious in
> terms of conformance to the RTC interface.
>
> Shouldn't it be:
>
> if (t->enabled) {
> if (cmos->wake_on)
> cmos->wake_on(dev);
> } else {
> if (cmos->wake_off)
> cmos->wake_off(dev);
> }
>
> instead? Else I don't see what would disable the
> alarm when it's supposed to be disabled. Plus, if
> that's done here, I suspect it also needs to be
> done in rtc_ioctl() for RTC_AIE_{ON,OFF} and *NOT*
> done in cmos_{suspend,resume)() ... very odd for a
> wakeup hook, since none of those presume the system
> is entering a state from which it could wake!
If alarm is only used as the wake event source from sleeping state, it
is enough to call hook function in suspend/resume.
But if the system is booted with acpi enabled , I suspect whether RTC
alarm can trigger RTC IRQ. If the RTC alarm is dedicated as the wake
event source, it will be also OK to call the hook function when the
alarm is enabled.
> But in general this raises the question of how
> to properly hook up to ACPI. If ACPI's hook is for
> normal alarm behavior (supplanting the RTC IRQ),
> that's odd ... and it shouldn't fit into a hook
> designed to ensure only that wake-from-sleep works.
>
> At any rate, this patch clearly can't merge as-is.
>
> - Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
