RE: [PATCH v4 12/12] video: da8xx-fb: CCF clock divider handling

[Mohammed, Afzal]

Hi Mike,

On Thu, Jan 24, 2013 at 01:52:04, Mike Turquette wrote:
> Quoting Afzal Mohammed (2013-01-23 03:48:56)

> > +static inline void da8xx_fb_clkc_enable(void)
> > +{
> >         if (lcd_revision == LCD_VERSION_2)
> >                 lcdc_write(LCD_V2_DMA_CLK_EN | LCD_V2_LIDD_CLK_EN |
> >                                 LCD_V2_CORE_CLK_EN, LCD_CLK_ENABLE_REG);

> > +static inline int da8xx_fb_calc_config_clk_divider(struct da8xx_fb_par *par,
> > +                                                   struct fb_videomode *mode)
> > +{

> > +       ret = clk_set_rate(par->child_clk, PICOS2KHZ(mode->pixclock) * 1000);
> > +       if (IS_ERR_VALUE(ret)) {
> > +               dev_err(par->dev, "unable to setup pixel clock of %u ps",
> > +                       mode->pixclock);
> > +               return ret;
> > +       }
> > +       da8xx_fb_clkc_enable();

> It looks like you are using the legacy method to enable/disable the
> clock and using the CCF basic divider to set the rate.  This feels a bit
> hacky to me.  If you want to model your clock in CCF then you should
> probably model the whole clock, not just the rate-change aspects of it.

Initially I thought about it, but seeing requirement of 3 gate clocks
(due to 3 bits meant for different purposes - DMA, LIDD and CORE
functionalities), felt that having 4 clocks (3 gate + 1 divider) in
driver would be an overdesign [leaving a branch instead of a leaf of
the tree in driver ;)].

> Have you looked at the composite clock patches from Prashant?  Those
> might give you the divider+gate properties that you are looking for:
> http://article.gmane.org/gmane.linux.kernel/1416697

Thanks for the pointer,

Now with the composite clock in mind, it was tried to relate to what
was required for the present scenario.

So there are 3 - LIDD is actually not for present use case, CORE could
be clubbed with the divider to have a composite clock. And CORE is
in functional clock path and logically it's perfectly alright to have
the composite clock.

And now we are left with DMA, this is actually in the interface clock
path which driver in unaware. An option would be to have DMA clock
as child of CORE plus divider composite clock, even though logically
DMA can't be considered in the same path.

Also tried not enabling DMA clock, but driver is able to provide
display without any issues, so was thinking whether to avoid
instantiating DMA clock at all and hence to have a simple single
composite clock. Trying to get information internally on whether
not setting DMA clock bits would actually make a difference.

If you have any opinion on how to deal here, let me know.

Regards
Afzal


¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_