From: Chanwoo Choi <cw00.choi@samsung.com>
To: Dmitry Osipenko <digetx@gmail.com>,
Thierry Reding <thierry.reding@gmail.com>,
Jonathan Hunter <jonathanh@nvidia.com>,
MyungJoo Ham <myungjoo.ham@samsung.com>,
Kyungmin Park <kyungmin.park@samsung.com>,
Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-pm@vger.kernel.org
Subject: Re: [PATCH v3 08/16] PM / devfreq: tegra: Clean up driver's probe / remove
Date: Tue, 30 Apr 2019 09:16:03 +0900 [thread overview]
Message-ID: <236a553b-ccb5-f8fd-d404-af78f04d6985@samsung.com> (raw)
In-Reply-To: <20190417222925.5815-9-digetx@gmail.com>
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>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
next prev parent reply other threads:[~2019-04-30 0:15 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-17 22:29 [PATCH v3 00/16] NVIDIA Tegra devfreq improvements and Tegra20/30 support Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 01/16] PM / devfreq: tegra: Fix kHz to Hz conversion Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 02/16] PM / devfreq: tegra: Replace readl-writel with relaxed versions Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 03/16] PM / devfreq: tegra: Replace write memory barrier with the read barrier Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 04/16] PM / devfreq: tegra: Don't ignore clk errors Dmitry Osipenko
2019-04-18 0:23 ` Chanwoo Choi
2019-04-17 22:29 ` [PATCH v3 05/16] PM / devfreq: tegra: Don't set EMC clock rate to maximum on probe Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 06/16] PM / devfreq: tegra: Drop primary interrupt handler Dmitry Osipenko
2019-04-18 0:24 ` Chanwoo Choi
2019-04-17 22:29 ` [PATCH v3 07/16] PM / devfreq: tegra: Properly disable interrupts Dmitry Osipenko
2019-04-30 0:05 ` Chanwoo Choi
2019-04-17 22:29 ` [PATCH v3 08/16] PM / devfreq: tegra: Clean up driver's probe / remove Dmitry Osipenko
2019-04-30 0:16 ` Chanwoo Choi [this message]
2019-04-17 22:29 ` [PATCH v3 09/16] PM / devfreq: tegra: Avoid inconsistency of current frequency value Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 10/16] PM / devfreq: tegra: Mark ACTMON's governor as immutable Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 11/16] PM / devfreq: tegra: Move governor registration to driver's probe Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 12/16] PM / devfreq: tegra: Reconfigure hardware on governor's restart Dmitry Osipenko
2019-04-30 1:19 ` Chanwoo Choi
2019-04-17 22:29 ` [PATCH v3 13/16] PM / devfreq: tegra: Support Tegra30 Dmitry Osipenko
2019-04-18 0:21 ` Chanwoo Choi
2019-04-17 22:29 ` [PATCH v3 14/16] PM / devfreq: tegra: Enable COMPILE_TEST for the driver Dmitry Osipenko
2019-04-17 22:29 ` [PATCH v3 15/16] PM / devfreq: tegra: Rename tegra-devfreq.c to tegra30-devfreq.c Dmitry Osipenko
2019-04-18 0:22 ` Chanwoo Choi
2019-04-17 22:29 ` [PATCH v3 16/16] PM / devfreq: Introduce driver for NVIDIA Tegra20 Dmitry Osipenko
2019-04-30 0:21 ` Chanwoo Choi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=236a553b-ccb5-f8fd-d404-af78f04d6985@samsung.com \
--to=cw00.choi@samsung.com \
--cc=digetx@gmail.com \
--cc=jonathanh@nvidia.com \
--cc=kyungmin.park@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=myungjoo.ham@samsung.com \
--cc=thierry.reding@gmail.com \
--cc=tomeu.vizoso@collabora.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).