Re: [PATCH v4] of: Add videomode helper

[Stephen Warren <swarren@wwwdotorg.org>]

On 09/24/2012 07:42 AM, Rob Herring wrote:
> On 09/19/2012 03:20 AM, Steffen Trumtrar wrote:
>> 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.

>> +++ 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.

>> +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.
> 
>> +		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.

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