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:
quoted hunk
+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 =).
quoted hunk
+ - 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.
quoted hunk
+Example: + + display-timings { + native-mode = <&timing0>; + timing0: 1920p24 {
This should still be 1080p24, not 1920p24 =).
quoted hunk
+ /* 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.
quoted hunk
+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