Hi!
Finally, v7 of the series.
Changes since v6: - get rid of some empty lines etc. - move functions to their subsystems - split of_ from non-of_ functions - add at least some kerneldoc to some functions
Regards, Steffen
Steffen Trumtrar (8): video: add display_timing struct and helpers of: add helper to parse display timings of: add generic videomode description video: add videomode helpers fbmon: add videomode helpers fbmon: add of_videomode helpers drm_modes: add videomode helpers drm_modes: add of_videomode helpers
.../devicetree/bindings/video/display-timings.txt | 139 +++++++++++++++ drivers/gpu/drm/drm_modes.c | 78 +++++++++ drivers/of/Kconfig | 12 ++ drivers/of/Makefile | 2 + drivers/of/of_display_timings.c | 185 ++++++++++++++++++++ drivers/of/of_videomode.c | 47 +++++ drivers/video/Kconfig | 11 ++ drivers/video/Makefile | 2 + drivers/video/display_timing.c | 24 +++ drivers/video/fbmon.c | 76 ++++++++ drivers/video/videomode.c | 44 +++++ include/drm/drmP.h | 8 + include/linux/display_timing.h | 69 ++++++++ include/linux/fb.h | 5 + include/linux/of_display_timings.h | 20 +++ include/linux/of_videomode.h | 15 ++ include/linux/videomode.h | 36 ++++ 17 files changed, 773 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt create mode 100644 drivers/of/of_display_timings.c create mode 100644 drivers/of/of_videomode.c create mode 100644 drivers/video/display_timing.c create mode 100644 drivers/video/videomode.c create mode 100644 include/linux/display_timing.h create mode 100644 include/linux/of_display_timings.h create mode 100644 include/linux/of_videomode.h create mode 100644 include/linux/videomode.h
Add display_timing structure and the according helper functions. This allows the description of a display via its supported timing parameters.
Every timing parameter can be specified as a single value or a range <min typ max>.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/video/Kconfig | 5 +++ drivers/video/Makefile | 1 + drivers/video/display_timing.c | 24 ++++++++++++++ include/linux/display_timing.h | 69 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+) create mode 100644 drivers/video/display_timing.c create mode 100644 include/linux/display_timing.h
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index d08d799..1421fc8 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL This framework adds support for low-level control of the video output switch.
+config DISPLAY_TIMING + bool "Enable display timings helpers" + help + Say Y here, to use the display timing helpers. + menuconfig FB tristate "Support for frame buffer devices" ---help--- diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 23e948e..552c045 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -167,3 +167,4 @@ obj-$(CONFIG_FB_VIRTUAL) += vfb.o
#video output switch sysfs driver obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c new file mode 100644 index 0000000..9ccfdb3 --- /dev/null +++ b/drivers/video/display_timing.c @@ -0,0 +1,24 @@ +/* + * generic display timing functions + * + * Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de, Pengutronix + * + * This file is released under the GPLv2 + */ + +#include <linux/slab.h> +#include <linux/display_timing.h> + +void timings_release(struct display_timings *disp) +{ + int i; + + for (i = 0; i < disp->num_timings; i++) + kfree(disp->timings[i]); +} + +void display_timings_release(struct display_timings *disp) +{ + timings_release(disp); + kfree(disp->timings); +} diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h new file mode 100644 index 0000000..aa02a12 --- /dev/null +++ b/include/linux/display_timing.h @@ -0,0 +1,69 @@ +/* + * Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de + * + * description of display timings + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_DISPLAY_TIMINGS_H +#define __LINUX_DISPLAY_TIMINGS_H + +#include <linux/types.h> + +struct timing_entry { + u32 min; + u32 typ; + u32 max; +}; + +struct display_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; + + unsigned int vsync_pol_active; + unsigned int hsync_pol_active; + unsigned int de_pol_active; + unsigned int pixelclk_pol; + bool interlaced; + bool doublescan; +}; + +struct display_timings { + unsigned int num_timings; + unsigned int native_mode; + + struct display_timing **timings; +}; + +/* placeholder function until ranges are really needed */ +static inline u32 display_timing_get_value(struct timing_entry *te, int index) +{ + return te->typ; +} + +static inline struct display_timing *display_timings_get(struct display_timings *disp, + int index) +{ + struct display_timing *dt; + + if (disp->num_timings > index) { + dt = disp->timings[index]; + return dt; + } else + return NULL; +} + +void timings_release(struct display_timings *disp); +void display_timings_release(struct display_timings *disp); + +#endif
Hi Steffen,
Thanks for the patch.
As we'll need a v8 anyway due to the comment on patch 5/8, here are a couple of other small comments.
On Wednesday 31 October 2012 10:28:01 Steffen Trumtrar wrote:
Add display_timing structure and the according helper functions. This allows the description of a display via its supported timing parameters.
Every timing parameter can be specified as a single value or a range <min typ max>.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de
drivers/video/Kconfig | 5 +++ drivers/video/Makefile | 1 + drivers/video/display_timing.c | 24 ++++++++++++++ include/linux/display_timing.h | 69 +++++++++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+) create mode 100644 drivers/video/display_timing.c create mode 100644 include/linux/display_timing.h
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index d08d799..1421fc8 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL This framework adds support for low-level control of the video output switch.
+config DISPLAY_TIMING
bool "Enable display timings helpers"
help
Say Y here, to use the display timing helpers.
menuconfig FB tristate "Support for frame buffer devices" ---help--- diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 23e948e..552c045 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -167,3 +167,4 @@ obj-$(CONFIG_FB_VIRTUAL) += vfb.o
#video output switch sysfs driver obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c new file mode 100644 index 0000000..9ccfdb3 --- /dev/null +++ b/drivers/video/display_timing.c @@ -0,0 +1,24 @@ +/*
- generic display timing functions
- Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de,
Pengutronix + *
- This file is released under the GPLv2
- */
+#include <linux/slab.h> +#include <linux/display_timing.h>
I try to keep #include's sorted alphabetically, but I won't push for that.
+void timings_release(struct display_timings *disp) +{
- int i;
- for (i = 0; i < disp->num_timings; i++)
disp->num_timings is an unsigned int, i should be an unsigned int as well to avoid signed vs. unsigned comparisons.
kfree(disp->timings[i]);
+}
+void display_timings_release(struct display_timings *disp) +{
- timings_release(disp);
- kfree(disp->timings);
+} diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h new file mode 100644 index 0000000..aa02a12 --- /dev/null +++ b/include/linux/display_timing.h @@ -0,0 +1,69 @@ +/*
- Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
- description of display timings
- This file is released under the GPLv2
- */
+#ifndef __LINUX_DISPLAY_TIMINGS_H +#define __LINUX_DISPLAY_TIMINGS_H
+#include <linux/types.h>
+struct timing_entry {
- u32 min;
- u32 typ;
- u32 max;
+};
+struct display_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;
- unsigned int vsync_pol_active;
- unsigned int hsync_pol_active;
- unsigned int de_pol_active;
- unsigned int pixelclk_pol;
- bool interlaced;
- bool doublescan;
+};
+struct display_timings {
- unsigned int num_timings;
- unsigned int native_mode;
- struct display_timing **timings;
+};
+/* placeholder function until ranges are really needed */ +static inline u32 display_timing_get_value(struct timing_entry *te, int index)
What is the index parameter for ?
+{
- return te->typ;
+}
+static inline struct display_timing *display_timings_get(struct display_timings *disp,
int index)
+{
- struct display_timing *dt;
- if (disp->num_timings > index) {
index should be an unsigned int for the same reason as above.
dt = disp->timings[index];
return dt;
Maybe just
return disp->timings[index];
?
- } else
return NULL;
+}
+void timings_release(struct display_timings *disp); +void display_timings_release(struct display_timings *disp);
+#endif
Hi Steffen,
One more comment.
On Wednesday 31 October 2012 10:28:01 Steffen Trumtrar wrote:
Add display_timing structure and the according helper functions. This allows the description of a display via its supported timing parameters.
Every timing parameter can be specified as a single value or a range <min typ max>.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de
drivers/video/Kconfig | 5 +++ drivers/video/Makefile | 1 + drivers/video/display_timing.c | 24 ++++++++++++++ include/linux/display_timing.h | 69 +++++++++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+) create mode 100644 drivers/video/display_timing.c create mode 100644 include/linux/display_timing.h
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index d08d799..1421fc8 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -33,6 +33,11 @@ config VIDEO_OUTPUT_CONTROL This framework adds support for low-level control of the video output switch.
+config DISPLAY_TIMING
bool "Enable display timings helpers"
help
Say Y here, to use the display timing helpers.
menuconfig FB tristate "Support for frame buffer devices" ---help--- diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 23e948e..552c045 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -167,3 +167,4 @@ obj-$(CONFIG_FB_VIRTUAL) += vfb.o
#video output switch sysfs driver obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o +obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c new file mode 100644 index 0000000..9ccfdb3 --- /dev/null +++ b/drivers/video/display_timing.c @@ -0,0 +1,24 @@ +/*
- generic display timing functions
- Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de,
Pengutronix + *
- This file is released under the GPLv2
- */
+#include <linux/slab.h> +#include <linux/display_timing.h>
+void timings_release(struct display_timings *disp) +{
- int i;
- for (i = 0; i < disp->num_timings; i++)
kfree(disp->timings[i]);
+}
This function doesn't seem to be called externally, you can make it static.
+void display_timings_release(struct display_timings *disp) +{
- timings_release(disp);
- kfree(disp->timings);
+} diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h new file mode 100644 index 0000000..aa02a12 --- /dev/null +++ b/include/linux/display_timing.h @@ -0,0 +1,69 @@ +/*
- Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
- description of display timings
- This file is released under the GPLv2
- */
+#ifndef __LINUX_DISPLAY_TIMINGS_H +#define __LINUX_DISPLAY_TIMINGS_H
+#include <linux/types.h>
+struct timing_entry {
- u32 min;
- u32 typ;
- u32 max;
+};
+struct display_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;
- unsigned int vsync_pol_active;
- unsigned int hsync_pol_active;
- unsigned int de_pol_active;
- unsigned int pixelclk_pol;
- bool interlaced;
- bool doublescan;
+};
+struct display_timings {
- unsigned int num_timings;
- unsigned int native_mode;
- struct display_timing **timings;
+};
+/* placeholder function until ranges are really needed */ +static inline u32 display_timing_get_value(struct timing_entry *te, int index) +{
- return te->typ;
+}
+static inline struct display_timing *display_timings_get(struct display_timings *disp, + int index) +{
- struct display_timing *dt;
- if (disp->num_timings > index) {
dt = disp->timings[index];
return dt;
- } else
return NULL;
+} +void timings_release(struct display_timings *disp); +void display_timings_release(struct display_timings *disp);
+#endif
On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote: [...]
+void timings_release(struct display_timings *disp) +{
- int i;
- for (i = 0; i < disp->num_timings; i++)
kfree(disp->timings[i]);
+}
+void display_timings_release(struct display_timings *disp) +{
- timings_release(disp);
- kfree(disp->timings);
+}
I'm not quite sure I understand how these are supposed to be used. The only use-case where a struct display_timings is dynamically allocated is for the OF helpers. In that case, wouldn't it be more useful to have a function that frees the complete structure, including the struct display_timings itself? Something like this, which has all of the above rolled into one:
void display_timings_free(struct display_timings *disp) { if (disp->timings) { unsigned int i;
for (i = 0; i < disp->num_timings; i++) kfree(disp->timings[i]); }
kfree(disp->timings); kfree(disp); }
Is there a use-case where a struct display_timings is not dynamically allocated? The only one I can think of is where it is defined as platform data, but in that case you don't want to be calling display_timing_release() on it anyway.
Thierry
On Thu, Nov 01, 2012 at 09:08:42PM +0100, Thierry Reding wrote:
On Wed, Oct 31, 2012 at 10:28:01AM +0100, Steffen Trumtrar wrote: [...]
+void timings_release(struct display_timings *disp) +{
- int i;
- for (i = 0; i < disp->num_timings; i++)
kfree(disp->timings[i]);
+}
+void display_timings_release(struct display_timings *disp) +{
- timings_release(disp);
- kfree(disp->timings);
+}
I'm not quite sure I understand how these are supposed to be used. The only use-case where a struct display_timings is dynamically allocated is for the OF helpers. In that case, wouldn't it be more useful to have a function that frees the complete structure, including the struct display_timings itself? Something like this, which has all of the above rolled into one:
void display_timings_free(struct display_timings *disp) { if (disp->timings) { unsigned int i;
for (i = 0; i < disp->num_timings; i++) kfree(disp->timings[i]); } kfree(disp->timings); kfree(disp);
}
Well, you are right. They can be rolled into one function. The extra function call is useless and as it seems confusing.
Regards, Steffen
devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de --- .../devicetree/bindings/video/display-timings.txt | 139 +++++++++++++++ drivers/of/Kconfig | 6 + drivers/of/Makefile | 1 + drivers/of/of_display_timings.c | 185 ++++++++++++++++++++ include/linux/of_display_timings.h | 20 +++ 5 files changed, 351 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.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..04c94a3 --- /dev/null +++ b/Documentation/devicetree/bindings/video/display-timings.txt @@ -0,0 +1,139 @@ +display-timings 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. + +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: displayclock 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 + - 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 + +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 +=================== + + +----------+---------------------------------------------+----------+-------+ + | | ↑ | | | + | | |vback_porch | | | + | | ↓ | | | + +----------###############################################----------+-------+ + | # ↑ # | | + | # | # | | + | hback # | # hfront | hsync | + | porch # | hactive # porch | len | + |<-------->#<---------------+--------------------------->#<-------->|<----->| + | # | # | | + | # |vactive # | | + | # | # | | + | # ↓ # | | + +----------###############################################----------+-------+ + | | ↑ | | | + | | |vfront_porch | | | + | | ↓ | | | + +----------+---------------------------------------------+----------+-------+ + | | ↑ | | | + | | |vsync_len | | | + | | ↓ | | | + +----------+---------------------------------------------+----------+-------+ + + +Example: + + display-timings { + native-mode = <&timing0>; + timing0: 1920p24 { + /* 1920x1080p24 */ + 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 = <1>; + }; + }; + +Every required 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. diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig index dfba3e6..781e773 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -83,4 +83,10 @@ config OF_MTD depends on MTD def_bool y
+config OF_DISPLAY_TIMINGS + def_bool y + depends on DISPLAY_TIMING + help + helper to parse display timings from the devicetree + endmenu # OF diff --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.o diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c new file mode 100644 index 0000000..388fe4c --- /dev/null +++ b/drivers/of/of_display_timings.c @@ -0,0 +1,185 @@ +/* + * 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> + +/** + * parse_property - parse timing_entry from device_node + * @np: device_node with the property + * @name: name of the property + * @result: will be set to the return value + * + * DESCRIPTION: + * Every display_timing can be specified with either just the typical value or + * a range consisting of min/typ/max. This function helps handling this + **/ +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); + result->min = result->typ; + result->max = result->typ; + } 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; +} + +/** + * of_get_display_timing - parse display_timing entry from device_node + * @np: device_node with the properties + **/ +struct display_timing *of_get_display_timing(struct device_node *np) +{ + struct display_timing *dt; + int ret = 0; + + dt = kzalloc(sizeof(*dt), GFP_KERNEL); + if (!dt) { + pr_err("%s: could not allocate display_timing struct\n", __func__); + return NULL; + } + + ret |= parse_property(np, "hback-porch", &dt->hback_porch); + ret |= parse_property(np, "hfront-porch", &dt->hfront_porch); + ret |= parse_property(np, "hactive", &dt->hactive); + ret |= parse_property(np, "hsync-len", &dt->hsync_len); + ret |= parse_property(np, "vback-porch", &dt->vback_porch); + ret |= parse_property(np, "vfront-porch", &dt->vfront_porch); + ret |= parse_property(np, "vactive", &dt->vactive); + ret |= parse_property(np, "vsync-len", &dt->vsync_len); + ret |= parse_property(np, "clock-frequency", &dt->pixelclock); + + of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active); + of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active); + of_property_read_u32(np, "de-active", &dt->de_pol_active); + of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol); + dt->interlaced = of_property_read_bool(np, "interlaced"); + dt->doublescan = of_property_read_bool(np, "doublescan"); + + if (ret) { + pr_err("%s: error reading timing properties\n", __func__); + return NULL; + } + + return dt; +} +EXPORT_SYMBOL_GPL(of_get_display_timing); + +/** + * of_get_display_timing_list - parse all display_timing entries from a device_node + * @np: device_node with the subnodes + **/ +struct display_timings *of_get_display_timing_list(struct device_node *np) +{ + struct device_node *timings_np; + struct device_node *entry; + struct device_node *native_mode; + struct display_timings *disp; + + 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, "native-mode", 0); + /* assume first child as native mode if none provided */ + if (!entry) + entry = of_get_next_child(np, NULL); + if (!entry) { + pr_err("%s: no timing specifications given\n", __func__); + return NULL; + } + + pr_info("%s: using %s as default timing\n", __func__, entry->name); + + native_mode = entry; + + disp->num_timings = of_get_child_count(timings_np); + disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings, + GFP_KERNEL); + disp->num_timings = 0; + disp->native_mode = 0; + + 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; + } + + if (native_mode == entry) + disp->native_mode = disp->num_timings; + + disp->timings[disp->num_timings] = dt; + 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->native_mode + 1); + else { + pr_err("%s: no valid timings specified\n", __func__); + return NULL; + } + return disp; +} +EXPORT_SYMBOL_GPL(of_get_display_timing_list); + +/** + * 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; + struct device_node *default_np; + + if (!np) + return -EINVAL; + + timings_np = of_parse_phandle(np, "display-timings", 0); + if (!timings_np) + return -EINVAL; + + 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..e4e1f22 --- /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> + +#define OF_USE_NATIVE_MODE -1 + +struct display_timings *of_get_display_timing_list(struct device_node *np); +struct display_timing *of_get_display_timing(struct device_node *np); +int of_display_timings_exists(struct device_node *np); + +#endif
On 10/31/2012 03:28 AM, Steffen Trumtrar wrote:
Patch description? The patch defines the DT binding as well, which isn't mentioned in the patch subject.
new file mode 100644 index 0000000..04c94a3 --- /dev/null +++ b/Documentation/devicetree/bindings/video/display-timings.txt
+Usage in backend +================
Everything before this point in the binding docs looks reasonable to me. Everything after this point is Linux-specific/internal implementation detail, and hence shouldn't be in the binding document.
I only read the DT binding.
On Wed, Oct 31, 2012 at 10:28:02AM +0100, Steffen Trumtrar wrote: [...]
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
[...]
@@ -0,0 +1,139 @@ +display-timings bindings +==================
+display-timings-node +------------
Maybe extend the underline to the length of the section and subsection titles respectively?
+struct display_timing +===================
Same here.
+config OF_DISPLAY_TIMINGS
- def_bool y
- depends on DISPLAY_TIMING
Maybe this should be called OF_DISPLAY_TIMING to match DISPLAY_TIMING, or rename DISPLAY_TIMING to DISPLAY_TIMINGS for the sake of consistency?
+/**
- of_get_display_timing_list - parse all display_timing entries from a device_node
- @np: device_node with the subnodes
- **/
+struct display_timings *of_get_display_timing_list(struct device_node *np)
Perhaps this would better be named of_get_display_timings() to match the return type?
- disp = kzalloc(sizeof(*disp), GFP_KERNEL);
Shouldn't you be checking this for allocation failures?
- disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
GFP_KERNEL);
Same here.
Thierry
On Thu, Nov 01, 2012 at 09:15:10PM +0100, Thierry Reding wrote:
On Wed, Oct 31, 2012 at 10:28:02AM +0100, Steffen Trumtrar wrote: [...]
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
[...]
@@ -0,0 +1,139 @@ +display-timings bindings +==================
+display-timings-node +------------
Maybe extend the underline to the length of the section and subsection titles respectively?
+struct display_timing +===================
Same here.
+config OF_DISPLAY_TIMINGS
- def_bool y
- depends on DISPLAY_TIMING
Maybe this should be called OF_DISPLAY_TIMING to match DISPLAY_TIMING, or rename DISPLAY_TIMING to DISPLAY_TIMINGS for the sake of consistency?
Yes, to all three above.
+/**
- of_get_display_timing_list - parse all display_timing entries from a device_node
- @np: device_node with the subnodes
- **/
+struct display_timings *of_get_display_timing_list(struct device_node *np)
Perhaps this would better be named of_get_display_timings() to match the return type?
Hm, I'm not really sure about that. I found it to error prone, to have a function of_get_display_timing and of_get_display_timings. That's why I chose of_get_display_timing_list. But you are correct, that it doesn't match the return value. Maybe I should just make the first function static and change the name as you suggested.
- disp = kzalloc(sizeof(*disp), GFP_KERNEL);
Shouldn't you be checking this for allocation failures?
- disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
GFP_KERNEL);
Same here.
Yes, to both.
Regards, Steffen
Hello Steffen,
On Wed, Oct 31, 2012 at 2:58 PM, Steffen Trumtrar s.trumtrar@pengutronix.de wrote:
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de
.../devicetree/bindings/video/display-timings.txt | 139 +++++++++++++++ drivers/of/Kconfig | 6 + drivers/of/Makefile | 1 + drivers/of/of_display_timings.c | 185 ++++++++++++++++++++ include/linux/of_display_timings.h | 20 +++ 5 files changed, 351 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.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..04c94a3 --- /dev/null +++ b/Documentation/devicetree/bindings/video/display-timings.txt @@ -0,0 +1,139 @@ +display-timings 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.
+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: displayclock 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
- 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
+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 +===================
- +----------+---------------------------------------------+----------+-------+
- | | ↑ | | |
- | | |vback_porch | | |
- | | ↓ | | |
- +----------###############################################----------+-------+
- | # ↑ # | |
- | # | # | |
- | hback # | # hfront | hsync |
- | porch # | hactive # porch | len |
- |<-------->#<---------------+--------------------------->#<-------->|<----->|
- | # | # | |
- | # |vactive # | |
- | # | # | |
- | # ↓ # | |
- +----------###############################################----------+-------+
- | | ↑ | | |
- | | |vfront_porch | | |
- | | ↓ | | |
- +----------+---------------------------------------------+----------+-------+
- | | ↑ | | |
- | | |vsync_len | | |
- | | ↓ | | |
- +----------+---------------------------------------------+----------+-------+
+Example:
display-timings {
native-mode = <&timing0>;
timing0: 1920p24 {
/* 1920x1080p24 */
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 = <1>;
};
};
+Every required 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. diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig index dfba3e6..781e773 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -83,4 +83,10 @@ config OF_MTD depends on MTD def_bool y
+config OF_DISPLAY_TIMINGS
def_bool y
depends on DISPLAY_TIMING
help
helper to parse display timings from the devicetree
endmenu # OF diff --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.o diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c new file mode 100644 index 0000000..388fe4c --- /dev/null +++ b/drivers/of/of_display_timings.c @@ -0,0 +1,185 @@ +/*
- 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>
+/**
- parse_property - parse timing_entry from device_node
- @np: device_node with the property
- @name: name of the property
- @result: will be set to the return value
- DESCRIPTION:
- Every display_timing can be specified with either just the typical value or
- a range consisting of min/typ/max. This function helps handling this
- **/
+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);
As you are reading only one vaue, you can use of_property_read_u32 instead.
result->min = result->typ;
result->max = result->typ;
} else if (cells == 3) {
ret = of_property_read_u32_array(np, name, &result->min, cells);
You are considering only min element, what about typ and max elements?
} else {
pr_err("%s: illegal timing specification in %s\n", __func__, name);
return -EINVAL;
}
return ret;
+}
+/**
- of_get_display_timing - parse display_timing entry from device_node
- @np: device_node with the properties
- **/
+struct display_timing *of_get_display_timing(struct device_node *np) +{
struct display_timing *dt;
int ret = 0;
dt = kzalloc(sizeof(*dt), GFP_KERNEL);
if (!dt) {
pr_err("%s: could not allocate display_timing struct\n", __func__);
return NULL;
}
ret |= parse_property(np, "hback-porch", &dt->hback_porch);
ret |= parse_property(np, "hfront-porch", &dt->hfront_porch);
ret |= parse_property(np, "hactive", &dt->hactive);
ret |= parse_property(np, "hsync-len", &dt->hsync_len);
ret |= parse_property(np, "vback-porch", &dt->vback_porch);
ret |= parse_property(np, "vfront-porch", &dt->vfront_porch);
ret |= parse_property(np, "vactive", &dt->vactive);
ret |= parse_property(np, "vsync-len", &dt->vsync_len);
ret |= parse_property(np, "clock-frequency", &dt->pixelclock);
of_property_read_u32(np, "vsync-active", &dt->vsync_pol_active);
of_property_read_u32(np, "hsync-active", &dt->hsync_pol_active);
of_property_read_u32(np, "de-active", &dt->de_pol_active);
of_property_read_u32(np, "pixelclk-inverted", &dt->pixelclk_pol);
dt->interlaced = of_property_read_bool(np, "interlaced");
dt->doublescan = of_property_read_bool(np, "doublescan");
if (ret) {
pr_err("%s: error reading timing properties\n", __func__);
return NULL;
}
return dt;
+} +EXPORT_SYMBOL_GPL(of_get_display_timing);
+/**
- of_get_display_timing_list - parse all display_timing entries from a device_node
- @np: device_node with the subnodes
- **/
+struct display_timings *of_get_display_timing_list(struct device_node *np) +{
struct device_node *timings_np;
struct device_node *entry;
struct device_node *native_mode;
struct display_timings *disp;
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, "native-mode", 0);
/* assume first child as native mode if none provided */
if (!entry)
entry = of_get_next_child(np, NULL);
if (!entry) {
pr_err("%s: no timing specifications given\n", __func__);
return NULL;
}
pr_info("%s: using %s as default timing\n", __func__, entry->name);
native_mode = entry;
disp->num_timings = of_get_child_count(timings_np);
disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
GFP_KERNEL);
disp->num_timings = 0;
disp->native_mode = 0;
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;
}
if (native_mode == entry)
disp->native_mode = disp->num_timings;
disp->timings[disp->num_timings] = dt;
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->native_mode + 1);
else {
pr_err("%s: no valid timings specified\n", __func__);
return NULL;
}
return disp;
+} +EXPORT_SYMBOL_GPL(of_get_display_timing_list);
+/**
- 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;
struct device_node *default_np;
if (!np)
return -EINVAL;
timings_np = of_parse_phandle(np, "display-timings", 0);
if (!timings_np)
return -EINVAL;
return -EINVAL;
Here it should return success instead of -EINVAL.
And one query.. are the binding properties names and "display-timings" node structure template finalized..?
Best Wishes, Leela Krishna Amudala.
+} +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..e4e1f22 --- /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>
+#define OF_USE_NATIVE_MODE -1
+struct display_timings *of_get_display_timing_list(struct device_node *np); +struct display_timing *of_get_display_timing(struct device_node *np); +int of_display_timings_exists(struct device_node *np);
+#endif
1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi!
On Fri, Nov 02, 2012 at 10:49:47PM +0530, Leela Krishna Amudala wrote:
Hello Steffen,
On Wed, Oct 31, 2012 at 2:58 PM, Steffen Trumtrar
+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);
As you are reading only one vaue, you can use of_property_read_u32 instead.
Yes, thats copypasta, no need for _array here.
result->min = result->typ;
result->max = result->typ;
} else if (cells == 3) {
ret = of_property_read_u32_array(np, name, &result->min, cells);
You are considering only min element, what about typ and max elements?
I start at the address of result->min and read three u32-values, therefore all three (min,typ,max) are filled with values.
+/**
- 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;
struct device_node *default_np;
if (!np)
return -EINVAL;
timings_np = of_parse_phandle(np, "display-timings", 0);
if (!timings_np)
return -EINVAL;
return -EINVAL;
Here it should return success instead of -EINVAL.
Yes.
And one query.. are the binding properties names and "display-timings" node structure template finalized..?
I sure hope so. There actually is one error in the examples though. The property clock is called clock-frequency. I included it correctly at the top of display-timings.txt, but overlooked it in the examples.
Regards, Steffen
Get videomode from devicetree in a format appropriate for the backend. drm_display_mode and fb_videomode are supported atm. Uses the display signal timings from of_display_timings
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/of/Kconfig | 6 ++++++ drivers/of/Makefile | 1 + drivers/of/of_videomode.c | 47 ++++++++++++++++++++++++++++++++++++++++++ include/linux/of_videomode.h | 15 ++++++++++++++ 4 files changed, 69 insertions(+) create mode 100644 drivers/of/of_videomode.c create mode 100644 include/linux/of_videomode.h
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig index 781e773..0575ffe 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -89,4 +89,10 @@ config OF_DISPLAY_TIMINGS help helper to parse display timings from the devicetree
+config OF_VIDEOMODE + def_bool y + depends on VIDEOMODE + help + helper to get videomodes from the devicetree + endmenu # OF diff --git a/drivers/of/Makefile b/drivers/of/Makefile index c8e9603..09d556f 100644 --- a/drivers/of/Makefile +++ b/drivers/of/Makefile @@ -12,3 +12,4 @@ 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.o +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c new file mode 100644 index 0000000..91a26fc --- /dev/null +++ b/drivers/of/of_videomode.c @@ -0,0 +1,47 @@ +/* + * generic videomode helper + * + * Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de, Pengutronix + * + * This file is released under the GPLv2 + */ +#include <linux/of.h> +#include <linux/of_display_timings.h> +#include <linux/of_videomode.h> +#include <linux/export.h> + +/** + * of_get_videomode - get the videomode #<index> from devicetree + * @np - devicenode with the display_timings + * @vm - set to return value + * @index - index into list of display_timings + * DESCRIPTION: + * Get a list of all display timings and put the one + * specified by index into *vm. This function should only be used, if + * only one videomode is to be retrieved. A driver that needs to work + * with multiple/all videomodes should work with + * of_get_display_timing_list instead. + **/ +int of_get_videomode(struct device_node *np, struct videomode *vm, int index) +{ + struct display_timings *disp; + int ret; + + disp = of_get_display_timing_list(np); + if (!disp) { + pr_err("%s: no timings specified\n", __func__); + return -EINVAL; + } + + if (index == OF_USE_NATIVE_MODE) + index = disp->native_mode; + + ret = videomode_from_timing(disp, vm, index); + if (ret) + return ret; + + display_timings_release(disp); + + return 0; +} +EXPORT_SYMBOL_GPL(of_get_videomode); diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h new file mode 100644 index 0000000..01518e6 --- /dev/null +++ b/include/linux/of_videomode.h @@ -0,0 +1,15 @@ +/* + * 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> + +int of_get_videomode(struct device_node *np, struct videomode *vm, int index); +#endif /* __LINUX_OF_VIDEOMODE_H */
On Wed, Oct 31, 2012 at 10:28:03AM +0100, Steffen Trumtrar wrote: [...]
+config OF_VIDEOMODE
- def_bool y
- depends on VIDEOMODE
- help
helper to get videomodes from the devicetree
I think patches 3 and 4 need to be swapped, since patch 4 introduces the VIDEOMODE Kconfig symbol (as well as the videomode.h helper) that patch 3 uses.
Thierry
Add helper functions to convert from display timings to a generic videomode structure. This videomode can then be converted to the corresponding subsystem mode representation (e.g. fb_videomode).
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/video/Kconfig | 6 ++++++ drivers/video/Makefile | 1 + drivers/video/videomode.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/videomode.h | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+) create mode 100644 drivers/video/videomode.c create mode 100644 include/linux/videomode.h
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 1421fc8..45dd393 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -38,6 +38,12 @@ config DISPLAY_TIMING help Say Y here, to use the display timing helpers.
+config VIDEOMODE + bool "Enable videomode helpers" + help + Say Y here, to use the generic videomode helpers. This allows + converting from display timings to fb_videomode and drm_display_mode + menuconfig FB tristate "Support for frame buffer devices" ---help--- diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 552c045..fc30439 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -168,3 +168,4 @@ obj-$(CONFIG_FB_VIRTUAL) += vfb.o #video output switch sysfs driver obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o +obj-$(CONFIG_VIDEOMODE) += videomode.o diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c new file mode 100644 index 0000000..a9fe010 --- /dev/null +++ b/drivers/video/videomode.c @@ -0,0 +1,44 @@ +/* + * generic display timing functions + * + * Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de, Pengutronix + * + * This file is released under the GPLv2 + */ + +#include <linux/kernel.h> +#include <linux/export.h> +#include <linux/errno.h> +#include <linux/display_timing.h> +#include <linux/videomode.h> + +int videomode_from_timing(struct display_timings *disp, struct videomode *vm, + int index) +{ + struct display_timing *dt = NULL; + + dt = display_timings_get(disp, index); + if (!dt) { + pr_err("%s: no signal timings found\n", __func__); + return -EINVAL; + } + + vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0); + vm->hactive = display_timing_get_value(&dt->hactive, 0); + vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0); + vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0); + vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0); + + vm->vactive = display_timing_get_value(&dt->vactive, 0); + vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0); + vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0); + vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0); + + vm->vah = dt->vsync_pol_active; + vm->hah = dt->hsync_pol_active; + vm->interlaced = dt->interlaced; + vm->doublescan = dt->doublescan; + + return 0; +} +EXPORT_SYMBOL_GPL(videomode_from_timing); diff --git a/include/linux/videomode.h b/include/linux/videomode.h new file mode 100644 index 0000000..f932147 --- /dev/null +++ b/include/linux/videomode.h @@ -0,0 +1,36 @@ +/* + * Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de + * + * generic videomode description + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_VIDEOMODE_H +#define __LINUX_VIDEOMODE_H + +#include <linux/display_timing.h> + +struct videomode { + u32 pixelclock; + u32 refreshrate; + + u32 hactive; + u32 hfront_porch; + u32 hback_porch; + u32 hsync_len; + + u32 vactive; + u32 vfront_porch; + u32 vback_porch; + u32 vsync_len; + + u32 hah; + u32 vah; + bool interlaced; + bool doublescan; +}; + +int videomode_from_timing(struct display_timings *disp, struct videomode *vm, + int index); +#endif
Hi Steffen,
Thanks for the patch.
On Wednesday 31 October 2012 10:28:04 Steffen Trumtrar wrote:
Add helper functions to convert from display timings to a generic videomode structure. This videomode can then be converted to the corresponding subsystem mode representation (e.g. fb_videomode).
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de
drivers/video/Kconfig | 6 ++++++ drivers/video/Makefile | 1 + drivers/video/videomode.c | 44 ++++++++++++++++++++++++++++++++++++++++++ include/linux/videomode.h | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+) create mode 100644 drivers/video/videomode.c create mode 100644 include/linux/videomode.h
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index 1421fc8..45dd393 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -38,6 +38,12 @@ config DISPLAY_TIMING help Say Y here, to use the display timing helpers.
+config VIDEOMODE
bool "Enable videomode helpers"
Shouldn't this option should be automatically selected through a select statement in other options that depend on it instead of manually selected ? Same for the DISPLAY_TIMING option in 1/8.
There's so little code here, do you think it would be a good idea to merge patches 1/8 and 4/8 and have a single Kconfig option ?
help
Say Y here, to use the generic videomode helpers. This allows
converting from display timings to fb_videomode and drm_display_mode
menuconfig FB tristate "Support for frame buffer devices" ---help--- diff --git a/drivers/video/Makefile b/drivers/video/Makefile index 552c045..fc30439 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -168,3 +168,4 @@ obj-$(CONFIG_FB_VIRTUAL) += vfb.o #video output switch sysfs driver obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o +obj-$(CONFIG_VIDEOMODE) += videomode.o diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c new file mode 100644 index 0000000..a9fe010 --- /dev/null +++ b/drivers/video/videomode.c @@ -0,0 +1,44 @@ +/*
- generic display timing functions
- Copyright (c) 2012 Steffen Trumtrar s.trumtrar@pengutronix.de,
Pengutronix + *
- This file is released under the GPLv2
- */
+#include <linux/kernel.h> +#include <linux/export.h> +#include <linux/errno.h> +#include <linux/display_timing.h> +#include <linux/videomode.h>
As in 1/8, I try to keep #include's sorted alphabetically, but I won't push for it here either :-)
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
int index)
unsigned int for index ?
+{
- struct display_timing *dt = NULL;
No need to initialize dt to NULL.
- dt = display_timings_get(disp, index);
- if (!dt) {
pr_err("%s: no signal timings found\n", __func__);
I wonder whether this really deserves a pr_err() here. Would this be a caller bug, or can there be valid use cases where this function would return an error ?
return -EINVAL;
- }
- vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
- vm->hactive = display_timing_get_value(&dt->hactive, 0);
- vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
- vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
- vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
- vm->vactive = display_timing_get_value(&dt->vactive, 0);
- vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
- vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
- vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);
- vm->vah = dt->vsync_pol_active;
- vm->hah = dt->hsync_pol_active;
- vm->interlaced = dt->interlaced;
- vm->doublescan = dt->doublescan;
- return 0;
+} +EXPORT_SYMBOL_GPL(videomode_from_timing); diff --git a/include/linux/videomode.h b/include/linux/videomode.h new file mode 100644 index 0000000..f932147 --- /dev/null +++ b/include/linux/videomode.h @@ -0,0 +1,36 @@ +/*
- Copyright 2012 Steffen Trumtrar s.trumtrar@pengutronix.de
- generic videomode description
- This file is released under the GPLv2
- */
+#ifndef __LINUX_VIDEOMODE_H +#define __LINUX_VIDEOMODE_H
+#include <linux/display_timing.h>
+struct videomode {
- u32 pixelclock;
- u32 refreshrate;
- u32 hactive;
- u32 hfront_porch;
- u32 hback_porch;
- u32 hsync_len;
- u32 vactive;
- u32 vfront_porch;
- u32 vback_porch;
- u32 vsync_len;
- u32 hah;
- u32 vah;
- bool interlaced;
- bool doublescan;
+};
+int videomode_from_timing(struct display_timings *disp, struct videomode *vm,
int index);
+#endif
Add a function to convert from the generic videomode to a fb_videomode.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/video/fbmon.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/fb.h | 2 ++ 2 files changed, 38 insertions(+)
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index cef6557..b9e6ab3 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -1373,6 +1373,42 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf kfree(timings); return err; } + +#if IS_ENABLED(CONFIG_VIDEOMODE) +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode) +{ + fbmode->xres = vm->hactive; + fbmode->left_margin = vm->hback_porch; + fbmode->right_margin = vm->hfront_porch; + fbmode->hsync_len = vm->hsync_len; + + fbmode->yres = vm->vactive; + fbmode->upper_margin = vm->vback_porch; + fbmode->lower_margin = vm->vfront_porch; + fbmode->vsync_len = vm->vsync_len; + + fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000); + + fbmode->sync = 0; + fbmode->vmode = 0; + if (vm->hah) + fbmode->sync |= FB_SYNC_HOR_HIGH_ACT; + if (vm->vah) + fbmode->sync |= FB_SYNC_VERT_HIGH_ACT; + if (vm->interlaced) + fbmode->vmode |= FB_VMODE_INTERLACED; + if (vm->doublescan) + fbmode->vmode |= FB_VMODE_DOUBLE; + + fbmode->refresh = 60; + fbmode->flag = 0; + + return 0; +} +EXPORT_SYMBOL_GPL(videomode_to_fb_videomode); +#endif + + #else int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var) { diff --git a/include/linux/fb.h b/include/linux/fb.h index c7a9571..46c665b 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -714,6 +714,8 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb); extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
+extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode); + /* drivers/video/modedb.c */ #define VESA_MODEDB_SIZE 34 extern void fb_var_to_videomode(struct fb_videomode *mode,
Hi Steffen,
On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
Add a function to convert from the generic videomode to a fb_videomode.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de
drivers/video/fbmon.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/fb.h | 2 ++ 2 files changed, 38 insertions(+)
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index cef6557..b9e6ab3 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -1373,6 +1373,42 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf kfree(timings); return err; }
+#if IS_ENABLED(CONFIG_VIDEOMODE) +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode) +{
- fbmode->xres = vm->hactive;
- fbmode->left_margin = vm->hback_porch;
- fbmode->right_margin = vm->hfront_porch;
- fbmode->hsync_len = vm->hsync_len;
- fbmode->yres = vm->vactive;
- fbmode->upper_margin = vm->vback_porch;
- fbmode->lower_margin = vm->vfront_porch;
- fbmode->vsync_len = vm->vsync_len;
- fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
- fbmode->sync = 0;
- fbmode->vmode = 0;
- if (vm->hah)
fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
- if (vm->vah)
fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
- if (vm->interlaced)
fbmode->vmode |= FB_VMODE_INTERLACED;
- if (vm->doublescan)
fbmode->vmode |= FB_VMODE_DOUBLE;
"pixelclk-inverted" property of the panel is not percolated fb_videomode. Please let me know if I am missing something.
Thanks, Prakash
- fbmode->refresh = 60;
- fbmode->flag = 0;
- return 0;
+} +EXPORT_SYMBOL_GPL(videomode_to_fb_videomode); +#endif
#else int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var) { diff --git a/include/linux/fb.h b/include/linux/fb.h index c7a9571..46c665b 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -714,6 +714,8 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb); extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
+extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
/* drivers/video/modedb.c */ #define VESA_MODEDB_SIZE 34 extern void fb_var_to_videomode(struct fb_videomode *mode, -- 1.7.10.4
-- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Prakash!
On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
Hi Steffen,
On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
Add a function to convert from the generic videomode to a fb_videomode.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de
drivers/video/fbmon.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/fb.h | 2 ++ 2 files changed, 38 insertions(+)
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index cef6557..b9e6ab3 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -1373,6 +1373,42 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf kfree(timings); return err; }
+#if IS_ENABLED(CONFIG_VIDEOMODE) +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode) +{
- fbmode->xres = vm->hactive;
- fbmode->left_margin = vm->hback_porch;
- fbmode->right_margin = vm->hfront_porch;
- fbmode->hsync_len = vm->hsync_len;
- fbmode->yres = vm->vactive;
- fbmode->upper_margin = vm->vback_porch;
- fbmode->lower_margin = vm->vfront_porch;
- fbmode->vsync_len = vm->vsync_len;
- fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
- fbmode->sync = 0;
- fbmode->vmode = 0;
- if (vm->hah)
fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
- if (vm->vah)
fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
- if (vm->interlaced)
fbmode->vmode |= FB_VMODE_INTERLACED;
- if (vm->doublescan)
fbmode->vmode |= FB_VMODE_DOUBLE;
"pixelclk-inverted" property of the panel is not percolated fb_videomode. Please let me know if I am missing something.
You are right. I forgot that :(
Regards, Steffen
Hi!
On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
Hi Steffen,
On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
+#if IS_ENABLED(CONFIG_VIDEOMODE) +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode) +{
- fbmode->xres = vm->hactive;
- fbmode->left_margin = vm->hback_porch;
- fbmode->right_margin = vm->hfront_porch;
- fbmode->hsync_len = vm->hsync_len;
- fbmode->yres = vm->vactive;
- fbmode->upper_margin = vm->vback_porch;
- fbmode->lower_margin = vm->vfront_porch;
- fbmode->vsync_len = vm->vsync_len;
- fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
- fbmode->sync = 0;
- fbmode->vmode = 0;
- if (vm->hah)
fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
- if (vm->vah)
fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
- if (vm->interlaced)
fbmode->vmode |= FB_VMODE_INTERLACED;
- if (vm->doublescan)
fbmode->vmode |= FB_VMODE_DOUBLE;
"pixelclk-inverted" property of the panel is not percolated fb_videomode. Please let me know if I am missing something.
The next version is almost finished. Only thing I'm missing is this. And I actually do not know which flag would represent an inverted pixelclock in fb_videomode. Does anybody have any idea what I have to do here?
if (vm->pixelclk_pol) fbmode->sync = ???
That's as far as I have come and I don't see a flag that seems right. Is this even a valid property of fb_videomode?
Regards, Steffen
Hi Steffen,
On Fri, Nov 09, 2012 at 02:55:45, Steffen Trumtrar wrote:
Hi!
On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
Hi Steffen,
On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
+#if IS_ENABLED(CONFIG_VIDEOMODE) +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode) +{
- fbmode->xres = vm->hactive;
- fbmode->left_margin = vm->hback_porch;
- fbmode->right_margin = vm->hfront_porch;
- fbmode->hsync_len = vm->hsync_len;
- fbmode->yres = vm->vactive;
- fbmode->upper_margin = vm->vback_porch;
- fbmode->lower_margin = vm->vfront_porch;
- fbmode->vsync_len = vm->vsync_len;
- fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
- fbmode->sync = 0;
- fbmode->vmode = 0;
- if (vm->hah)
fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
- if (vm->vah)
fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
- if (vm->interlaced)
fbmode->vmode |= FB_VMODE_INTERLACED;
- if (vm->doublescan)
fbmode->vmode |= FB_VMODE_DOUBLE;
"pixelclk-inverted" property of the panel is not percolated fb_videomode. Please let me know if I am missing something.
The next version is almost finished. Only thing I'm missing is this. And I actually do not know which flag would represent an inverted pixelclock in fb_videomode. Does anybody have any idea what I have to do here?
if (vm->pixelclk_pol) fbmode->sync = ???
That's as far as I have come and I don't see a flag that seems right. Is this even a valid property of fb_videomode?
Thanks for considering it, I see IMX addresses it as proprietary FB_SYNC_ flag. FB_SYNC_CLK_INVERT: arch/arm/plat-mxc/include/mach/mx3fb.h
Thanks, Prakash
Hi!
On Fri, Nov 09, 2012 at 04:54:16PM +0000, Manjunathappa, Prakash wrote:
Hi Steffen,
On Fri, Nov 09, 2012 at 02:55:45, Steffen Trumtrar wrote:
Hi!
On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
Hi Steffen,
On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
+#if IS_ENABLED(CONFIG_VIDEOMODE) +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode) +{
- fbmode->xres = vm->hactive;
- fbmode->left_margin = vm->hback_porch;
- fbmode->right_margin = vm->hfront_porch;
- fbmode->hsync_len = vm->hsync_len;
- fbmode->yres = vm->vactive;
- fbmode->upper_margin = vm->vback_porch;
- fbmode->lower_margin = vm->vfront_porch;
- fbmode->vsync_len = vm->vsync_len;
- fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
- fbmode->sync = 0;
- fbmode->vmode = 0;
- if (vm->hah)
fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
- if (vm->vah)
fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
- if (vm->interlaced)
fbmode->vmode |= FB_VMODE_INTERLACED;
- if (vm->doublescan)
fbmode->vmode |= FB_VMODE_DOUBLE;
"pixelclk-inverted" property of the panel is not percolated fb_videomode. Please let me know if I am missing something.
The next version is almost finished. Only thing I'm missing is this. And I actually do not know which flag would represent an inverted pixelclock in fb_videomode. Does anybody have any idea what I have to do here?
if (vm->pixelclk_pol) fbmode->sync = ???
That's as far as I have come and I don't see a flag that seems right. Is this even a valid property of fb_videomode?
Thanks for considering it, I see IMX addresses it as proprietary FB_SYNC_ flag. FB_SYNC_CLK_INVERT: arch/arm/plat-mxc/include/mach/mx3fb.h
No problem. So, it seems this flag has to be set in some imx-specific videomode_to_fb_videomode function. It is included in the struct videomode, so that should be no problem. But it will not be part of this series.
Regards, Steffen
Hi Steffen,
On Friday 09 November 2012 20:31:33 Steffen Trumtrar wrote:
On Fri, Nov 09, 2012 at 04:54:16PM +0000, Manjunathappa, Prakash wrote:
On Fri, Nov 09, 2012 at 02:55:45, Steffen Trumtrar wrote:
On Wed, Oct 31, 2012 at 03:30:03PM +0000, Manjunathappa, Prakash wrote:
On Wed, Oct 31, 2012 at 14:58:05, Steffen Trumtrar wrote:
+#if IS_ENABLED(CONFIG_VIDEOMODE) +int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode) +{
- fbmode->xres = vm->hactive;
- fbmode->left_margin = vm->hback_porch;
- fbmode->right_margin = vm->hfront_porch;
- fbmode->hsync_len = vm->hsync_len;
- fbmode->yres = vm->vactive;
- fbmode->upper_margin = vm->vback_porch;
- fbmode->lower_margin = vm->vfront_porch;
- fbmode->vsync_len = vm->vsync_len;
- fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
- fbmode->sync = 0;
- fbmode->vmode = 0;
- if (vm->hah)
fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
- if (vm->vah)
fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
- if (vm->interlaced)
fbmode->vmode |= FB_VMODE_INTERLACED;
- if (vm->doublescan)
fbmode->vmode |= FB_VMODE_DOUBLE;
"pixelclk-inverted" property of the panel is not percolated fb_videomode. Please let me know if I am missing something.
The next version is almost finished. Only thing I'm missing is this. And I actually do not know which flag would represent an inverted pixelclock in fb_videomode. Does anybody have any idea what I have to do here?> > if (vm->pixelclk_pol)
fbmode->sync = ???
That's as far as I have come and I don't see a flag that seems right. Is this even a valid property of fb_videomode?
Thanks for considering it, I see IMX addresses it as proprietary FB_SYNC_ flag. FB_SYNC_CLK_INVERT: arch/arm/plat-mxc/include/mach/mx3fb.h
No problem. So, it seems this flag has to be set in some imx-specific videomode_to_fb_videomode function. It is included in the struct videomode, so that should be no problem. But it will not be part of this series.
A possible solution would be to standardize the FB_SYNC_CLK_INVERT flag in fbdev, but that's in my opinion out of scope of this patch series. iMX3 developers, feel free to send patches :-)
Add helper to get fb_videomode from devicetree.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/video/fbmon.c | 40 ++++++++++++++++++++++++++++++++++++++++ include/linux/fb.h | 3 +++ 2 files changed, 43 insertions(+)
diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c index b9e6ab3..871aa76 100644 --- a/drivers/video/fbmon.c +++ b/drivers/video/fbmon.c @@ -1408,6 +1408,46 @@ int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode) EXPORT_SYMBOL_GPL(videomode_to_fb_videomode); #endif
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE) +static void dump_fb_videomode(struct fb_videomode *m) +{ + pr_debug("fb_videomode = %ux%u@%uHz (%ukHz) %u %u %u %u %u %u %u %u %u\n", + m->xres, m->yres, m->refresh, m->pixclock, m->left_margin, + m->right_margin, m->upper_margin, m->lower_margin, + m->hsync_len, m->vsync_len, m->sync, m->vmode, m->flag); +} + +/** + * of_get_fb_videomode - get a fb_videomode from devicetree + * @np: device_node with the timing specification + * @fb: will be set to the return value + * @index: index into the list of display timings in devicetree + * + * DESCRIPTION: + * This function is expensive and should only be used, if only one mode is to be + * read from DT. To get multiple modes start with of_get_display_timing_list ond + * work with that instead. + */ +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, + int index) +{ + struct videomode vm; + int ret; + + ret = of_get_videomode(np, &vm, index); + if (ret) + return ret; + + videomode_to_fb_videomode(&vm, fb); + + pr_info("%s: got %dx%d display mode from %s\n", __func__, vm.hactive, + vm.vactive, np->name); + dump_fb_videomode(fb); + + return 0; +} +EXPORT_SYMBOL_GPL(of_get_fb_videomode); +#endif
#else int fb_parse_edid(unsigned char *edid, struct fb_var_screeninfo *var) diff --git a/include/linux/fb.h b/include/linux/fb.h index 46c665b..9892fd6 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -14,6 +14,8 @@ #include <linux/backlight.h> #include <linux/slab.h> #include <asm/io.h> +#include <linux/of.h> +#include <linux/of_videomode.h>
struct vm_area_struct; struct fb_info; @@ -714,6 +716,7 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb); extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
+extern int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fb, int index); extern int videomode_to_fb_videomode(struct videomode *vm, struct fb_videomode *fbmode);
/* drivers/video/modedb.c */
Add conversion from videomode to drm_display_mode
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/gpu/drm/drm_modes.c | 36 ++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 3 +++ 2 files changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 59450f3..049624d 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -35,6 +35,7 @@ #include <linux/export.h> #include <drm/drmP.h> #include <drm/drm_crtc.h> +#include <linux/videomode.h>
/** * drm_mode_debug_printmodeline - debug print a mode @@ -504,6 +505,41 @@ drm_gtf_mode(struct drm_device *dev, int hdisplay, int vdisplay, int vrefresh, } EXPORT_SYMBOL(drm_gtf_mode);
+#if IS_ENABLED(CONFIG_VIDEOMODE) +int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode) +{ + dmode->hdisplay = vm->hactive; + dmode->hsync_start = dmode->hdisplay + vm->hfront_porch; + dmode->hsync_end = dmode->hsync_start + vm->hsync_len; + dmode->htotal = dmode->hsync_end + vm->hback_porch; + + dmode->vdisplay = vm->vactive; + dmode->vsync_start = dmode->vdisplay + vm->vfront_porch; + dmode->vsync_end = dmode->vsync_start + vm->vsync_len; + dmode->vtotal = dmode->vsync_end + vm->vback_porch; + + dmode->clock = vm->pixelclock / 1000; + + dmode->flags = 0; + if (vm->hah) + dmode->flags |= DRM_MODE_FLAG_PHSYNC; + else + dmode->flags |= DRM_MODE_FLAG_NHSYNC; + if (vm->vah) + dmode->flags |= DRM_MODE_FLAG_PVSYNC; + else + dmode->flags |= DRM_MODE_FLAG_NVSYNC; + if (vm->interlaced) + dmode->flags |= DRM_MODE_FLAG_INTERLACE; + if (vm->doublescan) + dmode->flags |= DRM_MODE_FLAG_DBLSCAN; + drm_mode_set_name(dmode); + + return 0; +} +EXPORT_SYMBOL_GPL(videomode_to_display_mode); +#endif + /** * drm_mode_set_name - set the name on a mode * @mode: name will be set in this mode diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 3fd8280..e9fa1e3 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -56,6 +56,7 @@ #include <linux/cdev.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/videomode.h> #if defined(__alpha__) || defined(__powerpc__) #include <asm/pgtable.h> /* For pte_wrprotect */ #endif @@ -1454,6 +1455,8 @@ extern struct drm_display_mode * drm_mode_create_from_cmdline_mode(struct drm_device *dev, struct drm_cmdline_mode *cmd);
+extern int videomode_to_display_mode(struct videomode *vm, + struct drm_display_mode *dmode); /* Modesetting support */ extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc); extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
Add helper to get drm_display_mode from devicetree.
Signed-off-by: Steffen Trumtrar s.trumtrar@pengutronix.de --- drivers/gpu/drm/drm_modes.c | 42 ++++++++++++++++++++++++++++++++++++++++++ include/drm/drmP.h | 5 +++++ 2 files changed, 47 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 049624d..d51afe6 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -35,6 +35,7 @@ #include <linux/export.h> #include <drm/drmP.h> #include <drm/drm_crtc.h> +#include <linux/of.h> #include <linux/videomode.h>
/** @@ -540,6 +541,47 @@ int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmo EXPORT_SYMBOL_GPL(videomode_to_display_mode); #endif
+#if IS_ENABLED(CONFIG_OF_VIDEOMODE) +static void dump_drm_displaymode(struct drm_display_mode *m) +{ + pr_debug("drm_displaymode = %d %d %d %d %d %d %d %d %d\n", + m->hdisplay, m->hsync_start, m->hsync_end, m->htotal, + m->vdisplay, m->vsync_start, m->vsync_end, m->vtotal, + m->clock); +} + +/** + * of_get_drm_display_mode - get a drm_display_mode from devicetree + * @np: device_node with the timing specification + * @dmode: will be set to the return value + * @index: index into the list of display timings in devicetree + * + * DESCRIPTION: + * This function is expensive and should only be used, if only one mode is to be + * read from DT. To get multiple modes start with of_get_display_timing_list ond + * work with that instead. + */ +int of_get_drm_display_mode(struct device_node *np, struct drm_display_mode *dmode, + int index) +{ + struct videomode vm; + int ret; + + ret = of_get_videomode(np, &vm, index); + if (ret) + return ret; + + videomode_to_display_mode(&vm, dmode); + + pr_info("%s: got %dx%d display mode from %s\n", __func__, vm.hactive, + vm.vactive, np->name); + dump_drm_displaymode(dmode); + + return 0; + +} +EXPORT_SYMBOL_GPL(of_get_drm_display_mode); +#endif /** * drm_mode_set_name - set the name on a mode * @mode: name will be set in this mode diff --git a/include/drm/drmP.h b/include/drm/drmP.h index e9fa1e3..4f9c44e 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -56,6 +56,7 @@ #include <linux/cdev.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/of.h> #include <linux/videomode.h> #if defined(__alpha__) || defined(__powerpc__) #include <asm/pgtable.h> /* For pte_wrprotect */ @@ -1457,6 +1458,10 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
extern int videomode_to_display_mode(struct videomode *vm, struct drm_display_mode *dmode); +extern int of_get_drm_display_mode(struct device_node *np, + struct drm_display_mode *dmode, + int index); + /* Modesetting support */ extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc); extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
On Wed, Oct 31, 2012 at 10:28:08AM +0100, Steffen Trumtrar wrote: [...]
+/**
- of_get_drm_display_mode - get a drm_display_mode from devicetree
- @np: device_node with the timing specification
- @dmode: will be set to the return value
- @index: index into the list of display timings in devicetree
- DESCRIPTION:
I don't think this is necessary.
- This function is expensive and should only be used, if only one mode is to be
- read from DT. To get multiple modes start with of_get_display_timing_list ond
You probably meant "and" at the end of this line. Also I'm not even sure that we should be exposing this function, but rather provide a helper which automatically adds the parsed modes to a DRM connector object.
Thierry
On 10/31/2012 04:28 AM, Steffen Trumtrar wrote:
Hi!
Finally, v7 of the series.
Changes since v6:
- get rid of some empty lines etc.
- move functions to their subsystems
- split of_ from non-of_ functions
- add at least some kerneldoc to some functions
Regards, Steffen
Steffen Trumtrar (8): video: add display_timing struct and helpers of: add helper to parse display timings of: add generic videomode description video: add videomode helpers fbmon: add videomode helpers fbmon: add of_videomode helpers drm_modes: add videomode helpers drm_modes: add of_videomode helpers
.../devicetree/bindings/video/display-timings.txt | 139 +++++++++++++++ drivers/gpu/drm/drm_modes.c | 78 +++++++++ drivers/of/Kconfig | 12 ++ drivers/of/Makefile | 2 + drivers/of/of_display_timings.c | 185 ++++++++++++++++++++ drivers/of/of_videomode.c | 47 +++++
Not sure why you moved this, but please move this back to drivers/video. We're trying to move subsystem specific pieces out of drivers/of.
Rob
drivers/video/Kconfig | 11 ++ drivers/video/Makefile | 2 + drivers/video/display_timing.c | 24 +++ drivers/video/fbmon.c | 76 ++++++++ drivers/video/videomode.c | 44 +++++ include/drm/drmP.h | 8 + include/linux/display_timing.h | 69 ++++++++ include/linux/fb.h | 5 + include/linux/of_display_timings.h | 20 +++ include/linux/of_videomode.h | 15 ++ include/linux/videomode.h | 36 ++++ 17 files changed, 773 insertions(+) create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt create mode 100644 drivers/of/of_display_timings.c create mode 100644 drivers/of/of_videomode.c create mode 100644 drivers/video/display_timing.c create mode 100644 drivers/video/videomode.c create mode 100644 include/linux/display_timing.h create mode 100644 include/linux/of_display_timings.h create mode 100644 include/linux/of_videomode.h create mode 100644 include/linux/videomode.h
On Thu, Nov 08, 2012 at 03:35:47PM -0600, Rob Herring wrote:
On 10/31/2012 04:28 AM, Steffen Trumtrar wrote:
Hi!
Finally, v7 of the series.
Changes since v6:
- get rid of some empty lines etc.
- move functions to their subsystems
- split of_ from non-of_ functions
- add at least some kerneldoc to some functions
Regards, Steffen
Steffen Trumtrar (8): video: add display_timing struct and helpers of: add helper to parse display timings of: add generic videomode description video: add videomode helpers fbmon: add videomode helpers fbmon: add of_videomode helpers drm_modes: add videomode helpers drm_modes: add of_videomode helpers
.../devicetree/bindings/video/display-timings.txt | 139 +++++++++++++++ drivers/gpu/drm/drm_modes.c | 78 +++++++++ drivers/of/Kconfig | 12 ++ drivers/of/Makefile | 2 + drivers/of/of_display_timings.c | 185 ++++++++++++++++++++ drivers/of/of_videomode.c | 47 +++++
Not sure why you moved this, but please move this back to drivers/video. We're trying to move subsystem specific pieces out of drivers/of.
Rob
Hm, the of_xxx part always was in drivers/of, but I can move that. No problem.
Regards, Steffen
dri-devel@lists.freedesktop.org