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@xxxxxxxxxxx>
> > >> Acked-by: Inki Dae <inki.dae@xxxxxxxxxxx>
> > >> Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > >> ---
> > >>
> > >> 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?
Thierry
Attachment:
pgpCVwtjRpfkr.pgp
Description: PGP signature
