Re: [PATCH v4 5/5] drm/rockchip: Add support for Rockchip Soc EDP

[Rob Clark]

On Tue, Sep 23, 2014 at 9:56 AM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> On Tue, Sep 23, 2014 at 4:47 AM, cym <cym@xxxxxxxxxxxxxx> wrote:
>>
>> On Tuesday, September 23, 2014 03:20 AM, Rob Clark wrote:
>>>
>>> On Mon, Sep 22, 2014 at 7:02 AM, Mark yao <mark.yao@xxxxxxxxxxxxxx> wrote:
>>>>
>>>> This adds support for Rockchip soc edp found on rk3288
>>>>
>>>> Signed-off-by: Mark Yao <mark.yao@xxxxxxxxxxxxxx>
>>>> Signed-off-by: Jeff Chen <jeff.chen@xxxxxxxxxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>> - fix code sytle
>>>> - use some define from drm_dp_helper.h
>>>> - use panel-simple driver for primary display.
>>>> - remove unnecessary clock clk_24m_parent.
>>>>
>>>> Changes in v3: None
>>>>
>>>> Changes in v4: None
>>>>
>>>>   drivers/gpu/drm/rockchip/Kconfig             |    9 +
>>>>   drivers/gpu/drm/rockchip/Makefile            |    2 +
>>>>   drivers/gpu/drm/rockchip/rockchip_edp_core.c |  853 ++++++++++++++++++
>>>>   drivers/gpu/drm/rockchip/rockchip_edp_core.h |  309 +++++++
>>>>   drivers/gpu/drm/rockchip/rockchip_edp_reg.c  | 1202
>>>> ++++++++++++++++++++++++++
>>>>   drivers/gpu/drm/rockchip/rockchip_edp_reg.h  |  345 ++++++++
>>>>   6 files changed, 2720 insertions(+)
>>>>   create mode 100644 drivers/gpu/drm/rockchip/rockchip_edp_core.c
>>>>   create mode 100644 drivers/gpu/drm/rockchip/rockchip_edp_core.h
>>>>   create mode 100644 drivers/gpu/drm/rockchip/rockchip_edp_reg.c
>>>>   create mode 100644 drivers/gpu/drm/rockchip/rockchip_edp_reg.h
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/Kconfig
>>>> b/drivers/gpu/drm/rockchip/Kconfig
>>>> index 7146c80..04b1f8c 100644
>>>> --- a/drivers/gpu/drm/rockchip/Kconfig
>>>> +++ b/drivers/gpu/drm/rockchip/Kconfig
>>>> @@ -17,3 +17,12 @@ config DRM_ROCKCHIP
>>>>            management to userspace. This driver does not provides
>>>>            2D or 3D acceleration; acceleration is performed by other
>>>>            IP found on the SoC.
>>>> +
>>>> +config ROCKCHIP_EDP
>>>> +       bool "Rockchip edp support"
>>>> +       depends on DRM_ROCKCHIP
>>>> +       help
>>>> +         Choose this option if you have a Rockchip eDP.
>>>> +         Rockchip rk3288 SoC has eDP TX Controller can be used.
>>>> +         If you have an Embedded DisplayPort Panel, say Y to enable its
>>>> +         driver.
>>>> diff --git a/drivers/gpu/drm/rockchip/Makefile
>>>> b/drivers/gpu/drm/rockchip/Makefile
>>>> index 6e6d468..a0fc3a1 100644
>>>> --- a/drivers/gpu/drm/rockchip/Makefile
>>>> +++ b/drivers/gpu/drm/rockchip/Makefile
>>>> @@ -7,4 +7,6 @@ ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/rockchip
>>>>   rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o
>>>> rockchip_drm_fbdev.o \
>>>>                  rockchip_drm_gem.o rockchip_drm_vop.o
>>>>
>>>> +rockchipdrm-$(CONFIG_ROCKCHIP_EDP) += rockchip_edp_core.o
>>>> rockchip_edp_reg.o
>>>> +
>>>>   obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_edp_core.c
>>>> b/drivers/gpu/drm/rockchip/rockchip_edp_core.c
>>>> new file mode 100644
>>>> index 0000000..5450d1fa
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_edp_core.c
>>>> @@ -0,0 +1,853 @@
>>>> +/*
>>>> +* Copyright (C) Fuzhou Rockchip Electronics Co.Ltd
>>>> +* Author:
>>>> +*      Andy yan <andy.yan@xxxxxxxxxxxxxx>
>>>> +*      Jeff chen <jeff.chen@xxxxxxxxxxxxxx>
>>>> +*
>>>> +* based on exynos_dp_core.c
>>>> +*
>>>
>>> hmm, did you look at all at drm_dp_helpers?  The exynos code probably
>>> pre-dates the helpers, so might not be the best example to work off
>>> of..
>>>
>>> If there is actually a valid reason not to use the dp-helpers, then
>>> you should mention the reasons, at least in the commit msg if not the
>>> code
>>>
>>> BR,
>>> -R
>>
>> Thanks Rob,Because RK3288 eDP controller IP design is similar to exynos.They
>> from same IP vendors but have some difference.
>> So we choosed exynos_dp as example to work off of.exynos_dp only used some
>> defines from drm_dp_helper.h like DPCD.
>>
>
>
> Hmm, it sounds like it perhaps should be refactored out into a
> drm_bridge so more of it can be shared between rockchip and exynos.
>
> Either way, it should be using the drm_dp_helpers..  That "the code I
> copied did it wrong" isn't a terribly good reason for new drivers to
> do it wrong.
>
> So NAK for the eDP part until you use the helpers.

and btw, if it wasn't clear, go ahead and at least repost the core
part of the driver.. the first patch just needed a few small tweaks to
get my r-b even if it takes longer to sort out something sane for the
DP part..

BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/