Re: [PATCH v4] of: Add videomode helper

[Steffen Trumtrar <s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

On Mon, Sep 24, 2012 at 05:09:30PM -0600, Stephen Warren wrote:
> 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.

I already added some of those things in my v5. For those who missed it
<1348500924-8551-1-git-send-email-s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
I renamed from "of videomode helper" to "of display helper", seemed to be more
clear what it is supposed to accomplish.

Regards,

Steffen


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |