Re: [PATCH v4] of: Add videomode helper

On 09/24/2012 07:42 AM, Rob Herring wrote:

On 09/19/2012 03:20 AM, Steffen Trumtrar 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

+++ b/Documentation/devicetree/bindings/video/displaymode

@@ -0,0 +1,74 @@
+videomode bindings
+=========
+
+Required properties:
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
+   in pixels
+   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
+   lines
+ - clock: displayclock in Hz

A major piece missing is the LCD controller to display interface width
and component ordering.

I thought this binding was solely defining the timing of the video
signal (hence "video mode"). Any definition of the physical interface to
the LCD/display-connector is something entirely orthogonal, so it seems
entirely reasonable to represent that separately.

quoted

+Example:
+
+	display@0 {

It would be useful to have a compatible string here. We may not always
know the panel type or have a fixed panel though. We could define
"generic-lcd" or something for cases where the panel type is unknown.

quoted

+		width-mm = <800>;
+		height-mm = <480>;

I would hope that everything in the example above this point is just
that - an example, and this binding only covers the display mode
definition - i.e. that part of the example below.

If that's not the intent, as Rob says, there's a /ton/ of stuff missing.

quoted

+		modes {
+			mode0: mode@0 {
+				/* 1920x1080p24 */
+				clock = <52000000>;
+				hactive = <1920>;
+				vactive = <1080>;
+				hfront-porch = <25>;
+				hback-porch = <25>;
+				hsync-len = <25>;
+				vback-porch = <2>;
+				vfront-porch = <2>;
+				vsync-len = <2>;
+				hsync-active-high;
+			};
+		};
+	};