| Message ID | 1365411346-30611-1-git-send-email-archit@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2013-04-08 11:55, Archit Taneja wrote: > Use devm_clk_get() instead of clk_get() for dss, and for outputs hdmi and venc. > This reduces reduces code and simplifies error handling. > > Signed-off-by: Archit Taneja <archit@ti.com> > --- > drivers/video/omap2/dss/dss.c | 18 +++--------------- > drivers/video/omap2/dss/hdmi.c | 16 ++-------------- > drivers/video/omap2/dss/venc.c | 10 +--------- > 3 files changed, 6 insertions(+), 38 deletions(-) > > diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c > index 054c2a2..645b3bc 100644 > --- a/drivers/video/omap2/dss/dss.c > +++ b/drivers/video/omap2/dss/dss.c > @@ -767,13 +767,11 @@ int dss_dpi_select_source(enum omap_channel channel) > static int dss_get_clocks(void) > { > struct clk *clk; > - int r; > > - clk = clk_get(&dss.pdev->dev, "fck"); > + clk = devm_clk_get(&dss.pdev->dev, "fck"); > if (IS_ERR(clk)) { > DSSERR("can't get clock fck\n"); > - r = PTR_ERR(clk); > - goto err; > + return PTR_ERR(clk); > } > > dss.dss_clk = clk; > @@ -782,8 +780,7 @@ static int dss_get_clocks(void) > clk = clk_get(NULL, dss.feat->clk_name); > if (IS_ERR(clk)) { > DSSERR("Failed to get %s\n", dss.feat->clk_name); > - r = PTR_ERR(clk); > - goto err; > + return PTR_ERR(clk); > } > } else { > clk = NULL; > @@ -792,21 +789,12 @@ static int dss_get_clocks(void) > dss.dpll4_m4_ck = clk; > > return 0; > - > -err: > - if (dss.dss_clk) > - clk_put(dss.dss_clk); > - if (dss.dpll4_m4_ck) > - clk_put(dss.dpll4_m4_ck); > - > - return r; > } Why didn't you use devm_clk_get for the dpll4_m4_ck clock also? Tomi
On Wednesday 10 April 2013 01:43 PM, Tomi Valkeinen wrote: > On 2013-04-08 11:55, Archit Taneja wrote: >> Use devm_clk_get() instead of clk_get() for dss, and for outputs hdmi and venc. >> This reduces reduces code and simplifies error handling. >> >> Signed-off-by: Archit Taneja <archit@ti.com> >> --- >> drivers/video/omap2/dss/dss.c | 18 +++--------------- >> drivers/video/omap2/dss/hdmi.c | 16 ++-------------- >> drivers/video/omap2/dss/venc.c | 10 +--------- >> 3 files changed, 6 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c >> index 054c2a2..645b3bc 100644 >> --- a/drivers/video/omap2/dss/dss.c >> +++ b/drivers/video/omap2/dss/dss.c >> @@ -767,13 +767,11 @@ int dss_dpi_select_source(enum omap_channel channel) >> static int dss_get_clocks(void) >> { >> struct clk *clk; >> - int r; >> >> - clk = clk_get(&dss.pdev->dev, "fck"); >> + clk = devm_clk_get(&dss.pdev->dev, "fck"); >> if (IS_ERR(clk)) { >> DSSERR("can't get clock fck\n"); >> - r = PTR_ERR(clk); >> - goto err; >> + return PTR_ERR(clk); >> } >> >> dss.dss_clk = clk; >> @@ -782,8 +780,7 @@ static int dss_get_clocks(void) >> clk = clk_get(NULL, dss.feat->clk_name); >> if (IS_ERR(clk)) { >> DSSERR("Failed to get %s\n", dss.feat->clk_name); >> - r = PTR_ERR(clk); >> - goto err; >> + return PTR_ERR(clk); >> } >> } else { >> clk = NULL; >> @@ -792,21 +789,12 @@ static int dss_get_clocks(void) >> dss.dpll4_m4_ck = clk; >> >> return 0; >> - >> -err: >> - if (dss.dss_clk) >> - clk_put(dss.dss_clk); >> - if (dss.dpll4_m4_ck) >> - clk_put(dss.dpll4_m4_ck); >> - >> - return r; >> } > > Why didn't you use devm_clk_get for the dpll4_m4_ck clock also? clk_get of dpll4_m4_ck isn't tied to a device: clk = clk_get(NULL, dss.feat->clk_name); We can't use devm_clk_get() if we don't tie it to a device, right? I think the dss.dss_clk clock above is same as the dpll4_m4_ck for all OMAPs. We could probably remove the dpll4_m4_clk all together, and use dss_clk to get the rate of DSS_FCK coming from PRCM. Archit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-04-10 11:37, Archit Taneja wrote: > On Wednesday 10 April 2013 01:43 PM, Tomi Valkeinen wrote: >> On 2013-04-08 11:55, Archit Taneja wrote: >>> Use devm_clk_get() instead of clk_get() for dss, and for outputs hdmi >>> and venc. >>> This reduces reduces code and simplifies error handling. >>> >>> Signed-off-by: Archit Taneja <archit@ti.com> >>> --- >>> drivers/video/omap2/dss/dss.c | 18 +++--------------- >>> drivers/video/omap2/dss/hdmi.c | 16 ++-------------- >>> drivers/video/omap2/dss/venc.c | 10 +--------- >>> 3 files changed, 6 insertions(+), 38 deletions(-) >>> >>> diff --git a/drivers/video/omap2/dss/dss.c >>> b/drivers/video/omap2/dss/dss.c >>> index 054c2a2..645b3bc 100644 >>> --- a/drivers/video/omap2/dss/dss.c >>> +++ b/drivers/video/omap2/dss/dss.c >>> @@ -767,13 +767,11 @@ int dss_dpi_select_source(enum omap_channel >>> channel) >>> static int dss_get_clocks(void) >>> { >>> struct clk *clk; >>> - int r; >>> >>> - clk = clk_get(&dss.pdev->dev, "fck"); >>> + clk = devm_clk_get(&dss.pdev->dev, "fck"); >>> if (IS_ERR(clk)) { >>> DSSERR("can't get clock fck\n"); >>> - r = PTR_ERR(clk); >>> - goto err; >>> + return PTR_ERR(clk); >>> } >>> >>> dss.dss_clk = clk; >>> @@ -782,8 +780,7 @@ static int dss_get_clocks(void) >>> clk = clk_get(NULL, dss.feat->clk_name); >>> if (IS_ERR(clk)) { >>> DSSERR("Failed to get %s\n", dss.feat->clk_name); >>> - r = PTR_ERR(clk); >>> - goto err; >>> + return PTR_ERR(clk); >>> } >>> } else { >>> clk = NULL; >>> @@ -792,21 +789,12 @@ static int dss_get_clocks(void) >>> dss.dpll4_m4_ck = clk; >>> >>> return 0; >>> - >>> -err: >>> - if (dss.dss_clk) >>> - clk_put(dss.dss_clk); >>> - if (dss.dpll4_m4_ck) >>> - clk_put(dss.dpll4_m4_ck); >>> - >>> - return r; >>> } >> >> Why didn't you use devm_clk_get for the dpll4_m4_ck clock also? > > clk_get of dpll4_m4_ck isn't tied to a device: > > clk = clk_get(NULL, dss.feat->clk_name); > > We can't use devm_clk_get() if we don't tie it to a device, right? Ah, true. > I think the dss.dss_clk clock above is same as the dpll4_m4_ck for all > OMAPs. We could probably remove the dpll4_m4_clk all together, and use > dss_clk to get the rate of DSS_FCK coming from PRCM. Hmm, no, if I recall right, dpll4_m4_clk is the parent of dss clock (or parent of the parent of...), and the dss_fck_multiplier affects the resulting dss fck. Or something like that. It's a bit messy, especially as we can't just use the dss fck to change the rate, we need to change the rate at the parent for some reason. Tomi
On Wednesday 10 April 2013 02:49 PM, Tomi Valkeinen wrote: > On 2013-04-10 11:37, Archit Taneja wrote: >> On Wednesday 10 April 2013 01:43 PM, Tomi Valkeinen wrote: >>> On 2013-04-08 11:55, Archit Taneja wrote: >>>> Use devm_clk_get() instead of clk_get() for dss, and for outputs hdmi >>>> and venc. >>>> This reduces reduces code and simplifies error handling. >>>> >>>> Signed-off-by: Archit Taneja <archit@ti.com> >>>> --- >>>> drivers/video/omap2/dss/dss.c | 18 +++--------------- >>>> drivers/video/omap2/dss/hdmi.c | 16 ++-------------- >>>> drivers/video/omap2/dss/venc.c | 10 +--------- >>>> 3 files changed, 6 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/drivers/video/omap2/dss/dss.c >>>> b/drivers/video/omap2/dss/dss.c >>>> index 054c2a2..645b3bc 100644 >>>> --- a/drivers/video/omap2/dss/dss.c >>>> +++ b/drivers/video/omap2/dss/dss.c >>>> @@ -767,13 +767,11 @@ int dss_dpi_select_source(enum omap_channel >>>> channel) >>>> static int dss_get_clocks(void) >>>> { >>>> struct clk *clk; >>>> - int r; >>>> >>>> - clk = clk_get(&dss.pdev->dev, "fck"); >>>> + clk = devm_clk_get(&dss.pdev->dev, "fck"); >>>> if (IS_ERR(clk)) { >>>> DSSERR("can't get clock fck\n"); >>>> - r = PTR_ERR(clk); >>>> - goto err; >>>> + return PTR_ERR(clk); >>>> } >>>> >>>> dss.dss_clk = clk; >>>> @@ -782,8 +780,7 @@ static int dss_get_clocks(void) >>>> clk = clk_get(NULL, dss.feat->clk_name); >>>> if (IS_ERR(clk)) { >>>> DSSERR("Failed to get %s\n", dss.feat->clk_name); >>>> - r = PTR_ERR(clk); >>>> - goto err; >>>> + return PTR_ERR(clk); >>>> } >>>> } else { >>>> clk = NULL; >>>> @@ -792,21 +789,12 @@ static int dss_get_clocks(void) >>>> dss.dpll4_m4_ck = clk; >>>> >>>> return 0; >>>> - >>>> -err: >>>> - if (dss.dss_clk) >>>> - clk_put(dss.dss_clk); >>>> - if (dss.dpll4_m4_ck) >>>> - clk_put(dss.dpll4_m4_ck); >>>> - >>>> - return r; >>>> } >>> >>> Why didn't you use devm_clk_get for the dpll4_m4_ck clock also? >> >> clk_get of dpll4_m4_ck isn't tied to a device: >> >> clk = clk_get(NULL, dss.feat->clk_name); >> >> We can't use devm_clk_get() if we don't tie it to a device, right? > > Ah, true. > >> I think the dss.dss_clk clock above is same as the dpll4_m4_ck for all >> OMAPs. We could probably remove the dpll4_m4_clk all together, and use >> dss_clk to get the rate of DSS_FCK coming from PRCM. > > Hmm, no, if I recall right, dpll4_m4_clk is the parent of dss clock (or > parent of the parent of...), and the dss_fck_multiplier affects the > resulting dss fck. Or something like that. It's a bit messy, especially > as we can't just use the dss fck to change the rate, we need to change > the rate at the parent for some reason. Ah ok, I remember it now. One of the DPLL's divider's output is DSS_FCK. We get the parent's rate and iterate through all the possible hsdivider values to get the closest pixel clock. We don't/can't change dpll4_m4_clk as such, we just need to know it's rate for our calculations. So yes, we need dpll4_m4_clk, we just need to rename it to a better thing. Like dss_clk_parent. Anyway, Archit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 10 April 2013 02:58 PM, Archit Taneja wrote: > On Wednesday 10 April 2013 02:49 PM, Tomi Valkeinen wrote: >> On 2013-04-10 11:37, Archit Taneja wrote: >>> On Wednesday 10 April 2013 01:43 PM, Tomi Valkeinen wrote: >>>> On 2013-04-08 11:55, Archit Taneja wrote: >>>>> Use devm_clk_get() instead of clk_get() for dss, and for outputs hdmi >>>>> and venc. >>>>> This reduces reduces code and simplifies error handling. >>>>> >>>>> Signed-off-by: Archit Taneja <archit@ti.com> >>>>> --- >>>>> drivers/video/omap2/dss/dss.c | 18 +++--------------- >>>>> drivers/video/omap2/dss/hdmi.c | 16 ++-------------- >>>>> drivers/video/omap2/dss/venc.c | 10 +--------- >>>>> 3 files changed, 6 insertions(+), 38 deletions(-) >>>>> >>>>> diff --git a/drivers/video/omap2/dss/dss.c >>>>> b/drivers/video/omap2/dss/dss.c >>>>> index 054c2a2..645b3bc 100644 >>>>> --- a/drivers/video/omap2/dss/dss.c >>>>> +++ b/drivers/video/omap2/dss/dss.c >>>>> @@ -767,13 +767,11 @@ int dss_dpi_select_source(enum omap_channel >>>>> channel) >>>>> static int dss_get_clocks(void) >>>>> { >>>>> struct clk *clk; >>>>> - int r; >>>>> >>>>> - clk = clk_get(&dss.pdev->dev, "fck"); >>>>> + clk = devm_clk_get(&dss.pdev->dev, "fck"); >>>>> if (IS_ERR(clk)) { >>>>> DSSERR("can't get clock fck\n"); >>>>> - r = PTR_ERR(clk); >>>>> - goto err; >>>>> + return PTR_ERR(clk); >>>>> } >>>>> >>>>> dss.dss_clk = clk; >>>>> @@ -782,8 +780,7 @@ static int dss_get_clocks(void) >>>>> clk = clk_get(NULL, dss.feat->clk_name); >>>>> if (IS_ERR(clk)) { >>>>> DSSERR("Failed to get %s\n", dss.feat->clk_name); >>>>> - r = PTR_ERR(clk); >>>>> - goto err; >>>>> + return PTR_ERR(clk); >>>>> } >>>>> } else { >>>>> clk = NULL; >>>>> @@ -792,21 +789,12 @@ static int dss_get_clocks(void) >>>>> dss.dpll4_m4_ck = clk; >>>>> >>>>> return 0; >>>>> - >>>>> -err: >>>>> - if (dss.dss_clk) >>>>> - clk_put(dss.dss_clk); >>>>> - if (dss.dpll4_m4_ck) >>>>> - clk_put(dss.dpll4_m4_ck); >>>>> - >>>>> - return r; >>>>> } >>>> >>>> Why didn't you use devm_clk_get for the dpll4_m4_ck clock also? >>> >>> clk_get of dpll4_m4_ck isn't tied to a device: >>> >>> clk = clk_get(NULL, dss.feat->clk_name); >>> >>> We can't use devm_clk_get() if we don't tie it to a device, right? >> >> Ah, true. >> >>> I think the dss.dss_clk clock above is same as the dpll4_m4_ck for all >>> OMAPs. We could probably remove the dpll4_m4_clk all together, and use >>> dss_clk to get the rate of DSS_FCK coming from PRCM. >> >> Hmm, no, if I recall right, dpll4_m4_clk is the parent of dss clock (or >> parent of the parent of...), and the dss_fck_multiplier affects the >> resulting dss fck. Or something like that. It's a bit messy, especially >> as we can't just use the dss fck to change the rate, we need to change >> the rate at the parent for some reason. > > Ah ok, I remember it now. One of the DPLL's divider's output is DSS_FCK. > We get the parent's rate and iterate through all the possible hsdivider > values to get the closest pixel clock. We don't/can't change > dpll4_m4_clk as such, we just need to know it's rate for our calculations. > > So yes, we need dpll4_m4_clk, we just need to rename it to a better > thing. Like dss_clk_parent. > > Anyway, Sorry, hit the send button before completing the message. I wanted to add that we could push this patch as it is. Archit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013-04-10 12:28, Archit Taneja wrote: > Ah ok, I remember it now. One of the DPLL's divider's output is DSS_FCK. > We get the parent's rate and iterate through all the possible hsdivider > values to get the closest pixel clock. We don't/can't change > dpll4_m4_clk as such, we just need to know it's rate for our calculations. > > So yes, we need dpll4_m4_clk, we just need to rename it to a better > thing. Like dss_clk_parent. Right. And the field name in struct dss_features, "clk_name" is rather vague. At some point we should fix the omap clk framework so that we can set the dss fck directly. Presuming there's no good reason to have it as it is now. I'll apply the patch as it is now. Tomi
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c index 054c2a2..645b3bc 100644 --- a/drivers/video/omap2/dss/dss.c +++ b/drivers/video/omap2/dss/dss.c @@ -767,13 +767,11 @@ int dss_dpi_select_source(enum omap_channel channel) static int dss_get_clocks(void) { struct clk *clk; - int r; - clk = clk_get(&dss.pdev->dev, "fck"); + clk = devm_clk_get(&dss.pdev->dev, "fck"); if (IS_ERR(clk)) { DSSERR("can't get clock fck\n"); - r = PTR_ERR(clk); - goto err; + return PTR_ERR(clk); } dss.dss_clk = clk; @@ -782,8 +780,7 @@ static int dss_get_clocks(void) clk = clk_get(NULL, dss.feat->clk_name); if (IS_ERR(clk)) { DSSERR("Failed to get %s\n", dss.feat->clk_name); - r = PTR_ERR(clk); - goto err; + return PTR_ERR(clk); } } else { clk = NULL; @@ -792,21 +789,12 @@ static int dss_get_clocks(void) dss.dpll4_m4_ck = clk; return 0; - -err: - if (dss.dss_clk) - clk_put(dss.dss_clk); - if (dss.dpll4_m4_ck) - clk_put(dss.dpll4_m4_ck); - - return r; } static void dss_put_clocks(void) { if (dss.dpll4_m4_ck) clk_put(dss.dpll4_m4_ck); - clk_put(dss.dss_clk); } static int dss_runtime_get(void) diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c index 7292364..c7e0bf7 100644 --- a/drivers/video/omap2/dss/hdmi.c +++ b/drivers/video/omap2/dss/hdmi.c @@ -804,7 +804,7 @@ static int hdmi_get_clocks(struct platform_device *pdev) { struct clk *clk; - clk = clk_get(&pdev->dev, "sys_clk"); + clk = devm_clk_get(&pdev->dev, "sys_clk"); if (IS_ERR(clk)) { DSSERR("can't get sys_clk\n"); return PTR_ERR(clk); @@ -815,12 +815,6 @@ static int hdmi_get_clocks(struct platform_device *pdev) return 0; } -static void hdmi_put_clocks(void) -{ - if (hdmi.sys_clk) - clk_put(hdmi.sys_clk); -} - #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO) int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts) { @@ -1100,7 +1094,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev) r = hdmi_panel_init(); if (r) { DSSERR("can't init panel\n"); - goto err_panel_init; + return r; } dss_debugfs_create_file("hdmi", hdmi_dump_regs); @@ -1110,10 +1104,6 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev) hdmi_probe_pdata(pdev); return 0; - -err_panel_init: - hdmi_put_clocks(); - return r; } static int __exit hdmi_remove_child(struct device *dev, void *data) @@ -1135,8 +1125,6 @@ static int __exit omapdss_hdmihw_remove(struct platform_device *pdev) pm_runtime_disable(&pdev->dev); - hdmi_put_clocks(); - return 0; } diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c index 006caf3..c27ab6f 100644 --- a/drivers/video/omap2/dss/venc.c +++ b/drivers/video/omap2/dss/venc.c @@ -721,7 +721,7 @@ static int venc_get_clocks(struct platform_device *pdev) struct clk *clk; if (dss_has_feature(FEAT_VENC_REQUIRES_TV_DAC_CLK)) { - clk = clk_get(&pdev->dev, "tv_dac_clk"); + clk = devm_clk_get(&pdev->dev, "tv_dac_clk"); if (IS_ERR(clk)) { DSSERR("can't get tv_dac_clk\n"); return PTR_ERR(clk); @@ -735,12 +735,6 @@ static int venc_get_clocks(struct platform_device *pdev) return 0; } -static void venc_put_clocks(void) -{ - if (venc.tv_dac_clk) - clk_put(venc.tv_dac_clk); -} - static struct omap_dss_device * __init venc_find_dssdev(struct platform_device *pdev) { struct omap_dss_board_info *pdata = pdev->dev.platform_data; @@ -886,7 +880,6 @@ static int __init omap_venchw_probe(struct platform_device *pdev) err_panel_init: err_runtime_get: pm_runtime_disable(&pdev->dev); - venc_put_clocks(); return r; } @@ -904,7 +897,6 @@ static int __exit omap_venchw_remove(struct platform_device *pdev) venc_uninit_output(pdev); pm_runtime_disable(&pdev->dev); - venc_put_clocks(); return 0; }
Use devm_clk_get() instead of clk_get() for dss, and for outputs hdmi and venc. This reduces reduces code and simplifies error handling. Signed-off-by: Archit Taneja <archit@ti.com> --- drivers/video/omap2/dss/dss.c | 18 +++--------------- drivers/video/omap2/dss/hdmi.c | 16 ++-------------- drivers/video/omap2/dss/venc.c | 10 +--------- 3 files changed, 6 insertions(+), 38 deletions(-)