media: exynos4-is: add missed clk_disable_unprepare in remove
diff mbox series

Message ID 20191104154942.8935-1-hslester96@gmail.com
State New
Headers show
Series
  • media: exynos4-is: add missed clk_disable_unprepare in remove
Related show

Commit Message

Chuhong Yuan Nov. 4, 2019, 3:49 p.m. UTC
This driver forgets to disable and unprepare clock when remove.
Add a call to clk_disable_unprepare to fix it.

Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
---
 drivers/media/platform/exynos4-is/fimc-lite.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Hans Verkuil Dec. 12, 2019, 10:39 a.m. UTC | #1
On 11/4/19 4:49 PM, Chuhong Yuan wrote:
> This driver forgets to disable and unprepare clock when remove.
> Add a call to clk_disable_unprepare to fix it.

I'd like an Ack from Samsung before I apply this. I see this in the probe() in fimc-lite.c:

       if (!pm_runtime_enabled(dev)) {
                ret = clk_prepare_enable(fimc->clock);
                if (ret < 0)
                        goto err_sd;
        }

So is it right to always call clk_disable_unprepare in the remove()?

I suspect it is correct, but I would like someone else to take a look as well.

Regards,

	Hans

> 
> Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> ---
>  drivers/media/platform/exynos4-is/fimc-lite.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
> index e87c6a09205b..6748bd96aada 100644
> --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> @@ -1614,6 +1614,7 @@ static int fimc_lite_remove(struct platform_device *pdev)
>  	struct fimc_lite *fimc = platform_get_drvdata(pdev);
>  	struct device *dev = &pdev->dev;
>  
> +	clk_disable_unprepare(fimc->clock);
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_suspended(dev);
>  	fimc_lite_unregister_capture_subdev(fimc);
>
Krzysztof Kozlowski Dec. 12, 2019, 11:03 a.m. UTC | #2
On Thu, 12 Dec 2019 at 11:39, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/4/19 4:49 PM, Chuhong Yuan wrote:
> > This driver forgets to disable and unprepare clock when remove.
> > Add a call to clk_disable_unprepare to fix it.
>
> I'd like an Ack from Samsung before I apply this. I see this in the probe() in fimc-lite.c:
>
>        if (!pm_runtime_enabled(dev)) {
>                 ret = clk_prepare_enable(fimc->clock);
>                 if (ret < 0)
>                         goto err_sd;
>         }
>
> So is it right to always call clk_disable_unprepare in the remove()?
>
> I suspect it is correct, but I would like someone else to take a look as well.
>
> Regards,
>
>         Hans
>
> >
> > Signed-off-by: Chuhong Yuan <hslester96@gmail.com>
> > ---
> >  drivers/media/platform/exynos4-is/fimc-lite.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
> > index e87c6a09205b..6748bd96aada 100644
> > --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> > +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> > @@ -1614,6 +1614,7 @@ static int fimc_lite_remove(struct platform_device *pdev)
> >       struct fimc_lite *fimc = platform_get_drvdata(pdev);
> >       struct device *dev = &pdev->dev;
> >
> > +     clk_disable_unprepare(fimc->clock);
> >       pm_runtime_disable(dev);
> >       pm_runtime_set_suspended(dev);
> >       fimc_lite_unregister_capture_subdev(fimc);

No, it is wrong. The clock is enabled in probe only if
!pm_runtime_enabled(). This will matter only if PM is disabled but now
it leads to unbalanced disables. This was clearly not tested because I
believe any test would trigger error. In such case, please mark the
patches as RFT.

There is some tendency to post small "fixes" like this without
testing... ok, not everyone has hardware but then just mark it as
not-tested or RFT...

Best regards,
Krzysztof

Patch
diff mbox series

diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
index e87c6a09205b..6748bd96aada 100644
--- a/drivers/media/platform/exynos4-is/fimc-lite.c
+++ b/drivers/media/platform/exynos4-is/fimc-lite.c
@@ -1614,6 +1614,7 @@  static int fimc_lite_remove(struct platform_device *pdev)
 	struct fimc_lite *fimc = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
 
+	clk_disable_unprepare(fimc->clock);
 	pm_runtime_disable(dev);
 	pm_runtime_set_suspended(dev);
 	fimc_lite_unregister_capture_subdev(fimc);