Re: [RFT][PATCH 1/2] i2c: designware: Clean up PM handling in dw_i2c_plat_probe()

Jarkko Nikula
2017-09-05T14:40:28Z

Hi

On 09/04/2017 02:08 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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 <rafael.j.wysocki@intel.com>
> ---
> 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