Message ID | 20190417222925.5815-9-digetx@gmail.com |
---|---|
State | Deferred |
Headers | show |
Series | NVIDIA Tegra devfreq improvements and Tegra20/30 support | expand |
Hi, On 19. 4. 18. 오전 7:29, Dmitry Osipenko wrote: > Reset hardware, disable ACTMON clock, release OPP's and handle all > possible error cases correctly, maintaining the correct tear down > order. Also use devm_platform_ioremap_resource() which is now available > in the kernel. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/devfreq/tegra-devfreq.c | 83 +++++++++++++++++++-------------- > 1 file changed, 47 insertions(+), 36 deletions(-) > > diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c > index ce1eb97a2090..70946e432d3c 100644 > --- a/drivers/devfreq/tegra-devfreq.c > +++ b/drivers/devfreq/tegra-devfreq.c > @@ -610,7 +610,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev) > { > struct tegra_devfreq *tegra; > struct tegra_devfreq_device *dev; > - struct resource *res; > unsigned int i; > unsigned long rate; > int err; > @@ -619,9 +618,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev) > if (!tegra) > return -ENOMEM; > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - > - tegra->regs = devm_ioremap_resource(&pdev->dev, res); > + tegra->regs = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(tegra->regs)) > return PTR_ERR(tegra->regs); > > @@ -643,11 +640,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev) > return PTR_ERR(tegra->emc_clock); > } > > - tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb; > - err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb); > - if (err) { > - dev_err(&pdev->dev, > - "Failed to register rate change notifier\n"); > + tegra->irq = platform_get_irq(pdev, 0); > + if (tegra->irq < 0) { > + err = tegra->irq; > + dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err); > return err; > } > > @@ -678,54 +674,69 @@ static int tegra_devfreq_probe(struct platform_device *pdev) > > for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) { > rate = clk_round_rate(tegra->emc_clock, rate); > - dev_pm_opp_add(&pdev->dev, rate, 0); > - } > > - tegra->irq = platform_get_irq(pdev, 0); > - if (tegra->irq < 0) { > - err = tegra->irq; > - dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err); > - return err; > + err = dev_pm_opp_add(&pdev->dev, rate, 0); > + if (err) { > + dev_err(&pdev->dev, "Failed to add OPP: %d\n", err); > + goto remove_opps; > + } > } > > platform_set_drvdata(pdev, tegra); > > + tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb; > + err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb); > + if (err) { > + dev_err(&pdev->dev, > + "Failed to register rate change notifier\n"); > + goto remove_opps; > + } > + > + tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock); > + tegra->devfreq = devfreq_add_device(&pdev->dev, > + &tegra_devfreq_profile, > + "tegra_actmon", > + NULL); > + if (IS_ERR(tegra->devfreq)) { > + err = PTR_ERR(tegra->devfreq); > + goto unreg_notifier; > + } > + > err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL, > actmon_thread_isr, IRQF_ONESHOT, > "tegra-devfreq", tegra); > if (err) { > - dev_err(&pdev->dev, "Interrupt request failed\n"); > - return err; > + dev_err(&pdev->dev, "Interrupt request failed: %d\n", err); > + goto remove_devfreq; > } > > - tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock); > - tegra->devfreq = devm_devfreq_add_device(&pdev->dev, > - &tegra_devfreq_profile, > - "tegra_actmon", > - NULL); > - > return 0; > + > +remove_devfreq: > + devfreq_remove_device(tegra->devfreq); > + > +unreg_notifier: > + clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb); > + > +remove_opps: > + dev_pm_opp_remove_all_dynamic(&pdev->dev); > + > + reset_control_reset(tegra->reset); > + clk_disable_unprepare(tegra->clock); > + > + return err; > } > > static int tegra_devfreq_remove(struct platform_device *pdev) > { > struct tegra_devfreq *tegra = platform_get_drvdata(pdev); > - int irq = platform_get_irq(pdev, 0); > - u32 val; > - unsigned int i; > - > - for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) { > - val = device_readl(&tegra->devices[i], ACTMON_DEV_CTRL); > - val &= ~ACTMON_DEV_CTRL_ENB; > - device_writel(&tegra->devices[i], val, ACTMON_DEV_CTRL); > - } > - > - actmon_write_barrier(tegra); > > - devm_free_irq(&pdev->dev, irq, tegra); > + devfreq_remove_device(tegra->devfreq); > + dev_pm_opp_remove_all_dynamic(&pdev->dev); > > clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb); nitpick: the probe function has following call sequence if error case, First, clk_notifier_unregister() Second, dev_pm_opp_remove_all_dynamic() If possible, you better to keep the same sequence in the tegra_devfreq_remove(). But, it is just opinion. If you think that it doesn't break the routine of device removal, jut keep this code. > > + reset_control_reset(tegra->reset); > clk_disable_unprepare(tegra->clock); > > return 0; > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c index ce1eb97a2090..70946e432d3c 100644 --- a/drivers/devfreq/tegra-devfreq.c +++ b/drivers/devfreq/tegra-devfreq.c @@ -610,7 +610,6 @@ static int tegra_devfreq_probe(struct platform_device *pdev) { struct tegra_devfreq *tegra; struct tegra_devfreq_device *dev; - struct resource *res; unsigned int i; unsigned long rate; int err; @@ -619,9 +618,7 @@ static int tegra_devfreq_probe(struct platform_device *pdev) if (!tegra) return -ENOMEM; - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - - tegra->regs = devm_ioremap_resource(&pdev->dev, res); + tegra->regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(tegra->regs)) return PTR_ERR(tegra->regs); @@ -643,11 +640,10 @@ static int tegra_devfreq_probe(struct platform_device *pdev) return PTR_ERR(tegra->emc_clock); } - tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb; - err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb); - if (err) { - dev_err(&pdev->dev, - "Failed to register rate change notifier\n"); + tegra->irq = platform_get_irq(pdev, 0); + if (tegra->irq < 0) { + err = tegra->irq; + dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err); return err; } @@ -678,54 +674,69 @@ static int tegra_devfreq_probe(struct platform_device *pdev) for (rate = 0; rate <= tegra->max_freq * KHZ; rate++) { rate = clk_round_rate(tegra->emc_clock, rate); - dev_pm_opp_add(&pdev->dev, rate, 0); - } - tegra->irq = platform_get_irq(pdev, 0); - if (tegra->irq < 0) { - err = tegra->irq; - dev_err(&pdev->dev, "Failed to get IRQ: %d\n", err); - return err; + err = dev_pm_opp_add(&pdev->dev, rate, 0); + if (err) { + dev_err(&pdev->dev, "Failed to add OPP: %d\n", err); + goto remove_opps; + } } platform_set_drvdata(pdev, tegra); + tegra->rate_change_nb.notifier_call = tegra_actmon_rate_notify_cb; + err = clk_notifier_register(tegra->emc_clock, &tegra->rate_change_nb); + if (err) { + dev_err(&pdev->dev, + "Failed to register rate change notifier\n"); + goto remove_opps; + } + + tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock); + tegra->devfreq = devfreq_add_device(&pdev->dev, + &tegra_devfreq_profile, + "tegra_actmon", + NULL); + if (IS_ERR(tegra->devfreq)) { + err = PTR_ERR(tegra->devfreq); + goto unreg_notifier; + } + err = devm_request_threaded_irq(&pdev->dev, tegra->irq, NULL, actmon_thread_isr, IRQF_ONESHOT, "tegra-devfreq", tegra); if (err) { - dev_err(&pdev->dev, "Interrupt request failed\n"); - return err; + dev_err(&pdev->dev, "Interrupt request failed: %d\n", err); + goto remove_devfreq; } - tegra_devfreq_profile.initial_freq = clk_get_rate(tegra->emc_clock); - tegra->devfreq = devm_devfreq_add_device(&pdev->dev, - &tegra_devfreq_profile, - "tegra_actmon", - NULL); - return 0; + +remove_devfreq: + devfreq_remove_device(tegra->devfreq); + +unreg_notifier: + clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb); + +remove_opps: + dev_pm_opp_remove_all_dynamic(&pdev->dev); + + reset_control_reset(tegra->reset); + clk_disable_unprepare(tegra->clock); + + return err; } static int tegra_devfreq_remove(struct platform_device *pdev) { struct tegra_devfreq *tegra = platform_get_drvdata(pdev); - int irq = platform_get_irq(pdev, 0); - u32 val; - unsigned int i; - - for (i = 0; i < ARRAY_SIZE(actmon_device_configs); i++) { - val = device_readl(&tegra->devices[i], ACTMON_DEV_CTRL); - val &= ~ACTMON_DEV_CTRL_ENB; - device_writel(&tegra->devices[i], val, ACTMON_DEV_CTRL); - } - - actmon_write_barrier(tegra); - devm_free_irq(&pdev->dev, irq, tegra); + devfreq_remove_device(tegra->devfreq); + dev_pm_opp_remove_all_dynamic(&pdev->dev); clk_notifier_unregister(tegra->emc_clock, &tegra->rate_change_nb); + reset_control_reset(tegra->reset); clk_disable_unprepare(tegra->clock); return 0;
Reset hardware, disable ACTMON clock, release OPP's and handle all possible error cases correctly, maintaining the correct tear down order. Also use devm_platform_ioremap_resource() which is now available in the kernel. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/devfreq/tegra-devfreq.c | 83 +++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 36 deletions(-)