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 at samsung.com> >>>>> Acked-by: Inki Dae <inki.dae at samsung.com> >>>>> Acked-by: Kyungmin Park <kyungmin.park at 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 >