Re: [PATCH 3/3] RFC: net: smsc911x: add wake-up event interrupt support

[Sudeep Holla <sudeep.holla@arm.com>]

On 08/07/16 10:07, Linus Walleij wrote:
> The SMSC911x have a line out of the chip called "PME",
> Power Management Event. When connected to an asynchronous
> interrupt controller this is able to wake the system up
> from sleep in response to certain network events.
>
> This is the first attempt to support this in the Linux
> driver: the Qualcomm APQ8060 Dragonboard has this line
> routed to a GPIO line on the primary SoC padring, and as
> such it can be armed as a wakeup interrupt.
>
> The patch is inspired by the wakeup code in the RTC
> subsystem.
>
> The code looks for an additional interrupt - apart from the
> ordinary device interrupt - and in case that is present,
> we register an interrupt handler to respons to this,
> and flag the device and this interrupt as a wakeup.
>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Added some wakeup people at CC who can (hopefully) tell me if
> I'm doing this right or not.
> ---
>   drivers/net/ethernet/smsc/smsc911x.c | 48 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 48 insertions(+)
>
> diff --git a/drivers/net/ethernet/smsc/smsc911x.c b/drivers/net/ethernet/smsc/smsc911x.c
> index 125d58ac22bd..e43755ae130a 100644
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c

[...]

> @@ -2525,6 +2540,33 @@ static int smsc911x_drv_probe(struct platform_device *pdev)
>   		goto out_disable_resources;
>   	}
>
> +	irq = platform_get_irq(pdev, 1);
> +	if (irq == -EPROBE_DEFER) {
> +		retval = -EPROBE_DEFER;
> +		goto out_disable_resources;
> +	/* It's perfectly fine to not have a PME IRQ */
> +	} else if (irq > 0) {
> +		char *pme_name;
> +
> +		/*
> +		 * The Power Management Event (PME) IRQ appears as
> +		 * a pulse waking up the system from sleep in response to  a
> +		 * network event.
> +		 */
> +		retval = request_threaded_irq(irq, NULL,
> +					      smsc911x_pme_irq_thread,
> +					      IRQF_ONESHOT, "smsc911x-pme",
> +					      dev);
> +		if (retval) {
> +			SMSC_WARN(pdata, probe,
> +			"Unable to claim requested PME irq: %d", irq);
> +			goto out_disable_resources;
> +		}
> +		pdata->pme_irq = irq;
> +		device_init_wakeup(&pdev->dev, true);
> +		dev_pm_set_wake_irq(&pdev->dev, irq);
> +	}
> +
>   	netif_carrier_off(dev);
>
>   	retval = register_netdev(dev);
> @@ -2613,6 +2655,9 @@ static int smsc911x_suspend(struct device *dev)
>   		PMT_CTRL_PM_MODE_D1_ | PMT_CTRL_WOL_EN_ |
>   		PMT_CTRL_ED_EN_ | PMT_CTRL_PME_EN_);
>
> +	if (pdata->pme_irq && device_may_wakeup(dev))
> +		enable_irq_wake(pdata->pme_irq);
> +

Based on my understanding of dev_pm_{set,clear}_wake_irq, we don't need
this and the below hunk. It's taken care in device_wakeup_arm_wake_irqs
which is called from suspend_enter---->dpm_suspend_noirq path once you
have done the setup with dev_pm_set_wake_irq and is enabled both of
which is true in your case.

>   	return 0;
>   }
>
> @@ -2622,6 +2667,9 @@ static int smsc911x_resume(struct device *dev)
>   	struct smsc911x_data *pdata = netdev_priv(ndev);
>   	unsigned int to = 100;
>
> +	if (pdata->pme_irq && device_may_wakeup(dev))
> +		disable_irq_wake(pdata->pme_irq);
> +

Same as above.

-- 
Regards,
Sudeep