* [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints
2012-11-23 4:44 [PATCH 0/4] [media] exynos-gsc: Some fixes Sachin Kamat
@ 2012-11-23 4:44 ` Sachin Kamat
2012-11-23 9:31 ` Sylwester Nawrocki
2012-11-23 4:45 ` [PATCH 2/4] [media] exynos-gsc: Remove gsc_clk_put call from gsc_clk_get Sachin Kamat
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Sachin Kamat @ 2012-11-23 4:44 UTC (permalink / raw)
To: linux-media; +Cc: s.nawrocki, sachin.kamat, patches, Shaik Ameer Basha
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints
2012-11-23 4:44 ` [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints Sachin Kamat
@ 2012-11-23 9:31 ` Sylwester Nawrocki
2012-11-23 9:47 ` Sachin Kamat
0 siblings, 1 reply; 9+ messages in thread
From: Sylwester Nawrocki @ 2012-11-23 9:31 UTC (permalink / raw)
To: Sachin Kamat; +Cc: linux-media, patches, Shaik Ameer Basha
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints
2012-11-23 9:31 ` Sylwester Nawrocki
@ 2012-11-23 9:47 ` Sachin Kamat
2012-11-23 9:58 ` Sylwester Nawrocki
0 siblings, 1 reply; 9+ messages in thread
From: Sachin Kamat @ 2012-11-23 9:47 UTC (permalink / raw)
To: Sylwester Nawrocki; +Cc: linux-media, patches, Shaik Ameer Basha
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints
2012-11-23 9:47 ` Sachin Kamat
@ 2012-11-23 9:58 ` Sylwester Nawrocki
0 siblings, 0 replies; 9+ messages in thread
From: Sylwester Nawrocki @ 2012-11-23 9:58 UTC (permalink / raw)
To: Sachin Kamat; +Cc: linux-media, patches, Shaik Ameer Basha
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] [media] exynos-gsc: Remove gsc_clk_put call from gsc_clk_get
2012-11-23 4:44 [PATCH 0/4] [media] exynos-gsc: Some fixes Sachin Kamat
2012-11-23 4:44 ` [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints Sachin Kamat
@ 2012-11-23 4:45 ` Sachin Kamat
2012-11-23 9:51 ` Sylwester Nawrocki
2012-11-23 4:45 ` [PATCH 3/4] [media] exynos-gsc: Use devm_clk_get() Sachin Kamat
2012-11-23 4:45 ` [PATCH 4/4] [media] exynos-gsc: Fix checkpatch warning in gsc-m2m.c Sachin Kamat
3 siblings, 1 reply; 9+ messages in thread
From: Sachin Kamat @ 2012-11-23 4:45 UTC (permalink / raw)
To: linux-media; +Cc: s.nawrocki, sachin.kamat, patches, Shaik Ameer Basha
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/4] [media] exynos-gsc: Remove gsc_clk_put call from gsc_clk_get
2012-11-23 4:45 ` [PATCH 2/4] [media] exynos-gsc: Remove gsc_clk_put call from gsc_clk_get Sachin Kamat
@ 2012-11-23 9:51 ` Sylwester Nawrocki
0 siblings, 0 replies; 9+ messages in thread
From: Sylwester Nawrocki @ 2012-11-23 9:51 UTC (permalink / raw)
To: Sachin Kamat; +Cc: linux-media, s.nawrocki, patches, Shaik Ameer Basha
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;
> }
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] [media] exynos-gsc: Use devm_clk_get()
2012-11-23 4:44 [PATCH 0/4] [media] exynos-gsc: Some fixes Sachin Kamat
2012-11-23 4:44 ` [PATCH 1/4] [media] exynos-gsc: Rearrange error messages for valid prints Sachin Kamat
2012-11-23 4:45 ` [PATCH 2/4] [media] exynos-gsc: Remove gsc_clk_put call from gsc_clk_get Sachin Kamat
@ 2012-11-23 4:45 ` Sachin Kamat
2012-11-23 4:45 ` [PATCH 4/4] [media] exynos-gsc: Fix checkpatch warning in gsc-m2m.c Sachin Kamat
3 siblings, 0 replies; 9+ messages in thread
From: Sachin Kamat @ 2012-11-23 4:45 UTC (permalink / raw)
To: linux-media; +Cc: s.nawrocki, sachin.kamat, patches, Shaik Ameer Basha
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/4] [media] exynos-gsc: Fix checkpatch warning in gsc-m2m.c
2012-11-23 4:44 [PATCH 0/4] [media] exynos-gsc: Some fixes Sachin Kamat
` (2 preceding siblings ...)
2012-11-23 4:45 ` [PATCH 3/4] [media] exynos-gsc: Use devm_clk_get() Sachin Kamat
@ 2012-11-23 4:45 ` Sachin Kamat
3 siblings, 0 replies; 9+ messages in thread
From: Sachin Kamat @ 2012-11-23 4:45 UTC (permalink / raw)
To: linux-media; +Cc: s.nawrocki, sachin.kamat, patches, Shaik Ameer Basha
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
^ permalink raw reply related [flat|nested] 9+ messages in thread