| Message ID | 1465916848-8207-4-git-send-email-ulf.hansson@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 15 June 2016 at 14:47, Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > On 06/14/2016 06:07 PM, Ulf Hansson wrote: >> >> In cases when the designware specific flag "pm_runtime_disable" is set, >> ->probe() calls pm_runtime_forbid() for the device, but without enabling >> runtime PM. This increases the runtime PM usage count for the device, but >> as runtime PM is disabled it's pointless. >> > As far as I remember reason for that was to prevent suspending the device > when unloading the driver. Hmm. To me that's a weird way of implementing this. Moreover, as i2c_dw_disable() indeed is called in ->remove() how will the pm_runtime_forbid() help to avoid that from happen? > >> Let's instead convert to unconditionally enable runtime PM, which has the >> effect of making a parent device aware of its child. >> >> To also maintain the old behaviour when the "pm_runtime_disable" flag is >> set, which prevents userspace to enable runtime PM suspend via sysfs, >> switch from calling pm_runtime_forbid() into pm_runtime_get_noresume() >> during ->probe(). >> >> While changing this, let's also also correct the error path in ->probe() >> and fix ->remove(), as to decrease the runtime PM usage count when it has >> been in increased by pm_runtime_forbid() (after this change, by >> pm_runtime_get_noresume()). >> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/i2c/busses/i2c-designware-platdrv.c | 25 >> +++++++++++++------------ >> 1 file changed, 13 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c >> b/drivers/i2c/busses/i2c-designware-platdrv.c >> index 19174e7..94ff953 100644 >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >> @@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device >> *pdev) >> adap->dev.of_node = pdev->dev.of_node; > > ... >> >> + if (dev->pm_runtime_disabled) >> + pm_runtime_get_noresume(&pdev->dev); > > >> @@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device >> *pdev) >> >> i2c_dw_disable(dev); >> >> + if (dev->pm_runtime_disabled) >> + pm_runtime_put_noidle(&pdev->dev); > > > That will trigger power down after returning from dw_i2c_plat_remove() by > looking at debug print from acpi_device_set_power() but now I don't find > what call chain is actually causing it. Oh, I didn't realize that by power down you meant the acpi power down. Can you elaborate on why power down needs to be prevented? Is there a problem to power on the device again in the next ->probe() attempt? > > Must be something to do with drivers/acpi/acpi_lpss.c and notifier calls > from drivers/base/dd.c. Also I was wondering why RPM usage counts don't > increase monotonically over repeated module load/unload cycle. $subject patch changes this, as I didn't think that was really the wanted behaviour. Perhaps acpi doesn't power down the device in case the runtime PM usage count > 0. I will have look and see what I can dig out. > > I had a few paper notes about these when I last time looked and debugged PM > in i2c-designware but throw away them :-( Okay, I see. Seems like we definitely need to do some more testing on acpi enabled platforms. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 19174e7..94ff953 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -236,21 +236,21 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) adap->dev.of_node = pdev->dev.of_node; pm_runtime_get_noresume(&pdev->dev); - if (dev->pm_runtime_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_set_autosuspend_delay(&pdev->dev, 1000); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + + if (dev->pm_runtime_disabled) + pm_runtime_get_noresume(&pdev->dev); r = i2c_dw_probe(dev); if (r) { if (!IS_ERR(dev->clk)) clk_disable_unprepare(dev->clk); - if (!dev->pm_runtime_disabled) - pm_runtime_disable(&pdev->dev); + pm_runtime_disable(&pdev->dev); + if (dev->pm_runtime_disabled) + pm_runtime_put_noidle(&pdev->dev); } pm_runtime_put(&pdev->dev); @@ -267,10 +267,11 @@ static int dw_i2c_plat_remove(struct platform_device *pdev) i2c_dw_disable(dev); + if (dev->pm_runtime_disabled) + pm_runtime_put_noidle(&pdev->dev); pm_runtime_dont_use_autosuspend(&pdev->dev); pm_runtime_put_sync(&pdev->dev); - if (!dev->pm_runtime_disabled) - pm_runtime_disable(&pdev->dev); + pm_runtime_disable(&pdev->dev); return 0; }
In cases when the designware specific flag "pm_runtime_disable" is set, ->probe() calls pm_runtime_forbid() for the device, but without enabling runtime PM. This increases the runtime PM usage count for the device, but as runtime PM is disabled it's pointless. Let's instead convert to unconditionally enable runtime PM, which has the effect of making a parent device aware of its child. To also maintain the old behaviour when the "pm_runtime_disable" flag is set, which prevents userspace to enable runtime PM suspend via sysfs, switch from calling pm_runtime_forbid() into pm_runtime_get_noresume() during ->probe(). While changing this, let's also also correct the error path in ->probe() and fix ->remove(), as to decrease the runtime PM usage count when it has been in increased by pm_runtime_forbid() (after this change, by pm_runtime_get_noresume()). Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/i2c/busses/i2c-designware-platdrv.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) -- 1.9.1