Re: [PATCH v4] of: Add videomode helper

[Stephen Warren <swarren@wwwdotorg.org>]

On 09/24/2012 12:26 PM, Rob Herring wrote:
> 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.

The LCD panel itself defines both the mode and the physical interface
properties. The mode does not imply anything about the physical
interface, nor does the physical interface imply anything about the
mode. So, they are in fact orthogonal. In other words, just because you
need both sets of information, doesn't make the two pieces of
information correlated.

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

Everything related to the physical interface:

* For DSI, whatever it needs to be configured.
* For LVDS, e.g. number of lanes of R, G, B.
* Perhaps multi-pumping rates (# of clock signals to send each data
value for, to satisfy any minimum clock rates)
* Built-in displays typically need to be coupled with a backlight and
all the associated control of that.
* Pinctrl interaction.

and probably a bunch of other things I haven't thought about.