On Mon, Sep 24, 2012 at 10:18:39AM -0500, Rob Herring wrote:

On 09/24/2012 09:12 AM, Sascha Hauer wrote:

quoted

quoted

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.

quoted

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 |