On Tuesday, May 19, 2015 04:04:43 PM Rafael J. Wysocki wrote:
> 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?
I mean, what about making it depend on CONFIG_PM directly?
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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
