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, 29 Apr 2014 15:11:58 +0900 Message-ID: <535F42AE.1070009@samsung.com> References: <1398083321-8668-1-git-send-email-yj44.cho@samsung.com> <5275422.8CoHT7g51N@avalon> <5355C4D7.7010109@samsung.com> <1852265.JnliZKqXKk@avalon> <20140428212547.GA13897@mithrandir> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <20140428212547.GA13897@mithrandir> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Thierry Reding , Laurent Pinchart Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, linux-samsung-soc@vger.kernel.org, pawel.moll@arm.com, ijc+devicetree@hellion.org.uk, sachin.kamat@linaro.org, sw0312.kim@samsung.com, dri-devel@lists.freedesktop.org, a.hajda@samsung.com, kyungmin.park@samsung.com, robh+dt@kernel.org, galak@codeaurora.org, kgene.kim@samsung.com List-Id: devicetree@vger.kernel.org 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 >>>>> 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? >> >> 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 >