29.10.2021 18:56, Rafael J. Wysocki пишет:
> On Fri, Oct 29, 2021 at 5:20 PM Dmitry Osipenko <digetx@xxxxxxxxx> 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.
>
Please clarify why it's a questionable idea.
