Hi, On Mon, Nov 26, 2012 at 04:38:36PM +0200, Tomi Valkeinen wrote: > Hi, > > On 2012-11-26 11:07, Steffen Trumtrar wrote: > > This adds support for reading display timings from DT into a struct > > display_timings. The of_display_timing implementation supports multiple > > subnodes. All children are read into an array, that can be queried. > > > > If no native mode is specified, the first subnode will be used. > > > > For cases where the graphics driver knows there can be only one > > mode description or where the driver only supports one mode, a helper > > function of_get_videomode is added, that gets a struct videomode from DT. > > > > Signed-off-by: Steffen Trumtrar <s.trumt...@pengutronix.de> > > Signed-off-by: Philipp Zabel <p.za...@pengutronix.de> > > Acked-by: Stephen Warren <swar...@nvidia.com> > > Reviewed-by: Thierry Reding <thierry.red...@avionic-design.de> > > Acked-by: Thierry Reding <thierry.red...@avionic-design.de> > > Tested-by: Thierry Reding <thierry.red...@avionic-design.de> > > Tested-by: Philipp Zabel <p.za...@pengutronix.de> > > Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > > Acked-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > > --- > > .../devicetree/bindings/video/display-timing.txt | 107 ++++++++++ > > drivers/video/Kconfig | 15 ++ > > drivers/video/Makefile | 2 + > > drivers/video/of_display_timing.c | 219 > > ++++++++++++++++++++ > > drivers/video/of_videomode.c | 54 +++++ > > include/linux/of_display_timing.h | 20 ++ > > include/linux/of_videomode.h | 18 ++ > > 7 files changed, 435 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/video/display-timing.txt > > create mode 100644 drivers/video/of_display_timing.c > > create mode 100644 drivers/video/of_videomode.c > > create mode 100644 include/linux/of_display_timing.h > > create mode 100644 include/linux/of_videomode.h > > > > diff --git a/Documentation/devicetree/bindings/video/display-timing.txt > > b/Documentation/devicetree/bindings/video/display-timing.txt > > new file mode 100644 > > index 0000000..e238f27 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/video/display-timing.txt > > @@ -0,0 +1,107 @@ > > +display-timing bindings > > +======================= > > + > > +display-timings node > > +-------------------- > > + > > +required properties: > > + - none > > + > > +optional properties: > > + - native-mode: The native mode for the display, in case multiple modes are > > + provided. When omitted, assume the first node is the native. > > + > > +timing subnode > > +-------------- > > + > > +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-frequency: display clock in Hz > > + > > +optional properties: > > + - hsync-active: hsync pulse is active low/high/ignored > > + - vsync-active: vsync pulse is active low/high/ignored > > + - de-active: data-enable pulse is active low/high/ignored > > + - pixelclk-inverted: pixelclock is inverted (active on falling edge)/ > > + non-inverted (active on rising edge)/ > > + ignored (ignore property) > > I think hsync-active and vsync-active are clear, and commonly used, and > they are used for both drm and fb mode conversions in later patches. > > de-active is not used in drm and fb mode conversions, but I think it's > also clear. > > pixelclk-inverted is not used in the mode conversions. It's also a bit > unclear to me. What does it mean that pix clock is "active on rising > edge"? The pixel data is driven on rising edge? How about the sync > signals and DE, when are they driven? Does your HW have any settings > related to those? >
Those are properties commonly found in display specs. That is why they are here. If the GPU does not support the property it can be omitted. > OMAP has the invert pclk setting, but it also has a setting to define > when the sync signals are driven. The options are: > - syncs are driven on rising edge of pclk > - syncs are driven on falling edge of pclk > - syncs are driven on the opposite edge of pclk compared to the pixel data > > For DE there's no setting, except the active high/low. > > And if I'm not mistaken, if the optional properties are not defined, > they are not ignored, but left to the default 0. Which means active low, > or active on rising edge(?). I think it would be good to have a > "undefined" value for the properties. > Yes. As mentioned in my other mail, the intention of the omitted properties do not propagate properly. Omitted must be a value < 0, so it is clear in a later stage, that this property shall not be used. And isn't unintentionally considered to be active low. > > + - interlaced (bool): boolean to enable interlaced mode > > + - doublescan (bool): boolean to enable doublescan mode > > + - doubleclk (bool) > > As I mentioned in the other mail, doubleclk is not used nor documented here. > Yes. Rebase mistake I overlooked. > > +All the optional properties that are not bool follow the following logic: > > + <1>: high active > > + <0>: low active > > + omitted: not used on hardware > > + > > +There are different ways of describing the capabilities of a display. The > > devicetree > > +representation corresponds to the one commonly found in datasheets for > > displays. > > +If a display supports multiple signal timings, the native-mode can be > > specified. > > I have some of the same concerns for this series than with the > interpreted power sequences (on fbdev): when you publish the DT > bindings, it's somewhat final version then, and fixing it later will be > difficult. Of course, video modes are much clearer than the power > sequences, so it's possible there won't be any problems with the DT > bindings. > The binding is pretty much at the bare minimum after a lot of discussion about the properties. Even if the binding changes, I think it will rather grow than shrink. Take the doubleclock property for example. It got here mistakingly, because we had a display that has this feature. > However, I'd still feel safer if the series would be split to non-DT and > DT parts. The non-DT parts could be merged quite easily, and people > could start using them in their kernels. This should expose > bugs/problems related to the code. > > The DT part could be merged later, when there's confidence that the > timings are good for all platforms. > > Or, alternatively, all the non-common bindings (de-active, pck > invert,...) that are not used for fbdev or drm currently could be left > out for now. But I'd stil prefer merging it in two parts. I don't say that I'm against it, but the whole reason for the series was getting the display timings from a DT into a graphics driver. And I think I remember seeing some other attempts at achieving this, but all specific to one special case. There is even already a mainline driver that uses an older version of the DT bindings (vt8500-fb). 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 | _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
