Re: [PATCH v12 2/6] video: add of helper for videomode
From: Tomi Valkeinen <hidden>
Date: 2012-11-21 12:22:25
Also in:
dri-devel, linux-devicetree, linux-media
On 2012-11-20 17:54, Steffen Trumtrar wrote:
+timings 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/non-inverted/ignored
Inverted related to what? And isn't this a bool? Pixel clock is either normal (whatever that is), or inverted. It can't be "not used". I guess normal case is "pixel data is driven on the rising edge of pixel clock"? If that's common knowledge, I guess it doesn't need to be mentioned. But I always have to verify from the documentation what "normal" means on this particular panel/soc =).
+ - interlaced (bool) + - doublescan (bool) + +All the optional properties that are not bool follow the following logic: + <1>: high active + <0>: low active + omitted: not used on hardware
Perhaps it's obvious, but no harm being explicit: mention that the bool properties are off is omitted. And I didn't read the rest of the patches yet, so perhaps this is already correct, but as I think this framework is usable without DT also, the meanings of the fields in the structs should be explained in the header files also in case they are not obvious.
+Example:
+
+ display-timings {
+ native-mode = <&timing0>;
+ timing0: 1920p24 {This should still be 1080p24, not 1920p24 =).
+ /* 1920x1080p24 */ + clock-frequency = <52000000>; + hactive = <1920>; + vactive = <1080>; + hfront-porch = <25>; + hback-porch = <25>; + hsync-len = <25>; + vback-porch = <2>; + vfront-porch = <2>; + vsync-len = <2>; + hsync-active = <1>; + }; + }; +
quoted hunk
diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h new file mode 100644 index 0000000..2b4fa0a --- /dev/null +++ b/include/linux/of_display_timings.h@@ -0,0 +1,20 @@ +/* + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de> + * + * display timings of helpers + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H +#define __LINUX_OF_DISPLAY_TIMINGS_H + +#include <linux/display_timing.h> +#include <linux/of.h>
No need to include these, just add "struct ...;".
quoted hunk
+#define OF_USE_NATIVE_MODE -1 + +struct display_timings *of_get_display_timings(const struct device_node *np); +int of_display_timings_exists(const struct device_node *np); + +#endifdiff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h new file mode 100644 index 0000000..4de5fcc --- /dev/null +++ b/include/linux/of_videomode.h@@ -0,0 +1,18 @@ +/* + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de> + * + * videomode of-helpers + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_OF_VIDEOMODE_H +#define __LINUX_OF_VIDEOMODE_H + +#include <linux/videomode.h> +#include <linux/of.h>
Same here.
+int of_get_videomode(const struct device_node *np, struct videomode *vm, + int index); + +#endif /* __LINUX_OF_VIDEOMODE_H */
Tomi
Attachments
- signature.asc [application/pgp-signature] 897 bytes