RE: [PATCH V4 1/1] can: flexcan: add self wakeup support

[Joakim Zhang <qiangqing.zhang@nxp.com>]

Hi Aisheng,


> -----Original Message-----
> From: Aisheng DONG
> Sent: 2018年11月22日 10:31
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; linux-can@vger.kernel.org;
> mkl@pengutronix.de
> Cc: wg@grandegger.com; linux-kernel@vger.kernel.org; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: RE: [PATCH V4 1/1] can: flexcan: add self wakeup support
> 
> [...]
> 
> > +
> > +static int __maybe_unused flexcan_noirq_suspend(struct device
> > +*device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	if (netif_running(dev) && device_may_wakeup(device))
> > +		flexcan_enable_wakeup_irq(priv, true);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused flexcan_noirq_resume(struct device *device) {
> > +	struct net_device *dev = dev_get_drvdata(device);
> > +	struct flexcan_priv *priv = netdev_priv(dev);
> > +
> > +	if (netif_running(dev) && device_may_wakeup(device)) {
> > +		disable_irq_wake(dev->irq);
> 
> A bit more thinking:
> Can we put flexcan_enable_wakeup_irq(priv, false) here and move
> disable_irq_wake to resume function?
> Then it looks better on pairs for those functions.
> 
> I'm not sure if irq will be lost or we may even not need wakeup irq.
> Please help check it.

1. I have try that flexcan_enable_wakeup_irq(priv, false) can move into flexcan_noirq_resume() and disable_irq_wake() can move into flexcan_resume().
  If you think that is better, I will modify it.

2. If we don't disable wakeup irq after it exits stop mode, it will continue enter wakeup interrupt handle function when the wakeup event arriving during suspend after
  the system has done once more suspend/resume.

Best Regards,
Joakim Zhang

> Regards
> Dong Aisheng
> 
> > +		flexcan_exit_stop_mode(priv);
> >  	}
> > +
> >  	return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(flexcan_pm_ops, flexcan_suspend,
> > flexcan_resume);
> > +static const struct dev_pm_ops flexcan_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(flexcan_suspend, flexcan_resume)
> > +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(flexcan_noirq_suspend,
> > +flexcan_noirq_resume) };
> >
> >  static struct platform_driver flexcan_driver = {
> >  	.driver = {
> > --
> > 2.17.1