Message-ID: <alpine.LFD.2.00.1103112020370.2787@localhost6.localdomain6>
Date:	Fri, 11 Mar 2011 20:43:08 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Abhijeet Dharmapurikar <adharmap@...eaurora.org>
cc:	davidb@...eaurora.org, "David S. Miller" <davem@...emloft.net>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Bryan Huntsman <bryanh@...eaurora.org>,
	Daniel Walker <dwalker@...o99.com>,
	David Collins <collinsd@...eaurora.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Joe Perches <joe@...ches.com>,
	Russell King <linux@....linux.org.uk>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Stepan Moskovchenko <stepanm@...eaurora.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Linus Walleij <linus.walleij@...rricsson.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-arm-msm@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [Qualcomm PM8921 MFD v2 2/6] mfd: pm8xxx: Add irq support

On Fri, 11 Mar 2011, Abhijeet Dharmapurikar wrote:
> > > Yes however while updating the code I noticed that I would need to keep
> > > account of all the interrupts enabled and all the interrupts marked
> > > wakeup.
> > > This aids in switching to the wakeup set in the suspend callback and the
> > > enabled set in the resume callback. I will update the resume callback to
> > > enable the interrupts in irqs_allowed(the local state storage) in the next
> > > patch (my current patch does not do that).
> > > 
> > > IOW I need to keep the local state storage.
> > 
> > Wrong. The interrupts are disabled and reenabled by the core code and
> > not by some extra suspend/resume callbacks in your driver. The core
> > checks those marked as IRQ_WAKE, the wake callback to the irq chip is
> > only there if you need to set up some hardware register in order to
> > make the wake functionality work. So again, you don't need local state
> > as the core tracks the state for you.
> 
> Help me understand this, the core code calls disable on all the interrupts
> while going to suspend. Notice that I have no disable callback, which means
> those interrupts remain unmasked.
>
> The genirq code does not mask the interrupt while going to suspend, it only
> calls disable(), which I understand should not mask the interrupt for
> check_wakeup_irqs() to work.
>
> If I don't mask that accelerometer interrupts in the interrupt controller's
> suspend() the phone will wakeup every time the user moves around, draining the
> battery unnecessarily.

That's why we mark the interrupts which can wake up from suspend with
set_wake() so you can configure your hardware accordingly. That's how
all other stuff works, at least how it's supposed to work. 

If there is no way to tell the interrupt controller which interrupts
are wakeup sources and which are not, then working around it with
local state and private suspend/resume functions is the WRONG answer.

Simply because this kind of misdesigned hardware will creep up over
and over and we want to handle these cases in the core. Even for a
sinlge instance like yours solving it in the core is the right thing
to do, because it's a ~3 lines patch to the core code to get this
done.

Sigh, why insist people on working around core code instead of talking
to the responsible maintainers about their problem in the first place?

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists