| Message ID | 20250304033731.1762899-1-haibo.chen@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | can: flexcan: disable transceiver during system PM | expand |
On 04/03/2025 at 12:37, haibo.chen@nxp.com wrote: > From: Haibo Chen <haibo.chen@nxp.com> > > During system PM, if no wakeup requirement, disable transceiver to > save power. > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>> --- > drivers/net/can/flexcan/flexcan-core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c > index b347a1c93536..74f3137295fb 100644 > --- a/drivers/net/can/flexcan/flexcan-core.c > +++ b/drivers/net/can/flexcan/flexcan-core.c > @@ -2292,6 +2292,7 @@ static int __maybe_unused flexcan_suspend(struct device *device) > return err; > > flexcan_chip_interrupts_disable(dev); > + flexcan_transceiver_disable(priv); flexcan_transceiver_disable() may return an error. Can you check its return value? > err = pinctrl_pm_select_sleep_state(device); > if (err) > @@ -2325,6 +2326,10 @@ static int __maybe_unused flexcan_resume(struct device *device) > if (err) > return err; > > + err = flexcan_transceiver_enable(priv); > + if (err) > + return err; > + > err = flexcan_chip_start(dev); > if (err) > return err; If flexcan_chip_start() fails, shouldn't you have a clean-up branch that will flexcan_transceiver_disable()? Or do you consider that it is acceptable to keep the transceiver enabled if an error occurs? Speaking of which, if flexcan_resume() fails, priv->can.state would still be set to CAN_STATE_ERROR_ACTIVE. Yours sincerely, Vincent Mailhol
> -----Original Message----- > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > Sent: 2025年3月4日 12:06 > To: Bough Chen <haibo.chen@nxp.com>; mkl@pengutronix.de > Cc: Ciprian Marian Costea (OSS) <ciprianmarian.costea@oss.nxp.com>; > u.kleine-koenig@baylibre.com; linux-can@vger.kernel.org; > linux-kernel@vger.kernel.org; imx@lists.linux.dev > Subject: Re: [PATCH] can: flexcan: disable transceiver during system PM > > On 04/03/2025 at 12:37, haibo.chen@nxp.com wrote: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > During system PM, if no wakeup requirement, disable transceiver to > > save power. > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>> --- > > drivers/net/can/flexcan/flexcan-core.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/net/can/flexcan/flexcan-core.c > > b/drivers/net/can/flexcan/flexcan-core.c > > index b347a1c93536..74f3137295fb 100644 > > --- a/drivers/net/can/flexcan/flexcan-core.c > > +++ b/drivers/net/can/flexcan/flexcan-core.c > > @@ -2292,6 +2292,7 @@ static int __maybe_unused flexcan_suspend(struct > device *device) > > return err; > > > > flexcan_chip_interrupts_disable(dev); > > + flexcan_transceiver_disable(priv); > > flexcan_transceiver_disable() may return an error. Can you check its return > value? Yes, sure. > > > err = pinctrl_pm_select_sleep_state(device); > > if (err) > > @@ -2325,6 +2326,10 @@ static int __maybe_unused flexcan_resume(struct > device *device) > > if (err) > > return err; > > > > + err = flexcan_transceiver_enable(priv); > > + if (err) > > + return err; > > + > > err = flexcan_chip_start(dev); > > if (err) > > return err; > > If flexcan_chip_start() fails, shouldn't you have a clean-up branch that will > flexcan_transceiver_disable()? Or do you consider that it is acceptable to keep > the transceiver enabled if an error occurs? Good suggestion, I will add a clean-up branch. > > Speaking of which, if flexcan_resume() fails, priv->can.state would still be set to > CAN_STATE_ERROR_ACTIVE. Thanks for your quick response, I will take all these suggestions into consideration and prepare v2 patch. Regards Haibo Chen > > > Yours sincerely, > Vincent Mailhol
diff --git a/drivers/net/can/flexcan/flexcan-core.c b/drivers/net/can/flexcan/flexcan-core.c index b347a1c93536..74f3137295fb 100644 --- a/drivers/net/can/flexcan/flexcan-core.c +++ b/drivers/net/can/flexcan/flexcan-core.c @@ -2292,6 +2292,7 @@ static int __maybe_unused flexcan_suspend(struct device *device) return err; flexcan_chip_interrupts_disable(dev); + flexcan_transceiver_disable(priv); err = pinctrl_pm_select_sleep_state(device); if (err) @@ -2325,6 +2326,10 @@ static int __maybe_unused flexcan_resume(struct device *device) if (err) return err; + err = flexcan_transceiver_enable(priv); + if (err) + return err; + err = flexcan_chip_start(dev); if (err) return err;