Re: [PATCH v2] of: Add videomode helper

On 08/03/2012 01:38 AM, Sascha Hauer wrote:

Hi Stephen,

On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:

quoted

On 07/04/2012 01:56 AM, Sascha Hauer wrote:

quoted

This patch adds a helper function for parsing videomodes from the devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.

quoted

diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode

quoted

+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
+   in pixels
+   upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
+   lines

Perhaps bike-shedding, but...

For the margin property names, wouldn't it be better to use terminology
more commonly used for video timings rather than Linux FB naming. In
other words naming like:

hactive, hsync-len, hfront-porch, hback-porch?

Can do. Just to make sure:

hactive = xres
hsync-len = hsync-len
hfront-porch = right-margin
hback-porch = left-margin

I believe so yes.

quoted

a) They're already standardized and very common.

Indeed, that's a big plus for EDID. I have no intention of removing EDID
data from the devicetree. There are situations where EDID is handy, for
example when you get EDID data provided by your vendor.

quoted

b) They allow other information such as a display's HDMI audio
capabilities to be represented, which can then feed into an ALSA driver.

c) The few LCD panel datasheets I've seen actually quote their
specification as an EDID already, so deriving the EDID may actually be easy.

d) People familiar with displays are almost certainly familiar with
EDID's mode representation. There are many ways of representing display
modes (sync position vs. porch widths, htotal specified rather than
specifying all the components and hence htotal being calculated etc.).
Not everyone will be familiar with all representations. Conversion
errors are less likely if the target is EDID's familiar format.

You seem to think about a different class of displays for which EDID
indeed is a better way to handle. What I have to deal with here mostly
are dumb displays which:

- can only handle their native resolution
- Have no audio capabilities at all
- come with a datasheet which specify a min/typ/max triplet for
  xres,hsync,..., often enough they are scanned to pdf from some previously
  printed paper.

These displays are very common on embedded devices, probably that's the
reason I did not even think about the possibility that a single display
might have different modes.

That's true, but as I mentioned, there are at least some dumb panels
(both I've seen recently) whose specification provides the EDID. I don't
know how common that is though, I must admit.

quoted

e) You'll end up with exactly the same data as if you have a DDC-based
external monitor rather than an internal panel, so you end up getting to
a common path in display handling code much more quickly.

All we have in our display driver currently is:

	edidp = of_get_property(np, "edid", &imxpd->edid_len);
	if (edidp) {
		imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
	} else {
		ret = of_get_video_mode(np, &imxpd->mode, NULL);
		if (!ret)
			imxpd->mode_valid = 1;
	}

Presumably there's more to it though; something later checks
imxpd->mode_valid and if false, parses the EDID and sets up imxpd->mode,
etc.