On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
This adds support for reading display timings from DT or/and convert one of those timings to a videomode. The of_display_timing implementation supports multiple children where each property can have up to 3 values. All children are read into an array, that can be queried. of_get_videomode converts exactly one of that timings to a struct videomode.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de Signed-off-by: Philipp Zabel p.zabel@pengutronix.de
.../devicetree/bindings/video/display-timings.txt | 107 +++++++++++ drivers/video/Kconfig | 13 ++ drivers/video/Makefile | 2 + drivers/video/of_display_timing.c | 186 ++++++++++++++++++++ drivers/video/of_videomode.c | 47 +++++ include/linux/of_display_timings.h | 19 ++ include/linux/of_videomode.h | 15 ++ 7 files changed, 389 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/display-timings.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_timings.h create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt new file mode 100644 index 0000000..e22e2fd --- /dev/null +++ b/Documentation/devicetree/bindings/video/display-timings.txt @@ -0,0 +1,107 @@ +display-timings bindings +========================
+display-timings-node +--------------------
Maybe leave away the last -, so that it reads "display-timings node"? I think that makes it more obvious that the node is supposed to be called "display-timings".
+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.+timings-subnode +---------------
Same here: "timing subnodes"?
+required properties:
- hactive, vactive: Display resolution
- hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
"display"?
- in pixels
- vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
- lines
- clock-frequency: displayclock in Hz
"display clock"?
+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
- 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
Nitpick: You use space before : in the optional properties, but not in the required properties above.
+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.
+The parameters are defined as
+struct display_timing +=====================
struct display_timing has no meaning in device tree documentation. Maybe this line can just go away?
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 2a23b18..a324457 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -39,6 +39,19 @@ config DISPLAY_TIMING config VIDEOMODE bool
+config OF_DISPLAY_TIMING
OF_DISPLAY_TIMINGS?
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
of_display_timings.c?
+/**
- of_get_display_timings - parse all display_timing entries from a device_node
- @np: device_node with the subnodes
- **/
+struct display_timings *of_get_display_timings(struct device_node *np)
[...]
- for_each_child_of_node(timings_np, entry) {
struct display_timing *dt;dt = of_get_display_timing(entry);if (!dt) {/* to not encourage wrong devicetrees, fail in case of an error */pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);return NULL;}
In case of a parsing error, of_get_display_timing() already shows an error message, so I don't think we need another one here.
+/**
- of_display_timings_exists - check if a display-timings node is provided
- @np: device_node with the timing
- **/
+int of_display_timings_exists(struct device_node *np) +{
- struct device_node *timings_np;
- if (!np)
return -EINVAL;- timings_np = of_parse_phandle(np, "display-timings", 0);
- if (!timings_np)
return -EINVAL;- return 1;
I think this is missing of_node_put(timings_np) in both failure and success cases. Also, maybe this should really return a bool instead?
Also, why do you use of_parse_phandle() for this? Aren't the display-timings nodes expected to be children of some other node like an output/display device?
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
[...]
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index); +#endif /* __LINUX_OF_VIDEOMODE_H */
There should be a blank line between the last two lines I think.
Thierry