* Thomas Gleixner <tglx@xxxxxxxxxxxxx> [141113 14:27]:
> On Thu, 13 Nov 2014, Tony Lindgren wrote:
> > Oops thanks for catching that. As the devres stuff is separate, I've
> > updated the patch to keep it that way by adding a minimal manage.h.
> > This avoids including internals.h in devres.c. Does that seem usable
> > for you?
>
> What's wrong with internals.h? devres.c is core code, so it is not
> affected of the ban to include internals.h :)
No problem, just that we need to bring in few other includes and
devres.c is currently free of any core irq stuff :) I can switch to
internals.h no problem if you prefer that.
> > + * So if replaying the lost device interrupts is absolutely needed from the
> > + * hardware point of view, it's probably best to set up a completely
> > + * separate wake-up interrupt handler for the wake-up interrupt in the
> > + * device driver because of the reasons above.
>
> Can we please kill this last paragraph? I'm already seeing the
> gazillion of "I think it is required to do so for my soooo special
> chip" implementations in random drivers which all get it wrong again.
OK :)
> So I'd rather provide a mechanism upfront which lets the driver know
> that the wakeup interrupt originated from that device, i.e. let the
> wake up handler call
>
> pm_wakeup_irq(dev);
>
> which calls:
>
> pm_runtime_mark_last_busy(dev);
> pm_request_resume(dev);
>
> and aside of that tells the device via a flag or preferrably a
> sequence counter that the wakeup irq has been triggered. So affected
> devices can handle it based on that information w/o implementing the
> next broken variant of wakeup irq handlers.
OK I'll take a look if we can just set some pm_runtime flag and use
the pm_runtime counters for that.
> That also allows to remove the wakeflags check for level/edge.
>
> > + */
> > +int init_disabled_wakeirq(struct device *dev, unsigned int wakeirq,
> > + unsigned long wakeflags)
> > +{
> > + if (!(dev && wakeirq)) {
>
> This is the second time I stumbled over this. While it is correct it
> would be simpler to parse
>
> if (!dev || !wakeirq) {
>
> At least for my review damaged brain :)
Heh !!true.
> > + pr_err("Missing device or wakeirq for %s irq %d\n",
> > + dev_name(dev), wakeirq);
> > + return -EINVAL;
> > + }
> > +
> > + if (!(wakeflags & IRQF_ONESHOT)) {
> > + pr_err("Invalid wakeirq for %s irq %d, must be oneshot\n",
> > + dev_name(dev), wakeirq);
> > + return -EINVAL;
> > + }
>
> Is there a reason why we force the wakeirq into a threaded handler?
Yes the drivers may need to restore hardware state in the pm_runtime
calls and who knows what else drivers will be doing. So that too might
be a good reason to just set a flag in pm_runtime land.
Anyways, thanks for your comments. I'll post a complete series after
looking into the wake-up counters a bit.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
