| Message ID | 1353671443-2978-2-git-send-email-sachin.kamat@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester, On 25 November 2012 21:02, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> 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<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. 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
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; } }
devm_clk_get is device managed and makes error handling and cleanup a bit simpler. 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(-)