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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4563 bytes --]

Hi Thierry,

On Monday 28 April 2014 23:25:50 Thierry Reding wrote:
> On Mon, Apr 28, 2014 at 05:05:24PM +0200, Laurent Pinchart wrote:
> > 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.

Seems I've missed the addition of mipi_dsi_device to DRM.

> 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?

That could be done, but I'm not sure we should expose the nature of the panel 
device (i.e. "I'm a mipi_dsi_device") to the display controller. I would be 
worried about using container_of() on panel->dev to get a mipi_dsi_device 
pointer, as we would then need a strict guarantee that the panel->dev pointer 
is embedded directly in a mipi_dsi_device. This might be doable for DSI, but 
other kind of panels might be connected to different control busses (I'm 
thinking about I2C and SPI in particular) and still need to expose video 
interface parameters to the display controller driver.

-- 
Regards,

Laurent Pinchart

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel