> -----Original Message-----
> From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Sent: 2019年2月27日 18:55
> To: kernel@xxxxxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx
> Cc: Aisheng Dong <aisheng.dong@xxxxxxx>; Joakim Zhang
> <qiangqing.zhang@xxxxxxx>
> Subject: Re: [PATCH v5] can: flexcan: implement can Runtime PM
>
> On 2/27/19 11:41 AM, Marc Kleine-Budde wrote:
> > From: Aisheng Dong <aisheng.dong@xxxxxxx>
> >
> > Flexcan will be disabled during suspend if no wakeup function required
> > and enabled after resume accordingly. During this period, we could
> > explicitly disable clocks.
> > Since PM is optional, the clock is enabled at probe to guarante the
> > clock is running when PM is not enabled in the kernel.
> >
> > Implement Runtime PM which will:
> > 1) Without CONFIG_PM, clock is running whether Flexcan up or down.
> > 2) With CONFIG_PM, clock enabled while Flexcan up and disabled when
> > Flexcan down.
> > 3) Disable clock when do system suspend and enable clock while system
> > resume.
> > 4) Make Power Domain framework be able to shutdown the corresponding
> > power domain of this device.
>
> [...]
>
> > @@ -1607,7 +1626,7 @@ static int __maybe_unused
> flexcan_suspend(struct
> > device *device) {
> > struct net_device *dev = dev_get_drvdata(device);
> > struct flexcan_priv *priv = netdev_priv(dev);
> > - int err;
> > + int err = 0;
Hi Marc,
Thank you for all your effort, you are welcome.
Add this error handing could be more reasonable.
Here still keep:
int err;
> >
> > if (netif_running(dev)) {
> > /* if wakeup is enabled, enter stop mode @@ -1620,20 +1639,22
> @@
> > static int __maybe_unused flexcan_suspend(struct device *device)
> > err = flexcan_chip_disable(priv);
> > if (err)
> > return err;
> > +
> > + err = pm_runtime_force_suspend(device);
>
> One thing I noticed is the missing error handling...
We can add error handling like this:
err = pm_runtime_force_suspend(device);
if(err)
return err
> > }
> > netif_stop_queue(dev);
> > netif_device_detach(dev);
>
> ... you'll call these two functions...
>
> > }
> > priv->can.state = CAN_STATE_SLEEPING;
> >
> > - return 0;
> > + return err;
>
> ... but still return the error.
Here we can directly return 0.
> > }
>
> In which state is the system in case of an error then?
System notes suspend failed in case of this error.
Best Regards,
Joakim Zhang
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
