Re: [PATCH 1/2 v6] of: add helper to parse display timings
From: Tomi Valkeinen <hidden>
Date: 2012-10-08 07:07:45
Also in:
dri-devel, linux-devicetree, linux-media
Hi, On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
quoted hunk ↗ jump to hunk
Signed-off-by: Steffen Trumtrar <redacted> --- .../devicetree/bindings/video/display-timings.txt | 222 ++++++++++++++++++++ drivers/of/Kconfig | 5 + drivers/of/Makefile | 1 + drivers/of/of_display_timings.c | 183 ++++++++++++++++ include/linux/of_display_timings.h | 85 ++++++++ 5 files changed, 496 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt create mode 100644 drivers/of/of_display_timings.c create mode 100644 include/linux/of_display_timings.hdiff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt new file mode 100644 index 0000000..45e39bd --- /dev/null +++ b/Documentation/devicetree/bindings/video/display-timings.txt@@ -0,0 +1,222 @@ +display-timings bindings +================== + +display-timings-node +------------ + +required properties: + - none + +optional properties: + - default-timing: the default timing value + +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: displayclock in Hz + +optional properties: + - hsync-active-high (bool): Hsync pulse is active high + - vsync-active-high (bool): Vsync pulse is active high + - de-active-high (bool): Data-Enable pulse is active high + - pixelclk-inverted (bool): pixelclock is inverted + - interlaced (bool) + - doublescan (bool)
I think bool should be generally used for things that are on/off, like interlace. For hsync-active-high & others I'd rather have 0/1 values as others already suggested.
+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 default-timing can be specified.
+
+The parameters are defined as
+
+struct signal_timing
+===================
+
+ +----------+---------------------------------------------+----------+-------+
+ | | ↑ | | |
+ | | |vback_porch | | |
+ | | ↓ | | |
+ +----------###############################################----------+-------+
+ | # ↑ # | |
+ | # | # | |
+ | hback # | # hfront | hsync |
+ | porch # | hactive # porch | len |
+ |<-------->#<---------------+--------------------------->#<-------->|<----->|
+ | # | # | |
+ | # |vactive # | |
+ | # | # | |
+ | # ↓ # | |
+ +----------###############################################----------+-------+
+ | | ↑ | | |
+ | | |vfront_porch | | |
+ | | ↓ | | |
+ +----------+---------------------------------------------+----------+-------+
+ | | ↑ | | |
+ | | |vsync_len | | |
+ | | ↓ | | |
+ +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+ display-timings {
+ default-timing = <&timing0>;
+ timing0: 1920p24 {
+ /* 1920x1080p24 */I think this is commonly called 1080p24.
quoted hunk ↗ jump to hunk
+ clock = <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-high; + }; + }; + +Every property also supports the use of ranges, so the commonly used datasheet +description with <min typ max>-tuples can be used. + +Example: + + timing1: timing { + /* 1920x1080p24 */ + clock = <148500000>; + hactive = <1920>; + vactive = <1080>; + hsync-len = <0 44 60>; + hfront-porch = <80 88 95>; + hback-porch = <100 148 160>; + vfront-porch = <0 4 6>; + vback-porch = <0 36 50>; + vsync-len = <0 5 6>; + }; + +Usage in backend +================ + +A backend driver may choose to use the display-timings directly and convert the timing +ranges to a suitable mode. Or it may just use the conversion of the display timings +to the required mode via the generic videomode struct. + + dtb + | + | of_get_display_timing_list + ↓ + struct display_timings + | + | videomode_from_timing + ↓ + --- struct videomode --- + | | + videomode_to_displaymode | | videomode_to_fb_videomode + ↓ ↓ + drm_display_mode fb_videomode + +The functions of_get_fb_videomode and of_get_display_mode are provided +to conveniently get the respective mode representation from the devicetree. + +Conversion to videomode +======================= + +As device drivers normally work with some kind of video mode, the timings can be +converted (may be just a simple copying of the typical value) to a generic videomode +structure which then can be converted to the according mode used by the backend. + +Supported modes +=============== + +The generic videomode read in by the driver can be converted to the following +modes with the following parameters + +struct fb_videomode +=================== + + +----------+---------------------------------------------+----------+-------+ + | | ↑ | | | + | | |upper_margin | | | + | | ↓ | | | + +----------###############################################----------+-------+ + | # ↑ # | | + | # | # | | + | # | # | | + | # | # | | + | left # | # right | hsync | + | margin # | xres # margin | len | + |<-------->#<---------------+--------------------------->#<-------->|<----->| + | # | # | | + | # | # | | + | # | # | | + | # |yres # | | + | # | # | | + | # | # | | + | # | # | | + | # | # | | + | # | # | | + | # | # | | + | # | # | | + | # | # | | + | # ↓ # | | + +----------###############################################----------+-------+ + | | ↑ | | | + | | |lower_margin | | | + | | ↓ | | | + +----------+---------------------------------------------+----------+-------+ + | | ↑ | | | + | | |vsync_len | | | + | | ↓ | | | + +----------+---------------------------------------------+----------+-------+ + +clock in nanoseconds + +struct drm_display_mode +======================= + + +----------+---------------------------------------------+----------+-------+ + | | | | | ↑ + | | | | | | + | | | | | | + +----------###############################################----------+-------+ | + | # ↑ ↑ ↑ # | | | + | # | | | # | | | + | # | | | # | | | + | # | | | # | | | + | # | | | # | | | + | # | | | hdisplay # | | | + | #<--+--------------------+-------------------># | | | + | # | | | # | | | + | # |vsync_start | # | | | + | # | | | # | | | + | # | |vsync_end | # | | | + | # | | |vdisplay # | | | + | # | | | # | | | + | # | | | # | | | + | # | | | # | | | vtotal + | # | | | # | | | + | # | | | hsync_start # | | | + | #<--+---------+----------+------------------------------>| | | + | # | | | # | | | + | # | | | hsync_end # | | | + | #<--+---------+----------+-------------------------------------->| | + | # | | ↓ # | | | + +----------####|#########|################################----------+-------+ | + | | | | | | | | + | | | | | | | | + | | ↓ | | | | | + +----------+-------------+-------------------------------+----------+-------+ | + | | | | | | | + | | | | | | | + | | ↓ | | | ↓ + +----------+---------------------------------------------+----------+-------+ + htotal + <-------------------------------------------------------------------------> + +clock in kilohertzdiff --git a/drivers/of/Kconfig b/drivers/of/Kconfig index dfba3e6..646deb0 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig@@ -83,4 +83,9 @@ config OF_MTD depends on MTD def_bool y +config OF_DISPLAY_TIMINGS + def_bool y + help + helper to parse display timings from the devicetree + endmenu # OFdiff --git a/drivers/of/Makefile b/drivers/of/Makefile index e027f44..c8e9603 100644 --- a/drivers/of/Makefile +++ b/drivers/of/Makefile@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o obj-$(CONFIG_OF_PCI) += of_pci.o obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o obj-$(CONFIG_OF_MTD) += of_mtd.o +obj-$(CONFIG_OF_DISPLAY_TIMINGS) += of_display_timings.odiff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c new file mode 100644 index 0000000..e47bc63 --- /dev/null +++ b/drivers/of/of_display_timings.c@@ -0,0 +1,183 @@ +/* + * OF helpers for parsing display timings + * + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix + * + * based on of_videomode.c by Sascha Hauer <s.hauer@pengutronix.de> + * + * This file is released under the GPLv2 + */ +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/export.h> +#include <linux/of_display_timings.h> + +/* every signal_timing can be specified with either + * just the typical value or a range consisting of + * min/typ/max. + * This function helps handling this + */
The comment is not according to kernel coding style. And I'd start the sentence with a capital letter =).
+static int parse_property(struct device_node *np, char *name,
+ struct timing_entry *result)
+{
+ struct property *prop;
+ int length;
+ int cells;
+ int ret;
+
+ prop = of_find_property(np, name, &length);
+ if (!prop) {
+ pr_err("%s: could not find property %s\n", __func__, name);
+ return -EINVAL;
+ }
+
+ cells = length / sizeof(u32);
+
+ if (cells == 1)
+ ret = of_property_read_u32_array(np, name, &result->typ, cells);
+ else if (cells == 3)
+ ret = of_property_read_u32_array(np, name, &result->min, cells);
+ else {
+ pr_err("%s: illegal timing specification in %s\n", __func__,
+ name);
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+struct signal_timing *of_get_display_timing(struct device_node *np)
+{
+ struct signal_timing *st;
+ int ret = 0;
+
+ st = kzalloc(sizeof(*st), GFP_KERNEL);
+
+ if (!st) {
+ pr_err("%s: could not allocate signal_timing struct\n", __func__);
+ return NULL;
+ }
+
+ ret |= parse_property(np, "hback-porch", &st->hback_porch);
+ ret |= parse_property(np, "hfront-porch", &st->hfront_porch);
+ ret |= parse_property(np, "hactive", &st->hactive);
+ ret |= parse_property(np, "hsync-len", &st->hsync_len);
+ ret |= parse_property(np, "vback-porch", &st->vback_porch);
+ ret |= parse_property(np, "vfront-porch", &st->vfront_porch);
+ ret |= parse_property(np, "vactive", &st->vactive);
+ ret |= parse_property(np, "vsync-len", &st->vsync_len);
+ ret |= parse_property(np, "clock", &st->pixelclock);
+
+ st->vsync_pol_active_high = of_property_read_bool(np, "vsync-active-high");
+ st->hsync_pol_active_high = of_property_read_bool(np, "hsync-active-high");
+ st->de_pol_active_high = of_property_read_bool(np, "de-active-high");
+ st->pixelclk_pol_inverted = of_property_read_bool(np, "pixelclk-inverted");
+ st->interlaced = of_property_read_bool(np, "interlaced");
+ st->doublescan = of_property_read_bool(np, "doublescan");
+
+ if (ret) {
+ pr_err("%s: error reading timing properties\n", __func__);
+ return NULL;
+ }
+
+ return st;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timing);
+
+struct display_timings *of_get_display_timing_list(struct device_node *np)
+{
+ struct device_node *timings_np;
+ struct device_node *entry;
+ struct display_timings *disp;
+ char *default_timing;
+
+ if (!np) {
+ pr_err("%s: no devicenode given\n", __func__);
+ return NULL;
+ }
+
+ timings_np = of_find_node_by_name(np, "display-timings");
+
+ if (!timings_np) {
+ pr_err("%s: could not find display-timings node\n", __func__);
+ return NULL;
+ }
+
+ disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+
+ entry = of_parse_phandle(timings_np, "default-timing", 0);
+
+ if (!entry) {
+ pr_info("%s: no default-timing specified\n", __func__);
+ entry = of_find_node_by_name(np, "timing");
+ }If "default-timing" property is optional, I don't see any need for the pr_info above, as it should be business as usual if the property doesn't exist. If the default-timing property doesn't exist, wouldn't it be simpler to get the first subnode, instead of looking one with "timing" name?
+
+ if (!entry) {
+ pr_info("%s: no timing specifications given\n", __func__);
+ return disp;
+ }Again, I don't think the pr_info is needed if this is a normal case. Then again, perhaps this could be an error? Why would there be a display node without any timings?
+
+ pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+ default_timing = (char *)entry->full_name;const char *?
+
+ disp->num_timings = 0;
+
+ for_each_child_of_node(timings_np, entry) {
+ disp->num_timings++;
+ }
No need for { }
+ disp->timings = kzalloc(sizeof(struct signal_timing *)*disp->num_timings,
+ GFP_KERNEL);
+
+ disp->num_timings = 0;
+
+ for_each_child_of_node(timings_np, entry) {
+ struct signal_timing *st;
+
+ st = of_get_display_timing(entry);
+
+ if (!st)
+ continue;
+
+ if (strcmp(default_timing, entry->full_name) == 0)
+ disp->default_timing = disp->num_timings;I don't see you setting disp->default_timing to OF_DEFAULT_TIMING in case there's no default_timing found. Or, at least I presume OF_DEFAULT_TIMING is meant to mark non-existing default timing. The name OF_DEFAULT_TIMING is not very descriptive to me. Would it make more sense to have the disp->default_timing as a pointer to the timing, instead of index? Then a NULL value would mark a non-existing default timing.
quoted hunk ↗ jump to hunk
+ disp->timings[disp->num_timings] = st; + disp->num_timings++; + } + + + of_node_put(timings_np); + + if (disp->num_timings >= 0) + pr_info("%s: got %d timings. Using timing #%d as default\n", __func__, + disp->num_timings , disp->default_timing + 1); + else + pr_info("%s: no timings specified\n", __func__); + + return disp; +} +EXPORT_SYMBOL_GPL(of_get_display_timing_list); + +int of_display_timings_exists(struct device_node *np) +{ + struct device_node *timings_np; + struct device_node *default_np; + + if (!np) + return -EINVAL; + + timings_np = of_parse_phandle(np, "display-timings", 0); + + if (!timings_np) + return -EINVAL; + + default_np = of_parse_phandle(np, "default-timing", 0); + + if (default_np) + return 0; + + return -EINVAL; +} +EXPORT_SYMBOL_GPL(of_display_timings_exists);diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h new file mode 100644 index 0000000..1ad719e --- /dev/null +++ b/include/linux/of_display_timings.h@@ -0,0 +1,85 @@ +/* + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de> + * + * description of display timings + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H +#define __LINUX_OF_DISPLAY_TIMINGS_H + +#define OF_DEFAULT_TIMING -1 + +struct display_timings { + unsigned int num_timings; + unsigned int default_timing; + + struct signal_timing **timings;
Should this be a pointer to a const array of const data? Is there ever need to change them after they've been read from DT?
+};
+
+struct timing_entry {
+ u32 min;
+ u32 typ;
+ u32 max;
+};
+
+struct signal_timing {
+ struct timing_entry pixelclock;
+
+ struct timing_entry hactive;
+ struct timing_entry hfront_porch;
+ struct timing_entry hback_porch;
+ struct timing_entry hsync_len;
+
+ struct timing_entry vactive;
+ struct timing_entry vfront_porch;
+ struct timing_entry vback_porch;
+ struct timing_entry vsync_len;
+
+ bool vsync_pol_active_high;
+ bool hsync_pol_active_high;
+ bool de_pol_active_high;
+ bool pixelclk_pol_inverted;
+ bool interlaced;
+ bool doublescan;
+};
+
+struct display_timings *of_get_display_timing_list(struct device_node *np);
+struct signal_timing *of_get_display_timing(struct device_node *np);
+int of_display_timings_exists(struct device_node *np);
+
+/* placeholder function until ranges are really needed */
+static inline u32 signal_timing_get_value(struct timing_entry *te, int index)
+{
+ return te->typ;
+}
+
+static inline void timings_release(struct display_timings *disp)
+{
+ int i;
+
+ for (i = 0; i < disp->num_timings; i++)
+ kfree(disp->timings[i]);
+}
+
+static inline void display_timings_release(struct display_timings *disp)
+{
+ timings_release(disp);
+ kfree(disp->timings);
+}
+
+static inline struct signal_timing *display_timings_get(struct display_timings *disp,
+ int index)
+{
+ struct signal_timing *st;
+
+ if (disp->num_timings > index) {
+ st = disp->timings[index];
+ return st;
+ }
+ else
+ return NULL;
+}Why do you have these functions in the header file? Tomi
Attachments
- signature.asc [application/pgp-signature] 836 bytes