[PATCH 0/4] [media] exynos-gsc: Some fixes

[PATCH 0/4] [media] exynos-gsc: Some fixes

[Sachin Kamat]

Patch series build tested and based on samsung/for_v3.8 branch of
git://linuxtv.org/snawrocki/media.git.

Sachin Kamat (4):
  [media] exynos-gsc: Rearrange error messages for valid prints
  [media] exynos-gsc: Remove gsc_clk_put call from gsc_clk_get
  [media] exynos-gsc: Use devm_clk_get()
  [media] exynos-gsc: Fix checkpatch warning in gsc-m2m.c

 drivers/media/platform/exynos-gsc/gsc-core.c |   21 ++++++++-------------
 drivers/media/platform/exynos-gsc/gsc-m2m.c  |    2 +-
 2 files changed, 9 insertions(+), 14 deletions(-)

-- 
1.7.4.1

[PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints

[Sachin Kamat]

In case of clk_prepare failure, the function gsc_clk_get also prints
"failed to get clock" which is not correct. Hence move the error
messages to their respective blocks. While at it, also renamed the labels
meaningfully.

Cc: Shaik Ameer Basha <shaik.ameer@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/platform/exynos-gsc/gsc-core.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 6d6f65d..45bcfa7 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1017,25 +1017,26 @@ static int gsc_clk_get(struct gsc_dev *gsc)
 	dev_dbg(&gsc->pdev->dev, "gsc_clk_get Called\n");
 
 	gsc->clock = clk_get(&gsc->pdev->dev, GSC_CLOCK_GATE_NAME);
-	if (IS_ERR(gsc->clock))
-		goto err_print;
+	if (IS_ERR(gsc->clock)) {
+		dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
+			GSC_CLOCK_GATE_NAME);
+		goto err_clk_get;
+	}
 
 	ret = clk_prepare(gsc->clock);
 	if (ret < 0) {
+		dev_err(&gsc->pdev->dev, "clock prepare failed for clock: %s\n",
+			GSC_CLOCK_GATE_NAME);
 		clk_put(gsc->clock);
 		gsc->clock = NULL;
-		goto err;
+		goto err_clk_prepare;
 	}
 
 	return 0;
 
-err:
-	dev_err(&gsc->pdev->dev, "clock prepare failed for clock: %s\n",
-					GSC_CLOCK_GATE_NAME);
+err_clk_prepare:
 	gsc_clk_put(gsc);
-err_print:
-	dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
-					GSC_CLOCK_GATE_NAME);
+err_clk_get:
 	return -ENXIO;
 }
 
-- 
1.7.4.1

[PATCH 2/4] [media] exynos-gsc: Remove gsc_clk_put call from gsc_clk_get

[Sachin Kamat]

Since this function just returns (since gsc->clock is NULL),
remove it and make the exit code simpler.

Cc: Shaik Ameer Basha <shaik.ameer@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/platform/exynos-gsc/gsc-core.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 45bcfa7..99ee1a9 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1020,7 +1020,7 @@ static int gsc_clk_get(struct gsc_dev *gsc)
 	if (IS_ERR(gsc->clock)) {
 		dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
 			GSC_CLOCK_GATE_NAME);
-		goto err_clk_get;
+		goto err;
 	}
 
 	ret = clk_prepare(gsc->clock);
@@ -1029,14 +1029,12 @@ static int gsc_clk_get(struct gsc_dev *gsc)
 			GSC_CLOCK_GATE_NAME);
 		clk_put(gsc->clock);
 		gsc->clock = NULL;
-		goto err_clk_prepare;
+		goto err;
 	}
 
 	return 0;
 
-err_clk_prepare:
-	gsc_clk_put(gsc);
-err_clk_get:
+err:
 	return -ENXIO;
 }
 
-- 
1.7.4.1

[PATCH 3/4] [media] exynos-gsc: Use devm_clk_get()

[Sachin Kamat]

devm_clk_get() is a device managed function and makes error handling
a bit simpler.

Cc: Shaik Ameer Basha <shaik.ameer@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/platform/exynos-gsc/gsc-core.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 99ee1a9..b89afec 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1006,8 +1006,6 @@ static void gsc_clk_put(struct gsc_dev *gsc)
 		return;
 
 	clk_unprepare(gsc->clock);
-	clk_put(gsc->clock);
-	gsc->clock = NULL;
 }
 
 static int gsc_clk_get(struct gsc_dev *gsc)
@@ -1016,7 +1014,7 @@ static int gsc_clk_get(struct gsc_dev *gsc)
 
 	dev_dbg(&gsc->pdev->dev, "gsc_clk_get Called\n");
 
-	gsc->clock = clk_get(&gsc->pdev->dev, GSC_CLOCK_GATE_NAME);
+	gsc->clock = devm_clk_get(&gsc->pdev->dev, GSC_CLOCK_GATE_NAME);
 	if (IS_ERR(gsc->clock)) {
 		dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
 			GSC_CLOCK_GATE_NAME);
@@ -1027,8 +1025,6 @@ static int gsc_clk_get(struct gsc_dev *gsc)
 	if (ret < 0) {
 		dev_err(&gsc->pdev->dev, "clock prepare failed for clock: %s\n",
 			GSC_CLOCK_GATE_NAME);
-		clk_put(gsc->clock);
-		gsc->clock = NULL;
 		goto err;
 	}
 
-- 
1.7.4.1

[PATCH 4/4] [media] exynos-gsc: Fix checkpatch warning in gsc-m2m.c

[Sachin Kamat]

Fixes the following warning:
WARNING: space prohibited between function name and open parenthesis '('
FILE: media/platform/exynos-gsc/gsc-m2m.c:606:
	ctx = kzalloc(sizeof (*ctx), GFP_KERNEL);

Cc: Shaik Ameer Basha <shaik.ameer@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/media/platform/exynos-gsc/gsc-m2m.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 39dff20..10036d6 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -603,7 +603,7 @@ static int gsc_m2m_open(struct file *file)
 	if (mutex_lock_interruptible(&gsc->lock))
 		return -ERESTARTSYS;
 
-	ctx = kzalloc(sizeof (*ctx), GFP_KERNEL);
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx) {
 		ret = -ENOMEM;
 		goto unlock;
-- 
1.7.4.1

Re: [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints

[Sylwester Nawrocki]

Hi Sachin,

Thanks for the patches.

On 11/23/2012 05:44 AM, Sachin Kamat wrote:
> In case of clk_prepare failure, the function gsc_clk_get also prints
> "failed to get clock" which is not correct. Hence move the error
> messages to their respective blocks. While at it, also renamed the labels
> meaningfully.
> 
> Cc: Shaik Ameer Basha <shaik.ameer@samsung.com>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/media/platform/exynos-gsc/gsc-core.c |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 6d6f65d..45bcfa7 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1017,25 +1017,26 @@ static int gsc_clk_get(struct gsc_dev *gsc)
>  	dev_dbg(&gsc->pdev->dev, "gsc_clk_get Called\n");
>  
>  	gsc->clock = clk_get(&gsc->pdev->dev, GSC_CLOCK_GATE_NAME);
> -	if (IS_ERR(gsc->clock))
> -		goto err_print;
> +	if (IS_ERR(gsc->clock)) {
> +		dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
> +			GSC_CLOCK_GATE_NAME);
> +		goto err_clk_get;

You could also just return PTR_ERR(gsc->clock) here and remove
err_clk_get label entirely.		

> +	}
>  
>  	ret = clk_prepare(gsc->clock);
>  	if (ret < 0) {
> +		dev_err(&gsc->pdev->dev, "clock prepare failed for clock: %s\n",
> +			GSC_CLOCK_GATE_NAME);
>  		clk_put(gsc->clock);
>  		gsc->clock = NULL;
> -		goto err;
> +		goto err_clk_prepare;
>  	}
>  
>  	return 0;
>  
> -err:
> -	dev_err(&gsc->pdev->dev, "clock prepare failed for clock: %s\n",
> -					GSC_CLOCK_GATE_NAME);
> +err_clk_prepare:
>  	gsc_clk_put(gsc);

This call can be removed too. I would remove all labels and gotos in
this function. Since there is only one clock, you need to only call
clk_put when clk_prepare() fails, there is no need for gsc_clk_put().

> -err_print:
> -	dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
> -					GSC_CLOCK_GATE_NAME);
> +err_clk_get:
>  	return -ENXIO;
>  }

As a general remark, I think changes like in this series have to be
validated before we can think of applying it. I guess Shaik or
somebody else would need to test it. I still have no board I could
test Exynos5 Gscaler IP.

--

Regards,
Sylwester

Re: [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints

[Sachin Kamat]

Hi Sylwester,

Thanks for the review.

On 23 November 2012 15:01, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote:
> Hi Sachin,
>
> Thanks for the patches.
>
> On 11/23/2012 05:44 AM, Sachin Kamat wrote:
>> In case of clk_prepare failure, the function gsc_clk_get also prints
>> "failed to get clock" which is not correct. Hence move the error
>> messages to their respective blocks. While at it, also renamed the labels
>> meaningfully.
>>
>> Cc: Shaik Ameer Basha <shaik.ameer@samsung.com>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>>  drivers/media/platform/exynos-gsc/gsc-core.c |   19 ++++++++++---------
>>  1 files changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
>> index 6d6f65d..45bcfa7 100644
>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>> @@ -1017,25 +1017,26 @@ static int gsc_clk_get(struct gsc_dev *gsc)
>>       dev_dbg(&gsc->pdev->dev, "gsc_clk_get Called\n");
>>
>>       gsc->clock = clk_get(&gsc->pdev->dev, GSC_CLOCK_GATE_NAME);
>> -     if (IS_ERR(gsc->clock))
>> -             goto err_print;
>> +     if (IS_ERR(gsc->clock)) {
>> +             dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
>> +                     GSC_CLOCK_GATE_NAME);
>> +             goto err_clk_get;
>
> You could also just return PTR_ERR(gsc->clock) here and remove
> err_clk_get label entirely.

OK.

>
>> +     }
>>
>>       ret = clk_prepare(gsc->clock);
>>       if (ret < 0) {
>> +             dev_err(&gsc->pdev->dev, "clock prepare failed for clock: %s\n",
>> +                     GSC_CLOCK_GATE_NAME);
>>               clk_put(gsc->clock);
>>               gsc->clock = NULL;
>> -             goto err;
>> +             goto err_clk_prepare;
>>       }
>>
>>       return 0;
>>
>> -err:
>> -     dev_err(&gsc->pdev->dev, "clock prepare failed for clock: %s\n",
>> -                                     GSC_CLOCK_GATE_NAME);
>> +err_clk_prepare:
>>       gsc_clk_put(gsc);
>
> This call can be removed too. I would remove all labels and gotos in
> this function. Since there is only one clock, you need to only call
> clk_put when clk_prepare() fails, there is no need for gsc_clk_put().

I have removed gsc_clk_put() in the subsequent patch in this series.
I will probably incorporate your previous comment and remove the label
altogether (in patch 3)
and send it again.

>
>> -err_print:
>> -     dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
>> -                                     GSC_CLOCK_GATE_NAME);
>> +err_clk_get:
>>       return -ENXIO;
>>  }
>
> As a general remark, I think changes like in this series have to be
> validated before we can think of applying it. I guess Shaik or
> somebody else would need to test it. I still have no board I could
> test Exynos5 Gscaler IP.

Yes you are right. I have already talked to Shaik about it.
He has agreed to test the same.

>
> --
>
> Regards,
> Sylwester



-- 
With warm regards,
Sachin

Re: [PATCH 2/4] [media] exynos-gsc: Remove gsc_clk_put call from gsc_clk_get

[Sylwester Nawrocki]

On 11/23/2012 05:45 AM, Sachin Kamat wrote:
> Since this function just returns (since gsc->clock is NULL),
> remove it and make the exit code simpler.
> 
> Cc: Shaik Ameer Basha <shaik.ameer@samsung.com>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/media/platform/exynos-gsc/gsc-core.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 45bcfa7..99ee1a9 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1020,7 +1020,7 @@ static int gsc_clk_get(struct gsc_dev *gsc)
>  	if (IS_ERR(gsc->clock)) {
>  		dev_err(&gsc->pdev->dev, "failed to get clock~~~: %s\n",
>  			GSC_CLOCK_GATE_NAME);
> -		goto err_clk_get;
> +		goto err;
>  	}
>  
>  	ret = clk_prepare(gsc->clock);
> @@ -1029,14 +1029,12 @@ static int gsc_clk_get(struct gsc_dev *gsc)
>  			GSC_CLOCK_GATE_NAME);
>  		clk_put(gsc->clock);
>  		gsc->clock = NULL;
> -		goto err_clk_prepare;
> +		goto err;
>  	}
>  
>  	return 0;
>  
> -err_clk_prepare:
> -	gsc_clk_put(gsc);
> -err_clk_get:

Hmm, ok, here come the remaining part of the cleanup..
I think this patch can be folded into the previous one.

> +err:
>  	return -ENXIO;
>  }

Re: [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints

[Sylwester Nawrocki]

On 11/23/2012 10:47 AM, Sachin Kamat wrote:
>>> +err_clk_prepare:
>>>       gsc_clk_put(gsc);
>>
>> This call can be removed too. I would remove all labels and gotos in
>> this function. Since there is only one clock, you need to only call
>> clk_put when clk_prepare() fails, there is no need for gsc_clk_put().
> 
> I have removed gsc_clk_put() in the subsequent patch in this series.
> I will probably incorporate your previous comment and remove the label
> altogether (in patch 3) and send it again.

OK, sounds good to me that way too.