Hi,
On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> Signed-off-by: Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> ---
> .../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.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..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.
> + 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 kilohertz
> diff --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 # 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..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@xxxxxxxxxxxxxx>, Pengutronix
> + *
> + * based on of_videomode.c by Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> + *
> + * 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.
> + 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@xxxxxxxxxxxxxx>
> + *
> + * 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
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
