[PATCH 1/4] drm/tilcdc: add TI LCD Controller DRM driver (v3)

[Rob Clark]

On Mon, Jan 28, 2013 at 3:56 AM, Mohammed, Afzal <afzal at ti.com> wrote:
> Hi Rob,
>
> On Fri, Jan 25, 2013 at 20:22:55, Rob Clark wrote:
>> On Fri, Jan 25, 2013 at 8:15 AM, Mohammed, Afzal <afzal at ti.com> wrote:
>
>> > It's not about being simple, but not doing the wrong way, here you are
>> > relying on a platform specific clock in a driver, think about the case
>> > where same IP is used on another platform. Either way it is not a good
>> > thing to handle platform specific details (about disp_clk) in driver.
>
>> Right, but I was trying to understand what was the benefit that the
>> added complexity is.  I didn't realize on davinci that you are limited
>
> Here I am referring to usage of disp_clk,
>
> Driver is not supposed to do platform hacks - here you are trying to
> configure something (PLL) in your driver which is not part of LCDC IP.
> And LCDC IP is not aware of PLL which is a platform specific matter
> (existent only in AM335x), it is only aware of functional clock.
>
> You can set the rate on "fck" (functional clock) in AM335x using my patch,
> "ARM: AM33XX: clock: SET_RATE_PARENT in lcd path", and there
> would not be any need for driver to be aware of platform specific PLL.

right, but I think it would be better to just make another patch that
changes tilcdc to just set rate on fck after that patch is merged.  I
mean, I'd rather have the driver at least usable on AM33xx until then,
rather than broken for everyone.

BR,
-R

>> >> >> +     priv->clk = clk_get(dev->dev, "fck");
>
>> >> >> +     priv->disp_clk = clk_get(dev->dev, "dpll_disp_ck");
>
>> at the moment all this discussion is a bit moot.  I'd propose leaving
>> the driver as it is for now, because that at least makes it useful on
>> am33xx.  And when CCF and davinci have the needed support in place,
>
> Let's forget about leveraging CCF in driver, but sane solution w.r.t PLL
> configuration would be to do as mentioned above.
>
> Regards
> Afzal
>