Re: [PATCH] drm/panel: add lg4573 driver

[Thierry Reding]

On Mon, Jun 08, 2015 at 10:13:21AM +0200, Heiko Schocher wrote:
> Hello Thierry,
> 
> Am 05.06.2015 14:19, schrieb Thierry Reding:
> >On Wed, May 06, 2015 at 09:49:33AM +0200, Heiko Schocher wrote:
[...]
> >>+  - display-timings: timings for the connected panel according to [1]
> >
> >The timings are already implied by the compatible value, so there's no
> >need to list them in DT.
> 
> I look into it ... is there an example for a panel driver with fixed
> timings? Should I do it like it is done in the
> drivers/gpu/drm/panel/panel-simple.c driver?

The simple-panel driver actually implements two methods. The old method
is to provide a fixed mode. That works fairly well, but primarily
because we don't support very many boards in the upstream kernel. For a
couple of panels it was shown that the fixed mode doesn't work very
well. One of the reasons was that the mode used values that conflicted
with the restrictions of the display controller on another SoC. A more
future-proof way would be for the driver to expose the display timings.
That allows the driver to compute a "default" mode, but also provide the
display driver with a full range of valid timings so that an appropriate
mode can be computed, taking into account the restrictions of the
display hardware.

The two Hannstar panels supported by the driver use this mechanism.
Perhaps you can use those for reference.

> >>+static void lg4573_display_mode_settings(struct lg4573 *ctx)
> >>+{
> >>+	static u16 display_mode_settings[] = {
> >>+	  0x703A,
> >[...]
> >>+	  0x7200,
> >>+	};
> >
> >Please make use of the 78/80 columns. Also, I don't suppose it'd be
> >possible to obtain symbolic names for these magic numbers? More of the
> >same below.
> 
> Fixed ... I try to find out more about this magic numbers, but I
> can;t promise it ...

It's not usual to get full documentation on these numbers, but we should
at least try.

> >>+static int lg4573_get_modes(struct drm_panel *panel)
> >>+{
> >>+	struct drm_connector *connector = panel->connector;
> >>+	struct lg4573 *ctx = panel_to_lg4573(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->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> >>+	drm_mode_probed_add(connector, mode);
> >>+
> >>+	return 1;
> >>+}
> >
> >You can either use a hard-coded mode or use display timings along with
> >the helpers to convert the timings to a default mode. No need to parse
> >the information from DT.
> 
> Ok... see question above, could I do it like it is done in the
> panel-simple driver? Or is there another way?

Look at the panel_simple_get_fixed_modes() implementation. That computes
a default mode from the typical values of the display timings.
Alternatively you can also implement display timings support in your
display driver and directly use the ->get_timings() callback for the
panel you have.

Thierry

Attachment: pgptTg4tGyu76.pgp
Description: PGP signature