From mboxrd@z Thu Jan 1 00:00:00 1970 From: YoungJun Cho Subject: Re: [RFC v2 PATCH v3 10/14] drm/panel: add S6E3FA0 driver Date: Tue, 22 Apr 2014 10:24:39 +0900 Message-ID: <5355C4D7.7010109@samsung.com> References: <1398083321-8668-1-git-send-email-yj44.cho@samsung.com> <1398083321-8668-11-git-send-email-yj44.cho@samsung.com> <5275422.8CoHT7g51N@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout1.samsung.com (mailout1.samsung.com [203.254.224.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 174F36E446 for ; Mon, 21 Apr 2014 18:24:41 -0700 (PDT) Received: from epcpsbgr3.samsung.com (u143.gpu120.samsung.co.kr [203.254.230.143]) by mailout1.samsung.com (Oracle Communications Messaging Server 7u4-24.01 (7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0N4E00EVTRX4C7A0@mailout1.samsung.com> for dri-devel@lists.freedesktop.org; Tue, 22 Apr 2014 10:24:40 +0900 (KST) In-reply-to: <5275422.8CoHT7g51N@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart , dri-devel@lists.freedesktop.org Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org, pawel.moll@arm.com, ijc+devicetree@hellion.org.uk, sw0312.kim@samsung.com, a.hajda@samsung.com, sachin.kamat@linaro.org, kyungmin.park@samsung.com, robh+dt@kernel.org, galak@codeaurora.org, kgene.kim@samsung.com List-Id: dri-devel@lists.freedesktop.org Hi Laurent, Thank you for the comment. 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 >> Acked-by: Inki Dae >> Acked-by: Kyungmin Park >> --- >> 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? Thank you. Best regards YJ > >> + drm_mode_probed_add(connector, mode); >> + >> + return 1; >> +} > > [snip] >