Re: [PATCH V3 1/7] drm/exynos: Support DP CLKCON register in FIMD driver

[Jingoo Han <jg1.han@samsung.com>]

On Friday, June 27, 2014 10:03 PM, Ajay kumar wrote:
> On Fri, Jun 27, 2014 at 6:14 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> > On 06/27/2014 01:48 PM, Ajay kumar wrote:
> >> On Fri, Jun 27, 2014 at 4:52 PM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> >>> +CC DT
> >>>
> >>> On 06/27/2014 12:12 PM, Ajay Kumar wrote:
> >>>> Add the missing setting for DP CLKCON register.
> >>>>
> >>>> This register is present on Exynos5 based FIMD controllers,
> >>>> and needs to be set if we are using DP.
> >>>>
> >>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >>>> ---
> >>>>  .../devicetree/bindings/video/samsung-fimd.txt     |    1 +
> >>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c           |   23 ++++++++++++++++++++
> >>>>  include/video/samsung_fimd.h                       |    4 ++++
> >>>>  3 files changed, 28 insertions(+)

[.....]

> >>>>  static const struct of_device_id fimd_driver_dt_match[] = {
> >>>> @@ -331,6 +341,10 @@ static void fimd_commit(struct exynos_drm_manager *mgr)
> >>>>       if (clkdiv > 1)
> >>>>               val |= VIDCON0_CLKVAL_F(clkdiv - 1) | VIDCON0_CLKDIR;
> >>>>
> >>>> +     if (ctx->driver_data->has_dp_clkcon &&
> >>>> +             ctx->exynos_fimd_output_type == EXYNOS_FIMD_OUTPUT_DP)
> >>>> +             writel(DP_CLK_ENABLE, ctx->regs + DP_CLKCON);
> >>>> +
> >>>>       writel(val, ctx->regs + VIDCON0);
> >
> > New code should not split VIDCON0 related code.It should be moved few
> > lines above or few lines below.
> Ok, for better readability.
> 
> > Anyway this code should be rather placed in power related functions of
> > dp encoder, as it enables dp. The only question
> > is if DP_CLKCON update can be performed after VIDCON0 update. If yes the
> > solution of the whole problem
> I will check this.
> 
> > seems to be simple:
> > - fimd should provide function fimd_set_dp_clk_gate or sth similar,
> > - this function should be called in exynos_dp_poweron/exynos_dp_poweroff.
> > I hope I have not missed anything this time.
> But, it won't look good to export a FIMD function which sets a FIMD register,
> and call it in DP driver!
> What does Inki/Jingoo have to say about this?

I agree with Ajay Kumar's opinion.
It doesn't look good to export the function to set FIMD register
and call it by DP driver.

Best regards,
Jingoo Han
> 
> Regards,
> Ajay
> 
[....]