From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tony Lindgren Subject: Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup Date: Fri, 14 Nov 2014 09:08:17 -0800 Message-ID: <20141114170816.GW26481@atomide.com> References: <20140919161927.GA28613@kahuna> <20140919191649.GQ14505@atomide.com> <20141002034345.GH3122@atomide.com> <20141106204629.GF31454@atomide.com> <20141113174030.GM26481@atomide.com> <20141114161901.GG11538@saruman> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20141114161901.GG11538@saruman> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Felipe Balbi Cc: Thomas Gleixner , Nishanth Menon , lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, LKML , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Keerthy , Mark Brown , Samuel Ortiz , linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, LAK , Kevin Hilman List-Id: devicetree@vger.kernel.org * Felipe Balbi [141114 08:20]: > On Thu, Nov 13, 2014 at 09:40:31AM -0800, Tony Lindgren wrote: > > +/** > > + * handle_wakeirq_thread - call device runtime pm calls on wake-up interrupt > > + * @wakeirq: device specific wake-up interrupt > > + * @dev_id: struct device entry > > + */ > > +irqreturn_t handle_wakeirq_thread(int wakeirq, void *dev_id) > > +{ > > + struct device *dev = dev_id; > > + irqreturn_t ret = IRQ_NONE; > > + > > + if (pm_runtime_suspended(dev)) { > > + pm_runtime_mark_last_busy(dev); > > + pm_request_resume(dev); > > this assumes that every driver's ->resume() callback has a: > > if (pending) > handle_pending_irqs(); > > which might not be very nice. I'd rather follow what Thomas suggested > and always pass device irq so this can mark it pending. Keep in mind > that we *don't* need a pm_runtime_get_sync() in every IRQ handler > because of that. Adding it is but the easiest way to get things working > and, quite frankly, very silly. > > what we want is rather: > > irqreturn_t my_handler(int irq, void *dev_id) > { > struct device *dev = dev_id; > > if (pm_runtime_suspended(dev)) { > pending_irqs_to_be_handled_from_runtime_resume = true; > pm_runtime_get(dev); > clear_irq_source(dev); > return IRQ_HANDLED; > } > } > > or something similar. Yeah I'll take a look. > > + ret = IRQ_HANDLED; > > + } > > you're not masking the wake irq here which means that when this handler > returns, wake irq will be unmasked by core IRQ subsystem leaving it > unmasked after ->resume(). It currently assumes the consumer driver takes care of it. But I get your point, we should be able to automate this further. And right now there's also a dependency on dev->power.irq_safe so RPM_ASYNC is not set. And this all should ideally work even with runtime PM not set as it's also needed for resume from suspend. > you *know* you'll pass a NULL top half handler, why don't you just force > IRQF_ONESHOT instead of erroring out ? Just add: > > wakeflags |= IRQF_ONESHOT; > > and get it over with :-) Good point :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html