Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751992AbeACMkk (ORCPT + 1 other); Wed, 3 Jan 2018 07:40:40 -0500 Received: from fllnx209.ext.ti.com ([198.47.19.16]:9334 "EHLO fllnx209.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751376AbeACMkg (ORCPT ); Wed, 3 Jan 2018 07:40:36 -0500 Subject: Re: [PATCH v6 3/6] can: m_can: Add PM Runtime To: Marc Kleine-Budde , , , CC: , , , , , , , , References: <1513949488-13026-1-git-send-email-faiz_abbas@ti.com> <1513949488-13026-4-git-send-email-faiz_abbas@ti.com> From: Faiz Abbas Message-ID: <1308ee81-a1e9-ac5a-2d97-334fa825ef8d@ti.com> Date: Wed, 3 Jan 2018 18:09:33 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Hi, On Tuesday 02 January 2018 09:37 PM, Marc Kleine-Budde wrote: > On 12/22/2017 02:31 PM, Faiz Abbas wrote: >> From: Franklin S Cooper Jr >> >> Add support for PM Runtime which is the new way to handle managing clocks. >> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk >> management approach in place. > > There is no PM_RUNTIME anymore since 464ed18ebdb6 ("PM: Eliminate > CONFIG_PM_RUNTIME") Ok. Will change the commit message. > > Have a look at the discussion: https://patchwork.kernel.org/patch/9436507/ : > >>> Well, I admit it would be nicer if drivers didn't have to worry about >>> whether or not CONFIG_PM was enabled. A slightly cleaner approach >>> from the one outlined above would have the probe routine do this: >>> >>> my_power_up(dev); >>> pm_runtime_set_active(dev); >>> pm_runtime_get_noresume(dev); >>> pm_runtime_enable(dev); This discussion seems to be about cases in which CONFIG_PM is not enabled. CONFIG_PM is always selected in the case of omap devices. > >> PM_RUNTIME is required by OMAP based devices to handle clock management. >> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP >> to work with this driver. > > Who will set the SET_RUNTIME_PM_OPS in this case? It is set with a common SET_RUNTIME_PM_OPS in the case of omap at arch/arm/mach-omap2/omap_device.c:632 struct dev_pm_domain omap_device_pm_domain = { .ops = { SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume, NULL) USE_PLATFORM_PM_SLEEP_OPS SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(_od_suspend_noirq, _od_resume_noirq) } }; > >> Signed-off-by: Franklin S Cooper Jr >> [nsekhar@ti.com: handle pm_runtime_get_sync() failure, fix some bugs] >> Signed-off-by: Sekhar Nori >> Signed-off-by: Faiz Abbas >> --- >> drivers/net/can/m_can/m_can.c | 38 ++++++++++++++++++++++++++++++++++---- >> 1 file changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c >> index f72116e..53e764f 100644 >> --- a/drivers/net/can/m_can/m_can.c >> +++ b/drivers/net/can/m_can/m_can.c >> @@ -23,6 +23,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> >> @@ -625,19 +626,33 @@ static int m_can_clk_start(struct m_can_priv *priv) >> { >> int err; >> >> + err = pm_runtime_get_sync(priv->device); >> + if (err) { >> + pm_runtime_put_noidle(priv->device); > > Why do you call this in case of an error? pm_runtime_get_sync() increments the usage count of the device before any error is returned. This needs to be decremented using pm_runtime_put_noidle(). > >> + return err; >> + } >> + >> err = clk_prepare_enable(priv->hclk); >> if (err) >> - return err; >> + goto pm_runtime_put; >> >> err = clk_prepare_enable(priv->cclk); >> if (err) >> - clk_disable_unprepare(priv->hclk); >> + goto disable_hclk; >> >> return err; >> + >> +disable_hclk: >> + clk_disable_unprepare(priv->hclk); >> +pm_runtime_put: >> + pm_runtime_put_sync(priv->device); >> + return err; >> } >> >> static void m_can_clk_stop(struct m_can_priv *priv) >> { >> + pm_runtime_put_sync(priv->device); >> + >> clk_disable_unprepare(priv->cclk); >> clk_disable_unprepare(priv->hclk); >> } >> @@ -1577,13 +1592,20 @@ static int m_can_plat_probe(struct platform_device *pdev) >> /* Enable clocks. Necessary to read Core Release in order to determine >> * M_CAN version >> */ >> + pm_runtime_enable(&pdev->dev); >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (ret) { >> + pm_runtime_put_noidle(&pdev->dev); > > Why do you call this in case of error? Same here. Thanks, Faiz