Re: [PATCH v14 20/39] pwm: tegra: Add runtime PM and OPP support

On Fri, Oct 29, 2021 at 5:20 PM Dmitry Osipenko digetx@gmail.com wrote:

26.10.2021 01:40, Dmitry Osipenko пишет:

...

• ret = devm_pm_runtime_enable(&pdev->dev);
  

• if (ret)
  

•         return ret;
  

• ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
  

• if (ret)
  

•         return ret;
  

• ret = pm_runtime_resume_and_get(&pdev->dev);
  

• if (ret)
  

•         return ret;
  

• /* Set maximum frequency of the IP */
  

• ret = clk_set_rate(pwm->clk, pwm->soc->max_frequency);
  

• ret = dev_pm_opp_set_rate(pwm->dev, pwm->soc->max_frequency);
  if (ret < 0) {
          dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
  

•         return ret;
  

•         goto put_pm;
  }
  
  /*
  

@@ -278,7 +294,7 @@ static int tegra_pwm_probe(struct platform_device *pdev) if (IS_ERR(pwm->rst)) { ret = PTR_ERR(pwm->rst); dev_err(&pdev->dev, "Reset control is not found: %d\n", ret);

•         return ret;
  

•         goto put_pm;
  }
  
  reset_control_deassert(pwm->rst);
  

@@ -291,10 +307,15 @@ static int tegra_pwm_probe(struct platform_device *pdev) if (ret < 0) { dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); reset_control_assert(pwm->rst);

•         return ret;
  

•         goto put_pm;
  }
  
  

• pm_runtime_put(&pdev->dev);
  

• return 0;
  

+put_pm:

• pm_runtime_put_sync_suspend(&pdev->dev);
  

• return ret;
  

}

static int tegra_pwm_remove(struct platform_device *pdev) @@ -305,20 +326,44 @@ static int tegra_pwm_remove(struct platform_device *pdev)

  reset_control_assert(pc->rst);

• pm_runtime_force_suspend(&pdev->dev);
  

I just noticed that RPM core doesn't reset RPM-enable count of a device on driver's unbind (pm_runtime_reinit). It was a bad idea to use devm_pm_runtime_enable() + pm_runtime_force_suspend() here, since RPM is disabled twice on driver's removal, and thus, RPM will never be enabled again.

I'll fix it for PWM and other drivers in this series, in v15.

Well, for the record, IMV using pm_runtime_force_suspend() is generally a questionable idea.