Re: [RFC v2 PATCH v3 10/14] drm/panel: add S6E3FA0 driver

[YoungJun Cho <yj44.cho@samsung.com>]

Hi Laurent,

Thank you for sharing your idea.

On 04/29/2014 12:05 AM, Laurent Pinchart wrote:
> Hi YoungJun,
>
> On Tuesday 22 April 2014 10:24:39 YoungJun Cho wrote:
>> On 04/22/2014 08:00 AM, Laurent Pinchart wrote:
>>> Hi YoungJun,
>>>
>>> Thank you for the patch.
>>>
>>> On Monday 21 April 2014 21:28:37 YoungJun Cho wrote:
>>>> This patch adds MIPI-DSI command mode based S6E3FA0 AMOLED LCD Panel
>>>> driver.
>>>>
>>>> Changelog v2:
>>>> - Declares delay, size properties in probe routine instead of DT
>>>> Changelog v3:
>>>> - Moves CPU timings relevant properties from FIMD DT
>>>>
>>>>     (commented by Laurent Pinchart, Andrzej Hajda)
>>>>
>>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/panel/Kconfig         |    7 +
>>>>    drivers/gpu/drm/panel/Makefile        |    1 +
>>>>    drivers/gpu/drm/panel/panel-s6e3fa0.c |  569 ++++++++++++++++++++++++++
>>>>    3 files changed, 577 insertions(+)
>>>>    create mode 100644 drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>> b/drivers/gpu/drm/panel/panel-s6e3fa0.c new file mode 100644
>>>> index 0000000..1282678
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>> @@ -0,0 +1,569 @@
>>>
>>> [snip]
>>>
>>>> +static int s6e3fa0_get_modes(struct drm_panel *panel)
>>>> +{
>>>> +	struct drm_connector *connector = panel->connector;
>>>> +	struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
>>>> +	struct drm_display_mode *mode;
>>>> +
>>>> +	mode = drm_mode_create(connector->dev);
>>>> +	if (!mode) {
>>>> +		DRM_ERROR("failed to create a new display mode\n");
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	drm_display_mode_from_videomode(&ctx->vm, mode);
>>>> +	mode->width_mm = ctx->width_mm;
>>>> +	mode->height_mm = ctx->height_mm;
>>>> +	connector->display_info.width_mm = mode->width_mm;
>>>> +	connector->display_info.height_mm = mode->height_mm;
>>>> +
>>>> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>>>> +	mode->private = (void *)&ctx->cpu_timings;
>>>
>>> Wouldn't it make sense to create a new get_interface_params (or similar)
>>> operation for drm_panel to query interface configuration parameters
>>> instead of shoving it in the mode private field ?
>>
>> You mean "new get_interface_params operation" is different from
>> get_modes() ?
>>
>> Till now, struct drm_display_mode and most of mode relevant APIs are
>> only for video interface.
>> And CPU interface also needs video mode configurations.
>>
>> I have a plan to implement the CPU interface relevant APIs like video
>> mode ones, but I think they should be used under current DRM mode APIs
>> like fill_modes, get_modes and so on.
>> So after that implementation, this private field will be replaced by
>> new ones.
>>
>> Could you explain it in more detail?
>
> The idea is that the interface parameters (RD/WR signals timings in this case,
> but this could also include MIPI DSI lane configuration or any other kind of
> physical interface parameters) are distinct from the video modes.

Yes. The RD/WR signals timings are distinct from the video modes,
but in my opinion, others are covered by video mode already.

>
> Do you see a need to tie tie interface parameters with drm_display_mode ? Can
> they be mode-specific ? In any case I'd like not to use the private field of
> drm_display_mode. If we need to tie both information together then it should
> be done in a standard way.

I think this cpu-mode-timings is in struct drm_display_mode
(NOT by *private) and requires drm_display_mode_from_cpumode()
because the drm_display_mode_from_videomode() covers only video mode.

I'll try implement it as soon as possible.

Thank you,
Best regards YJ.
>