Re: [PATCH 2/5] PM / Wakeirq: Add automated device wake IRQ handling

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3777 bytes --]

Hi,

On Fri, May 15, 2015 at 03:25:13PM -0700, Tony Lindgren wrote:

<snip>

> diff --git a/drivers/base/power/wakeirq.c b/drivers/base/power/wakeirq.c
> new file mode 100644
> index 0000000..1125481
> --- /dev/null
> +++ b/drivers/base/power/wakeirq.c
> @@ -0,0 +1,276 @@
> +/*
> + * wakeirq.c - Device wakeirq helper functions
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/slab.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
> +
> +#include "power.h"
> +
> +/**
> + * dev_pm_attach_wake_irq - Attach device interrupt as a wake IRQ
> + * @dev: Device entry
> + * @irq: Device wake-up capable interrupt
> + * @wirq: Wake irq specific data
> + *
> + * Internal function to attach either a device IO interrupt or a
> + * dedicated wake-up interrupt as a wake IRQ.
> + */
> +static int dev_pm_attach_wake_irq(struct device *dev, int irq,
> +				  struct wake_irq *wirq)
> +{
> +	unsigned long flags;
> +	int err;
> +
> +	if (!dev || !wirq)
> +		return -EINVAL;
> +
> +	if (!dev->power.wakeup) {
> +		dev_err(dev, "forgot to call call device_init_wakeup?\n");
> +		return -EINVAL;
> +	}
> +
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	if (WARN_ON(dev->power.wakeirq)) {
> +		dev_err(dev, "wake irq already initialized\n");

these two can be combined if you can live with a WARN_ONCE() instead:

	if (dev_WARN_ONCE(dev, dev->power.wakeirq,
		"wake irq already initialized\n")) {
		spin_unlock_irqrestore(&dev->power.lock, flags);
		return -EEXIST;
	}

dev_WARN() needs to be fixed at some point to accept a "condition"
argument :s

But really, no strong feelings.

> +static irqreturn_t handle_threaded_wakeirq(int wakeirq, 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;

I wonder if you should add a pm_runtime_mark_last_busy() here. I guess
not after your previous patch ?

> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 7726200..7191519 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -238,6 +239,100 @@ int device_wakeup_enable(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(device_wakeup_enable);
>  
> +#ifdef CONFIG_PM_WAKEIRQ
> +
> +/**
> + * device_wakeup_attach_irq - Attach a wakeirq to a wakeup source
> + * @dev: Device to handle
> + * @irq: Device specific wakeirq entry

s/irq/wakeirq to match argument name below ?

> + * Attach a device specific wakeirq to the device specific
> + * wakeup source so the device wakeirq can be configured
> + * automatically for suspend and resume.
> + */
> +int device_wakeup_attach_irq(struct device *dev,
> +			     struct wake_irq *wakeirq)
> +{
> +	struct wakeup_source *ws;
> +	int ret = 0;
> +
> +	spin_lock_irq(&dev->power.lock);
> +	ws = dev->power.wakeup;
> +	if (!ws) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (ws->wakeirq) {
> +		ret = -EEXIST;
> +		goto unlock;
> +	}
> +
> +	ws->wakeirq = wakeirq;
> +
> +unlock:
> +	spin_unlock_irq(&dev->power.lock);
> +
> +	return ret;
> +}

<snip>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]