From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jarkko Nikula Subject: Re: [RFT][PATCH 1/2] i2c: designware: Clean up PM handling in dw_i2c_plat_probe() Date: Tue, 5 Sep 2017 17:40:22 +0300 Message-ID: References: <3023226.l5IfJK6GIc@aspire.rjw.lan> <57866801.UnLEtqmHq8@aspire.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <57866801.UnLEtqmHq8@aspire.rjw.lan> Content-Language: en-US Sender: linux-acpi-owner@vger.kernel.org To: "Rafael J. Wysocki" , linux-pm@vger.kernel.org, linux-i2c@vger.kernel.org Cc: Wolfram Sang , linux-acpi@vger.kernel.org, Kevin Hilman , Andy Shevchenko , Mika Westerberg , Jisheng Zhang , John Stultz , Guodong Xu , Sumit Semwal , Haojian Zhuang , Johannes Stezenbach , Ulf Hansson List-Id: linux-i2c@vger.kernel.org Hi On 09/04/2017 02:08 AM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > The power management handling in dw_i2c_plat_probe() is somewhat > messy and it is rather hard to figure out the code intention for > the case when pm_disabled is set. In that case, the driver doesn't > enable runtime PM at all, but in addition to that it calls > pm_runtime_forbid() as though it wasn't sure if runtime PM might > be enabled for the device later by someone else. > > Although that concern doesn't seem to be actually valid, the > device is clearly still expected to be PM-capable even in the > pm_disabled set case, so a better approach would be to enable > runtime PM for it unconditionally and then prevent it from > being runtime suspended by using pm_runtime_forbid(). > > Make the driver do that as that will help to clean up its system > sleep handling in a relatively straightforward way. > > Signed-off-by: Rafael J. Wysocki > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 36 +++++++++++++++++++--------- > 1 file changed, 25 insertions(+), 11 deletions(-) > > Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c > =================================================================== > --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c > +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -249,6 +249,16 @@ static void dw_i2c_set_fifo_size(struct > } > } > > +static void dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev) > +{ > + pm_runtime_get_noresume(dev->dev); > + if (dev->pm_disabled) > + pm_runtime_allow(dev->dev); > + > + pm_runtime_disable(dev->dev); > + pm_runtime_put_noidle(dev->dev); > +} > + > static int dw_i2c_plat_probe(struct platform_device *pdev) > { > struct dw_i2c_platform_data *pdata = dev_get_platdata(&pdev->dev); > @@ -362,14 +372,19 @@ static int dw_i2c_plat_probe(struct plat > ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); > adap->dev.of_node = pdev->dev.of_node; > > - if (dev->pm_disabled) { > + /* The code below assumes runtime PM to be disabled. */ > + WARN_ON(pm_runtime_enabled(&pdev->dev)); > + > + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_set_active(&pdev->dev); > + > + pm_runtime_get_noresume(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + if (dev->pm_disabled) > pm_runtime_forbid(&pdev->dev); > - } else { > - pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); > - pm_runtime_use_autosuspend(&pdev->dev); > - pm_runtime_set_active(&pdev->dev); > - pm_runtime_enable(&pdev->dev); > - } > + > + pm_runtime_put_noidle(&pdev->dev); > Is pm_runtime_get_noresume()/pm_runtime_put_noidle() cycle needed here? My vague memory tells platform device won't power off instantly because of plain pm_runtime_enable() even if dev->power.usage_count is zero. I guess it was the pm_request_idle() in driver_probe_device() that triggered the power transition after probe. drivers/base/dd.c: driver_probe_device(): pm_runtime_barrier(dev); ret = really_probe(dev, drv); pm_request_idle(dev); -- Jarkko