Hi Thierry,

On 09/21/2015 07:22 PM, Thierry Reding wrote:
> On Mon, Sep 21, 2015 at 06:27:40PM +0800, Yakir Yang wrote:
>> Hi Thierry,
>>
>> Thanks for your suggest :)
>>
>> On 09/21/2015 05:15 PM, Thierry Reding wrote:
>>> On Mon, Sep 21, 2015 at 04:45:44PM +0800, Yakir Yang wrote:
>>>> Hi Heiko,
>>>>
>>>> On 09/02/2015 10:15 AM, Yakir Yang wrote:
>>>>> Hi Heiko,
>>>>>
>>>>> 在 09/02/2015 05:47 AM, Heiko Stuebner 写道:
>>>>>> Hi Yakir,
>>>>>>
>>>>>> Am Dienstag, 1. September 2015, 13:46:11 schrieb Yakir Yang:
>>>>>>>     The Samsung Exynos eDP controller and Rockchip RK3288 eDP
>>>>>>> controller
>>>>>>> share the same IP, so a lot of parts can be re-used. I split the common
>>>>>>> code into bridge directory, then rk3288 and exynos only need to keep
>>>>>>> some platform code. Cause I can't find the exact IP name of exynos dp
>>>>>>> controller, so I decide to name dp core driver with "analogix" which I
>>>>>>> find in rk3288 eDP TRM ;)
>>>>>>>
>>>>>>> Beyond that, there are three light registers setting differents bewteen
>>>>>>> exynos and rk3288.
>>>>>>> 1. RK3288 have five special pll resigters which not indicata in exynos
>>>>>>>     dp controller.
>>>>>>> 2. The address of DP_PHY_PD(dp phy power manager register) are
>>>>>>> different
>>>>>>>     between rk3288 and exynos.
>>>>>>> 3. Rk3288 and exynos have different setting with AUX_HW_RETRY_CTL(dp
>>>>>>> debug
>>>>>>>     register).
>>>>>>>
>>>>>>> I have verified this series on two kinds of rockchip platform board,
>>>>>>> one
>>>>>>> is rk3288 sdk board which connect with a 2K display port monitor, the
>>>>>>> other
>>>>>>> is google jerry chromebook which connect with a eDP screen
>>>>>>> "cnm,n116bgeea2",
>>>>>>> both of them works rightlly.
>>>>>> it looks like during the rebase something did go wrong and I found some
>>>>>> issues
>>>>>> I mentioned in the replies to individual patches.
>>>>>>
>>>>>> I did prepare a branch based on mainline [0] with both the old and the
>>>>>> new edp
>>>>>> driver - rk3288_veyron_defconfig build both drivers into the image.
>>>>>>
>>>>>> While the old driver still works, I wasn't able to make the new one work
>>>>>> yet
>>>>>> ... the drm core does find the connector, but not that anything is
>>>>>> connected
>>>>>> to it. I'll try to dig deeper tomorrow, but maybe you'll see anything
>>>>>> interesting before then.
>>>>> Many thanks for your comment and debug, I would rebase on your
>>>>> "edp-with-veyron" branch and fix the broken, make sure v6 would
>>>>> work rightly at least in your side and my side.
>>>> Just like we talk off line, I guess there are two tricky questions which
>>>> make analogix_dp just crash/failed on rockchip platform:
>>>>
>>>> -  One is how to reach a agreement with the common way to register
>>>> connector. There would be a conflict with Exynos & IMX & Rockchip.
>>>>       On analogix_dp thread, Exynos want to register connector when that
>>>> connector is ready.
>>>>       On dw_hdmi thread, IMX want to register connector when all component is
>>>> already.
>>>>       So Exynos & IMX & Rockchip should reach a common way to register
>>>> connector to fix this issue.
>>>>
>>>> -  The other is atomic API.
>>>>        The rockchip drm haven't implemented the atomic API, but the original
>>>> exynos_dp have used the atomic API on connector helper function. That's why
>>>> analogix_dp just keep crash on your side.
>>> There's really no reason not to convert Rockchip to atomic. It will have
>>> to happen eventually anyway.
>> Do agree on this point, and I see Tomasz Figa have done some WIP
>> works on implementing the atomic_commit, maybe would upstream
>> in further.(https://chromium-review.googlesource.com/#/c/284560/1)
>>
>>
>>> That said, there's another option that would allow you to side-step both
>>> of the above problems at the same time. If you turn the common code into
>>> a helper library that should give you enough flexibility to integrate it
>>> into all existing users. For example you could leave out the connector
>>> registration and let the drivers do that. Similarly since the helpers
>>> are only hooked up at registration time you could probably find a way to
>>> share the low-level code but again leave it up to the drivers to glue it
>>> all together at registration time (drivers could wrap the low-level code
>>> with atomic or non-atomic callbacks).
>> Wow, sounds good, but I'm not sure I understand this rightly. Do you
>> mean that I could support two kinds of callbacks in analogix_dp_core
>> driver, and export them out. And move the connector registration code
>> into the helper driver (like exynos_dp.c), so helper driver could chose to
>> use the atomic or non-atomic callbacks. like:
>>
>> -- analogix_dp_core.c
>> --------------------------------------------------------------------
>> ...
>> struct drm_connector_funcs analogix_dp_connector_atomic_funcs = {
>>          .dpms = drm_atomic_helper_connector_dpms,
>>          .fill_modes = drm_helper_probe_single_connector_modes,
>>          .detect = analogix_dp_detect,
>>          .destroy = analogix_dp_connector_destroy,
>>          .reset = drm_atomic_helper_connector_reset,
>>          .atomic_duplicate_state =
>> drm_atomic_helper_connector_duplicate_state,
>>          .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> };
>> EXPORT_SYMBOL(analogix_dp_connector_atomic_funcs);
>>
>> struct drm_connector_funcs analogix_dp_connector_funcs = {
>>          .dpms = drm_helper_connector_dpms,
>>          .fill_modes = drm_helper_probe_single_connector_modes,
>>          .detect = analogix_dp_detect,
>>          .destroy = analogix_dp_connector_destroy,
>> };
>> EXPORT_SYMBOL(analogix_dp_connector_funcs);
>>
>> struct drm_connector_helper_funcs analogix_dp_connector_helper_funcs = {
>>          .get_modes = analogix_dp_get_modes,
>>          .best_encoder = analogix_dp_best_encoder,
>> };
>> EXPORT_SYMBOL(analogix_dp_connector_helper_funcs);
>> ...
>>
>> -- exynos_dp
>> ----------------------------------------------------------------------------
>>          ret = drm_connector_init(dp->drm_dev, connector,
>> &analogix_dp_connector_atomic_funcs,
>>                                   DRM_MODE_CONNECTOR_eDP);
>>          if (ret) {
>>                  DRM_ERROR("Failed to initialize connector with drm\n");
>>                  return ret;
>>          }
>>
>>          drm_connector_helper_add(connector,
>> &analogix_dp_connector_helper_funcs);
>>          drm_connector_register(connector);
>>          drm_mode_connector_attach_encoder(connector, encoder);
>>
>> -- analogix_dp-rockchip
>> ----------------------------------------------------------------------------
>>          ret = drm_connector_init(dp->drm_dev, connector,
>>                                   &analogix_dp_connector_funcs,
>>                                   DRM_MODE_CONNECTOR_eDP);
>>          if (ret) {
>>                  DRM_ERROR("Failed to initialize connector with drm\n");
>>                  return ret;
>>          }
>>
>>          drm_connector_helper_add(connector,
>> &analogix_dp_connector_helper_funcs);
>>          drm_mode_connector_attach_encoder(connector, encoder);
>>
>>
>> Are those code corresponding to your suggestion.   :)
> Yes, that looks about right. You could also move the implementations
> into the Exynos and Rockchip drivers, respectively, if they're only used
> from one place. Then you can simply export the low-level analogix_dp_*()
> functions. That might give you even more flexibility, but the above
> would probably work well enough.

Wow, much better, so I just need to export two low-level functions,

  * analogix_dp_detect()
  * analogix_dp_get_modes()

Pretty cool.

Thanks,
- Yakir

> Thierry