Re: [PATCH 1/6] [media] s5p-fimc: Use devm_clk_get in mipi-csis.c

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Sachin,

On 11/23/2012 12:50 PM, Sachin Kamat wrote:
> devm_clk_get is device managed and makes error handling and cleanup
> a bit simpler.

Can we postpone this once devm_clk_prepare() is available ?

> Signed-off-by: Sachin Kamat<sachin.kamat@linaro.org>
> ---
>   drivers/media/platform/s5p-fimc/mipi-csis.c |    6 +-----
>   1 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/s5p-fimc/mipi-csis.c b/drivers/media/platform/s5p-fimc/mipi-csis.c
> index 4c961b1..d624bfa 100644
> --- a/drivers/media/platform/s5p-fimc/mipi-csis.c
> +++ b/drivers/media/platform/s5p-fimc/mipi-csis.c
> @@ -341,8 +341,6 @@ static void s5pcsis_clk_put(struct csis_state *state)
>   		if (IS_ERR_OR_NULL(state->clock[i]))
>   			continue;
>   		clk_unprepare(state->clock[i]);
> -		clk_put(state->clock[i]);
> -		state->clock[i] = NULL;

This line shouldn't be removed, it protects from releasing already
released clock resource. In fact state->clock[i] = ERR_PTR(-EINVAL);
would be more correct, but that's a different story.

>   	}
>   }
>
> @@ -352,13 +350,11 @@ static int s5pcsis_clk_get(struct csis_state *state)
>   	int i, ret;
>
>   	for (i = 0; i<  NUM_CSIS_CLOCKS; i++) {
> -		state->clock[i] = clk_get(dev, csi_clock_name[i]);
> +		state->clock[i] = devm_clk_get(dev, csi_clock_name[i]);
>   		if (IS_ERR(state->clock[i]))
>   			goto err;
>   		ret = clk_prepare(state->clock[i]);
>   		if (ret<  0) {
> -			clk_put(state->clock[i]);
> -			state->clock[i] = NULL;

And same here, now we have a pointer to valid, unprepared clock in
state->clock[i]. When s5pcsis_clk_put() gets called afterwards it will
invoke unbalanced clk_unprepare() in this clock.

I would prefer to hold on with that sort of changes in s5p-fimc driver,
until after devm_clk_prepare() is available.

>   			goto err;
>   		}
>   	}

Regards,
Sylwester