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

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

Hi Thierry,

Thank you for the comments.

On 04/29/2014 06:25 AM, Thierry Reding wrote:
> On Mon, Apr 28, 2014 at 05:05:24PM +0200, 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.
>
> We already have the lanes field in struct mipi_dsi_device. I think in
> general I'd prefer to not spread these parameters around too wildly. If
> this is a general requirement for DBI devices, perhaps what we need is
> struct mipi_dbi_device?
>

Even though it requires CPU mode relevant parameters,
this is also mipi dsi interface.

So I think struct mipi_dsi_device is enough.

Thank you.
Best regards YJ

> Thierry
>