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

2012-11-25 Thread Sachin Kamat 
Hi Sylwester,

On 25 November 2012 21:02, Sylwester Nawrocki
 wrote:
> 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 ?
Ok. No problem. I will hold on till then.

>
>
>> Signed-off-by: Sachin Kamat
>> ---
>>   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.

Wouldn't 'state->clock[i] = NULL' cause a problem when put gets called
upon exit from this function by devres f/w as its argument would be
NULL?
In that case devm_clk_put would be better here?

> 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.

Ok.

>
>> goto err;
>> }
>> }
>
>
> Regards,
> Sylwester



-- 
With warm regards,
Sachin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-11-25 Thread Sylwester Nawrocki 

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
---
  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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-11-23 Thread Sachin Kamat 
devm_clk_get is device managed and makes error handling and cleanup
a bit simpler.

Signed-off-by: Sachin Kamat 
---
 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;
}
 }
 
@@ -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;
goto err;
}
}
-- 
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html