Message ID | 1428826797-576-1-git-send-email-hs@denx.de |
---|---|
State | Superseded |
Delegated to: | Anatolij Gustschin |
Headers | show |
Hi Heiko, On 04/12/2015 01:19 AM, Heiko Schocher wrote: > the ldb clock can be setup in board code (for example set through PLL5). > Update the ldb_clock rate also through board code. > > This should be removed, if a clock framework is availiable. > Any chance you're up to the task? Searching for 'clk_get' in the sources shows a proliferation of attempts for various SOCs and drivers. > Signed-off-by: Heiko Schocher <hs@denx.de> > --- > > drivers/video/ipu.h | 1 + > drivers/video/ipu_common.c | 11 +++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/drivers/video/ipu.h b/drivers/video/ipu.h > index 091b58f..f13934f 100644 > --- a/drivers/video/ipu.h > +++ b/drivers/video/ipu.h > @@ -266,4 +266,5 @@ void ipu_dp_uninit(ipu_channel_t channel); > void ipu_dp_dc_disable(ipu_channel_t channel, unsigned char swap); > ipu_color_space_t format_to_colorspace(uint32_t fmt); > I'd rather see this in imx-common/video.h to avoid #includes out of the driver tree. > +int ipu_set_ldb_clock(int rate); > #endif > diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c > index 1a209d4..dc054bc 100644 > --- a/drivers/video/ipu_common.c > +++ b/drivers/video/ipu_common.c > @@ -1198,3 +1198,14 @@ ipu_color_space_t format_to_colorspace(uint32_t fmt) > } > return RGB; > } > + > +/* should removed when clk framework is availiable */ > +int ipu_set_ldb_clock(int rate) > +{ This forces a tight dependency on when this is called that using ldb_clk directly would prevent. > + if (g_ldb_clk == NULL) > + return -ENOENT; > + > + g_ldb_clk->rate = rate; > + > + return 0; > +} > The use of g_ldb_clk seems pretty dodgy everywhere in the IPU driver. Otherwise: Tested-by: Eric Nelson <eric.nelson@boundarydevices.com>
Hello Eric, Am 16.04.2015 02:27, schrieb Eric Nelson: > Hi Heiko, > > On 04/12/2015 01:19 AM, Heiko Schocher wrote: >> the ldb clock can be setup in board code (for example set through PLL5). >> Update the ldb_clock rate also through board code. >> >> This should be removed, if a clock framework is availiable. >> > Any chance you're up to the task? > > Searching for 'clk_get' in the sources shows a proliferation > of attempts for various SOCs and drivers. Yes ... but it seems, I get no time for such a job :-( >> Signed-off-by: Heiko Schocher <hs@denx.de> >> --- >> >> drivers/video/ipu.h | 1 + >> drivers/video/ipu_common.c | 11 +++++++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/drivers/video/ipu.h b/drivers/video/ipu.h >> index 091b58f..f13934f 100644 >> --- a/drivers/video/ipu.h >> +++ b/drivers/video/ipu.h >> @@ -266,4 +266,5 @@ void ipu_dp_uninit(ipu_channel_t channel); >> void ipu_dp_dc_disable(ipu_channel_t channel, unsigned char swap); >> ipu_color_space_t format_to_colorspace(uint32_t fmt); >> > > I'd rather see this in imx-common/video.h to avoid #includes > out of the driver tree. ok, moved. >> +int ipu_set_ldb_clock(int rate); >> #endif >> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c >> index 1a209d4..dc054bc 100644 >> --- a/drivers/video/ipu_common.c >> +++ b/drivers/video/ipu_common.c >> @@ -1198,3 +1198,14 @@ ipu_color_space_t format_to_colorspace(uint32_t fmt) >> } >> return RGB; >> } >> + >> +/* should removed when clk framework is availiable */ >> +int ipu_set_ldb_clock(int rate) >> +{ > > This forces a tight dependency on when this is called that > using ldb_clk directly would prevent. Sorry, did not understand you here ... >> + if (g_ldb_clk == NULL) >> + return -ENOENT; >> + >> + g_ldb_clk->rate = rate; >> + >> + return 0; >> +} >> > > The use of g_ldb_clk seems pretty dodgy everywhere in the > IPU driver. > > Otherwise: > > Tested-by: Eric Nelson <eric.nelson@boundarydevices.com> Thanks for testing. bye, Heiko
Hi Heiko, On 04/17/2015 12:53 AM, Heiko Schocher wrote: >>> +int ipu_set_ldb_clock(int rate); >>> #endif >>> diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c >>> index 1a209d4..dc054bc 100644 >>> --- a/drivers/video/ipu_common.c >>> +++ b/drivers/video/ipu_common.c >>> @@ -1198,3 +1198,14 @@ ipu_color_space_t >>> format_to_colorspace(uint32_t fmt) >>> } >>> return RGB; >>> } >>> + >>> +/* should removed when clk framework is availiable */ >>> +int ipu_set_ldb_clock(int rate) >>> +{ >> >> This forces a tight dependency on when this is called that >> using ldb_clk directly would prevent. > > Sorry, did not understand you here ... > The variable g_ldb is initialized in ipu_probe(), so this routine must be called at a very particular time. You must have noticed it, since you put in a check for NULL. >>> + if (g_ldb_clk == NULL) >>> + return -ENOENT; >>> + If you replace this with ldb_clk.rate, the dependency goes away. >>> + g_ldb_clk->rate = rate; >>> + >>> + return 0; >>> +} >>> Separately, I don't see any reason not to have g_ldb_clk statically initialized... i.e. struct clk *g_ldb_clk = &ldb_clk;
diff --git a/drivers/video/ipu.h b/drivers/video/ipu.h index 091b58f..f13934f 100644 --- a/drivers/video/ipu.h +++ b/drivers/video/ipu.h @@ -266,4 +266,5 @@ void ipu_dp_uninit(ipu_channel_t channel); void ipu_dp_dc_disable(ipu_channel_t channel, unsigned char swap); ipu_color_space_t format_to_colorspace(uint32_t fmt); +int ipu_set_ldb_clock(int rate); #endif diff --git a/drivers/video/ipu_common.c b/drivers/video/ipu_common.c index 1a209d4..dc054bc 100644 --- a/drivers/video/ipu_common.c +++ b/drivers/video/ipu_common.c @@ -1198,3 +1198,14 @@ ipu_color_space_t format_to_colorspace(uint32_t fmt) } return RGB; } + +/* should removed when clk framework is availiable */ +int ipu_set_ldb_clock(int rate) +{ + if (g_ldb_clk == NULL) + return -ENOENT; + + g_ldb_clk->rate = rate; + + return 0; +}
the ldb clock can be setup in board code (for example set through PLL5). Update the ldb_clock rate also through board code. This should be removed, if a clock framework is availiable. Signed-off-by: Heiko Schocher <hs@denx.de> --- drivers/video/ipu.h | 1 + drivers/video/ipu_common.c | 11 +++++++++++ 2 files changed, 12 insertions(+)