On Monday, May 18, 2015 04:44:01 PM Tony Lindgren wrote:
> * Tony Lindgren <tony@xxxxxxxxxxx> [150518 15:06]:
> > +/**
> > + * dev_pm_set_wake_irq - Attach device IO interrupt as wake IRQ
> > + * @dev: Device entry
> > + * @irq: Device IO interrupt
> > + *
> > + * Attach a device IO interrupt as a wake IRQ. The wake IRQ gets
> > + * automatically configured for wake-up from suspend based
> > +void dev_pm_clear_wake_irq(struct device *dev)
> > +{
> > + struct wake_irq *wirq = dev->power.wakeirq;
> > + unsigned long flags;
> > +
> > + if (!wirq)
> > + return;
> > +
> > + device_wakeup_detach_irq(dev);
> > +
> > + spin_lock_irqsave(&dev->power.lock, flags);
> > + if (wirq->manage_irq) {
> > + free_irq(wirq->irq, wirq);
> > + wirq->manage_irq = false;
> > + }
> > + dev->power.wakeirq = NULL;
> > + spin_unlock_irqrestore(&dev->power.lock, flags);
> > +
> > + wirq->irq = -EINVAL;
> > + kfree(wirq);
> > +}
>
> I just noticed most of the dev_pm_clear_wake_irq is no longer needed.
> We're now freeing it anyways. so it can be just:
>
> void dev_pm_clear_wake_irq(struct device *dev)
> {
> struct wake_irq *wirq = dev->power.wakeirq;
> unsigned long flags;
>
> if (!wirq)
> return;
>
> spin_lock_irqsave(&dev->power.lock, flags);
> dev->power.wakeirq = NULL;
> spin_unlock_irqrestore(&dev->power.lock, flags);
>
> device_wakeup_detach_irq(dev);
> if (wirq->manage_irq)
> free_irq(wirq->irq, wirq);
> kfree(wirq);
> }
>
>
> Regards,
>
> Tony
>
> 8< ---------------------
> From: Tony Lindgren <tony@xxxxxxxxxxx>
> Date: Mon, 18 May 2015 15:40:29 -0700
> Subject: [PATCH] PM / Wakeirq: Add automated device wake IRQ handling
>
> Turns out we can automate the handling for the device_may_wakeup()
> quite a bit by using the kernel wakeup source list.
>
> And as some hardware has separate dedicated wake-up interrupt
> in addition to the IO interrupt, we can automate the handling by
> adding a generic threaded interrupt handler that just calls the
> device PM runtime to wake up the device.
>
> This allows dropping code from device drivers as we currently
> are doing it in multiple ways, and often wrong.
>
> For most drivers, we should be able to drop the following
> boilerplate code from runtime_suspend and runtime_resume
> functions:
>
> ...
> device_init_wakeup(dev, true);
> ...
> if (device_may_wakeup(dev)
> enable_irq_wake(irq);
> ...
> if (device_may_wakeup(dev)
> enable_irq_wake(irq);
Closing parens are missin in the above two if () statements.
Also, should the second one be disable_irq_wake(irq)?
> ...
> device_init_wakeup(dev, false);
> ...
>
> We can replace it with just the following init and exit
> time code:
>
> ...
> device_init_wakeup(dev, true);
> dev_pm_set_wake_irq(dev, irq);
> ...
> dev_pm_clear_wake_irq(dev);
> device_init_wakeup(dev, false);
> ...
>
> And for hardware with dedicated wake-up interrupts:
>
> ...
> device_init_wakeup(dev, true);
> dev_pm_set_dedicated_wake_irq(dev, irq);
> ...
> dev_pm_clear_wake_irq(dev);
> device_init_wakeup(dev, false);
> ...
>
> For now, let's only enable it for select PM_WAKEIRQ.
Why? What would be wrong with doing that unconditionally?
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
Looks good overall, a couple of nits below.
[cut]
> +/**
> + * handle_threaded_wake_irq - Handler for dedicated wake-up interrupts
> + * @irq: Device dedicated wake-up interrupt
> + * @_wirq: Wake IRQ data
> + *
> + * Some devices have a separate wake-up interrupt in addition to the
> + * device IO interrupt. The wake-up interrupts signal that the device
> + * should be woken up from a idle state. This handler uses device
> + * specific pm_runtime functions to wake the device and then it's
> + * up to the device to do whatever it needs to. Note as the device
> + * may need to restore context and start up regulators, we use a
> + * threaded IRQ.
> + *
> + * Also note that we are not resending the lost device interrupts.
> + * We assume that the wake-up interrupt just needs to wake-up the
> + * device, and the device pm_runtime_resume() can deal with the
> + * situation.
> + */
> +static irqreturn_t handle_threaded_wake_irq(int irq, void *_wirq)
> +{
> + struct wake_irq *wirq = _wirq;
> +
> + /* We don't want RPM_ASYNC or RPM_NOWAIT here */
> + return pm_runtime_resume(wirq->dev) ? IRQ_NONE : IRQ_HANDLED;
There are various reasons why pm_runtime_resume() may return error codes and
some of them don't mean that the interrupt was not legitimate.
Moreover, it returns 1 if the device is already active, in which case the above
check won't do any good to us.
Why not to return IRQ_HANDLED unconditionally from here?
[cut]
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 7e01f78..d3735bd 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -267,6 +267,10 @@ config PM_CLK
> def_bool y
> depends on PM && HAVE_CLK
>
> +config PM_WAKEIRQ
> + bool
> + depends on PM_SLEEP
> +
If you really really want this (I'm still not sure why exactly, though), it
should depend on PM_SLEEP || PM_RUNTIME, because the latter uses it too.
> config PM_GENERIC_DOMAINS
> bool
> depends on PM
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
