Re: [PATCH v4] of: Add videomode helper

[Rob Herring <robherring2@gmail.com>]

On 09/24/2012 10:45 AM, Stephen Warren wrote:
> 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.

It is not orthogonal because in many cases the LCD panel defines the
mode. It is only cases where you just have a connector like hdmi that
you have multiple modes. Ideally, you would use EDID in those cases, but
obviously there are boards where that is missing or broken.

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

It's fairly clear this binding is being defined based on what Linux
supports vs. what the h/w looks like.

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

Assuming not, what all is missing?

Rob

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