Hi Sascha,

On Friday 03 August 2012 09:38:44 Sascha Hauer wrote:

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.

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

b/Documentation/devicetree/bindings/video/displaymode

+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

That's correct. On the vertical direction, vfront-porch = lower-margin and 
vback-porch = upper-margin.

?

quoted

This node appears to describe a video mode, not a display, hence the
node name seems wrong.

Many displays can support multiple different video modes. As mentioned
elsewhere, properties like display width/height are properties of the
display not the mode.

So, I might expect something more like the following (various overhead
properties like reg/#address-cells etc. elided for simplicity):

disp: display {

    width-mm = <...>;
    height-mm = <...>;
    modes {
    
        mode@0 {
		
		/* 1920x1080p24 */
		clock = <52000000>;
		xres = <1920>;
		yres = <1080>;
		left-margin = <25>;
		right-margin = <25>;
		hsync-len = <25>;
		lower-margin = <2>;
		upper-margin = <2>;
		vsync-len = <2>;
		hsync-active-high;
		
        };
        mode@1 {
        };
    
    };

};

Ok, we can do this.

quoted

display-connector {

    display = <&disp>;
    // interface-specific properties such as pixel format here

};

Finally, have you considered just using an EDID instead of creating
something custom? I know that creating an EDID is harder than writing a

quoted

few simple properties into a DT, but EDIDs have the following advantages:

I have considered using EDID and I also tried it. It's painful. There
are no (open) tools available for creating EDID. That's something we
could change of course. Then when generating a devicetree there is
always an extra step involved creating the EDID blob. Once the EDID
blob is in devicetree it is not parsable anymore by mere humans, so
to see what we've got there is another tool involved to generate a
readable form again.

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.

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;
	}

-- 
Regards,

Laurent Pinchart