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 >
