Re: [PATCH v4] of: Add videomode helper

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

On Mon, Sep 24, 2012 at 09:16:28PM +0200, Sascha Hauer wrote:
> 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.

Okay. That doesn't seem to be a big problem. I can add that.

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 |