[PATCH v4] of: Add videomode helper

[Sascha Hauer]

On Mon, Sep 24, 2012 at 10:18:39AM -0500, Rob Herring wrote:
> On 09/24/2012 09:12 AM, Sascha Hauer wrote:
> >>
> >> A major piece missing is the LCD controller to display interface width
> >> and component ordering.
> > 
> > Psst. We silently skipped this for now...
> > 
> > I think having such a videomode helper is useful without having the
> > data order part. We are aware that this should be added later, but
> > I think currently it makes sense to concentrate on the basics.
> 
> Evolving bindings is not a good thing. We know this is needed, so we
> should address it now. If Linux had a standard way to describe the
> interface (it didn't a few years ago and I haven't kept up), then I
> would bet it would already be part of this binding. Defining the
> bindings in terms of what an OS uses or not is the wrong way around.

I see your point. I'm just afraid that if we start a discussion about
this now, it will take a long time we get something merged. In the
meantime we will get a lot of ad-hoc bindings like this one
suggested for the exynos:

> +       lcd_fimd0: lcd_panel0 {
> +                       lcd-htiming = <4 4 4 480>;
> +                       lcd-vtiming = <4 4 4 320>;
> +                       supports-mipi-panel;
> +       };

Or this one for the AMBA LCD controller;

> +       panel.mode.refresh      = get_val(node, "refresh");
> +       panel.mode.xres         = get_val(node, "xres");
> +       panel.mode.yres         = get_val(node, "yres");
> +       panel.mode.pixclock     = get_val(node, "pixclock");
> +       panel.mode.left_margin  = get_val(node, "left_margin");
> +       panel.mode.right_margin = get_val(node, "right_margin");
> +       panel.mode.upper_margin = get_val(node, "upper_margin");
> +       panel.mode.lower_margin = get_val(node, "lower_margin");
> +       panel.mode.hsync_len    = get_val(node, "hsync_len");
> +       panel.mode.vsync_len    = get_val(node, "vsync_len");
> +       panel.mode.sync         = get_val(node, "sync");
> +       panel.bpp               = get_val(node, "bpp");
> +       panel.width             = (signed short) get_val(node, "width");
> +       panel.height            = (signed short) get_val(node, "height");

(get_val() is a wrapper around of_property_read_u32)

BTW the SoC-camera guys will need a wire format description for their
cameras aswell.

> > 
> > We expect the display nodes being subnodes of a driver (which of course
> > has a compatible), or maybe referred to from a driver using phandles. So
> > I don't see why the display node itself should have a compatible
> > property. The display information is only ever useful in the context of
> > a driver.
> 
> A subnode or phandle will describe the h/w connection, but you need a
> name to describe what is at each end of the connection.
> 
> Where would the model number of an lcd panel be captured then? The
> timing parameters are a property of a specific panel, so both should be
> described together. You may not have any use for the compatible string
> now, but more information is better than less and adding it would not
> hurt anything. For pretty much any other device sitting on a board, we
> describe the manufacturer and type of device. LCD panels should be no
> different.

You convinced me. Lets add a compatible property, it won't hurt.

Sascha


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