diff mbox

[v8,2/6] video: add of helper for videomode

Message ID 1352734626-27412-3-git-send-email-s.trumtrar@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Steffen Trumtrar Nov. 12, 2012, 3:37 p.m. UTC
This adds support for reading display timings from DT or/and convert one of those
timings to a videomode.
The of_display_timing implementation supports multiple children where each
property can have up to 3 values. All children are read into an array, that
can be queried.
of_get_videomode converts exactly one of that timings to a struct videomode.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
 drivers/video/Kconfig                              |   13 ++
 drivers/video/Makefile                             |    2 +
 drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
 drivers/video/of_videomode.c                       |   47 +++++
 include/linux/of_display_timings.h                 |   19 ++
 include/linux/of_videomode.h                       |   15 ++
 7 files changed, 389 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
 create mode 100644 drivers/video/of_display_timing.c
 create mode 100644 drivers/video/of_videomode.c
 create mode 100644 include/linux/of_display_timings.h
 create mode 100644 include/linux/of_videomode.h

Comments

Sascha Hauer Nov. 12, 2012, 6:58 p.m. UTC | #1
Hi Steffen,

You lose memory in several places:

On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
> +static 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;

Here

> +	}
> +
> +	return dt;
> +}
> +
> +/**
> + * of_get_display_timings - parse all display_timing entries from a device_node
> + * @np: device_node with the subnodes
> + **/
> +struct display_timings *of_get_display_timings(struct device_node *np)
> +{
> +	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);
> +	if (!disp)
> +		return -ENOMEM;
> +
> +	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;

Here

> +	}
> +
> +	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);
> +	if (!disp->timings)
> +		return -ENOMEM;

Here

> +
> +	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;

Here

> +		}
> +
> +		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;

and here

Sascha
Alexey Klimov Nov. 12, 2012, 7 p.m. UTC | #2
Hello Steffen,

On Mon, Nov 12, 2012 at 7:37 PM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> This adds support for reading display timings from DT or/and convert one of those
> timings to a videomode.
> The of_display_timing implementation supports multiple children where each
> property can have up to 3 values. All children are read into an array, that
> can be queried.
> of_get_videomode converts exactly one of that timings to a struct videomode.
>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
>  drivers/video/Kconfig                              |   13 ++
>  drivers/video/Makefile                             |    2 +
>  drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
>  drivers/video/of_videomode.c                       |   47 +++++
>  include/linux/of_display_timings.h                 |   19 ++
>  include/linux/of_videomode.h                       |   15 ++
>  7 files changed, 389 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
>  create mode 100644 drivers/video/of_display_timing.c
>  create mode 100644 drivers/video/of_videomode.c
>  create mode 100644 include/linux/of_display_timings.h
>  create mode 100644 include/linux/of_videomode.h
>
> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> new file mode 100644
> index 0000000..e22e2fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> @@ -0,0 +1,107 @@
> +display-timings bindings
> +========================
> +
> +display-timings-node
> +--------------------
> +
> +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-frequency = <52000000>;
> +                       hactive = <1920>;
> +                       vactive = <1080>;
> +                       hfront-porch = <25>;
> +                       hback-porch = <25>;
> +                       hsync-len = <25>;
> +                       vback-porch = <2>;
> +                       vfront-porch = <2>;
> +                       vsync-len = <2>;
> +                       hsync-active = <1>;
> +               };
> +       };
> +
> +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-frequency = <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>;
> +       };
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 2a23b18..a324457 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -39,6 +39,19 @@ config DISPLAY_TIMING
>  config VIDEOMODE
>         bool
>
> +config OF_DISPLAY_TIMING
> +       def_bool y
> +       select DISPLAY_TIMING
> +       help
> +         helper to parse display timings from the devicetree
> +
> +config OF_VIDEOMODE
> +       def_bool y
> +       select VIDEOMODE
> +       select OF_DISPLAY_TIMING
> +       help
> +         helper to get videomodes from the devicetree
> +
>  menuconfig FB
>         tristate "Support for frame buffer devices"
>         ---help---
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index fc30439..b936b00 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -168,4 +168,6 @@ 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_OF_DISPLAY_TIMING) += of_display_timing.o
>  obj-$(CONFIG_VIDEOMODE) += videomode.o
> +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
> new file mode 100644
> index 0000000..0bd30cc
> --- /dev/null
> +++ b/drivers/video/of_display_timing.c
> @@ -0,0 +1,186 @@
> +/*
> + * 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, cells, 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(np, name, &result->typ);
> +               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
> + **/
> +static 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;
> +}
> +
> +/**
> + * of_get_display_timings - parse all display_timing entries from a device_node
> + * @np: device_node with the subnodes
> + **/
> +struct display_timings *of_get_display_timings(struct device_node *np)
> +{
> +       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);
> +       if (!disp)
> +               return -ENOMEM;
> +
> +       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);
> +       if (!disp->timings)
> +               return -ENOMEM;

Could you please check return values here ^^^ and above "disp =
kzalloc(sizeof(*disp), GFP_KERNEL);" ?
May be it's better to return NULL instead of -ENOMEM and put error message?
Stephen Warren Nov. 12, 2012, 8:40 p.m. UTC | #3
On 11/12/2012 08:37 AM, Steffen Trumtrar wrote:
> This adds support for reading display timings from DT or/and convert one of those
> timings to a videomode.
> The of_display_timing implementation supports multiple children where each
> property can have up to 3 values. All children are read into an array, that
> can be queried.
> of_get_videomode converts exactly one of that timings to a struct videomode.

> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt

The device tree bindings look reasonable to me, so,

Acked-by: Stephen Warren <swarren@nvidia.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steffen Trumtrar Nov. 13, 2012, 8:32 a.m. UTC | #4
Hi!

On Mon, Nov 12, 2012 at 07:58:40PM +0100, Sascha Hauer wrote:
> Hi Steffen,
> 
> You lose memory in several places:
> 
> On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
> > +static 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;
> 
> Here
> 
> > +	}
> > +
> > +	return dt;
> > +}
> > +
> > +/**
> > + * of_get_display_timings - parse all display_timing entries from a device_node
> > + * @np: device_node with the subnodes
> > + **/
> > +struct display_timings *of_get_display_timings(struct device_node *np)
> > +{
> > +	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);
> > +	if (!disp)
> > +		return -ENOMEM;
> > +
> > +	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;
> 
> Here
> 
> > +	}
> > +
> > +	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);
> > +	if (!disp->timings)
> > +		return -ENOMEM;
> 
> Here
> 
> > +
> > +	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;
> 
> Here
> 
> > +		}
> > +
> > +		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;
> 
> and here
> 

Well,... you are right. :-(

Regards,
Steffen
Steffen Trumtrar Nov. 13, 2012, 10:53 a.m. UTC | #5
On Mon, Nov 12, 2012 at 11:00:37PM +0400, Alexey Klimov wrote:
> Hello Steffen,
> 
> On Mon, Nov 12, 2012 at 7:37 PM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> > This adds support for reading display timings from DT or/and convert one of those
> > timings to a videomode.
> > The of_display_timing implementation supports multiple children where each
> > property can have up to 3 values. All children are read into an array, that
> > can be queried.
> > of_get_videomode converts exactly one of that timings to a struct videomode.
> >
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
> >  drivers/video/Kconfig                              |   13 ++
> >  drivers/video/Makefile                             |    2 +
> >  drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
> >  drivers/video/of_videomode.c                       |   47 +++++
> >  include/linux/of_display_timings.h                 |   19 ++
> >  include/linux/of_videomode.h                       |   15 ++
> >  7 files changed, 389 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
> >  create mode 100644 drivers/video/of_display_timing.c
> >  create mode 100644 drivers/video/of_videomode.c
> >  create mode 100644 include/linux/of_display_timings.h
> >  create mode 100644 include/linux/of_videomode.h
> >
> > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> > new file mode 100644
> > index 0000000..e22e2fd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> > @@ -0,0 +1,107 @@
> > +display-timings bindings
> > +========================
> > +
> > +display-timings-node
> > +--------------------
> > +
> > +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-frequency = <52000000>;
> > +                       hactive = <1920>;
> > +                       vactive = <1080>;
> > +                       hfront-porch = <25>;
> > +                       hback-porch = <25>;
> > +                       hsync-len = <25>;
> > +                       vback-porch = <2>;
> > +                       vfront-porch = <2>;
> > +                       vsync-len = <2>;
> > +                       hsync-active = <1>;
> > +               };
> > +       };
> > +
> > +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-frequency = <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>;
> > +       };
> > diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> > index 2a23b18..a324457 100644
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -39,6 +39,19 @@ config DISPLAY_TIMING
> >  config VIDEOMODE
> >         bool
> >
> > +config OF_DISPLAY_TIMING
> > +       def_bool y
> > +       select DISPLAY_TIMING
> > +       help
> > +         helper to parse display timings from the devicetree
> > +
> > +config OF_VIDEOMODE
> > +       def_bool y
> > +       select VIDEOMODE
> > +       select OF_DISPLAY_TIMING
> > +       help
> > +         helper to get videomodes from the devicetree
> > +
> >  menuconfig FB
> >         tristate "Support for frame buffer devices"
> >         ---help---
> > diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> > index fc30439..b936b00 100644
> > --- a/drivers/video/Makefile
> > +++ b/drivers/video/Makefile
> > @@ -168,4 +168,6 @@ 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_OF_DISPLAY_TIMING) += of_display_timing.o
> >  obj-$(CONFIG_VIDEOMODE) += videomode.o
> > +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> > diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
> > new file mode 100644
> > index 0000000..0bd30cc
> > --- /dev/null
> > +++ b/drivers/video/of_display_timing.c
> > @@ -0,0 +1,186 @@
> > +/*
> > + * 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, cells, 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(np, name, &result->typ);
> > +               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
> > + **/
> > +static 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;
> > +}
> > +
> > +/**
> > + * of_get_display_timings - parse all display_timing entries from a device_node
> > + * @np: device_node with the subnodes
> > + **/
> > +struct display_timings *of_get_display_timings(struct device_node *np)
> > +{
> > +       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);
> > +       if (!disp)
> > +               return -ENOMEM;
> > +
> > +       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);
> > +       if (!disp->timings)
> > +               return -ENOMEM;
> 
> Could you please check return values here ^^^ and above "disp =
> kzalloc(sizeof(*disp), GFP_KERNEL);" ?
> May be it's better to return NULL instead of -ENOMEM and put error message?
> 
I will do that along with the memory leak fixes.

Regards,
Steffen
Thierry Reding Nov. 13, 2012, 11:08 a.m. UTC | #6
On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
> This adds support for reading display timings from DT or/and convert one of those
> timings to a videomode.
> The of_display_timing implementation supports multiple children where each
> property can have up to 3 values. All children are read into an array, that
> can be queried.
> of_get_videomode converts exactly one of that timings to a struct videomode.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  .../devicetree/bindings/video/display-timings.txt  |  107 +++++++++++
>  drivers/video/Kconfig                              |   13 ++
>  drivers/video/Makefile                             |    2 +
>  drivers/video/of_display_timing.c                  |  186 ++++++++++++++++++++
>  drivers/video/of_videomode.c                       |   47 +++++
>  include/linux/of_display_timings.h                 |   19 ++
>  include/linux/of_videomode.h                       |   15 ++
>  7 files changed, 389 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
>  create mode 100644 drivers/video/of_display_timing.c
>  create mode 100644 drivers/video/of_videomode.c
>  create mode 100644 include/linux/of_display_timings.h
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> new file mode 100644
> index 0000000..e22e2fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> @@ -0,0 +1,107 @@
> +display-timings bindings
> +========================
> +
> +display-timings-node
> +--------------------

Maybe leave away the last -, so that it reads "display-timings node"? I
think that makes it more obvious that the node is supposed to be called
"display-timings".

> +
> +required properties:
> + - none
> +
> +optional properties:
> + - native-mode: the native mode for the display, in case multiple modes are
> +		provided. When omitted, assume the first node is the native.
> +
> +timings-subnode
> +---------------

Same here: "timing subnodes"?

> +
> +required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters

"display"?

> +   in pixels
> +   vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> +   lines
> + - clock-frequency: displayclock in Hz

"display clock"?

> +
> +optional properties:
> + - hsync-active : Hsync pulse is active low/high/ignored
> + - vsync-active : Vsync pulse is active low/high/ignored
> + - de-active : Data-Enable pulse is active low/high/ignored
> + - pixelclk-inverted : pixelclock is inverted/non-inverted/ignored
> + - interlaced (bool)
> + - doublescan (bool)
> +
> +All the optional properties that are not bool follow the following logic:
> +    <1> : high active
> +    <0> : low active
> +    omitted : not used on hardware

Nitpick: You use space before : in the optional properties, but not in
the required properties above.

> +
> +There are different ways of describing the capabilities of a display. The devicetree
> +representation corresponds to the one commonly found in datasheets for displays.
> +If a display supports multiple signal timings, the native-mode can be specified.
> +
> +The parameters are defined as
> +
> +struct display_timing
> +=====================

struct display_timing has no meaning in device tree documentation. Maybe
this line can just go away?

> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 2a23b18..a324457 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -39,6 +39,19 @@ config DISPLAY_TIMING
>  config VIDEOMODE
>         bool
>  
> +config OF_DISPLAY_TIMING

OF_DISPLAY_TIMINGS?

> diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c

of_display_timings.c?

> +/**
> + * of_get_display_timings - parse all display_timing entries from a device_node
> + * @np: device_node with the subnodes
> + **/
> +struct display_timings *of_get_display_timings(struct device_node *np)
[...]
> +	for_each_child_of_node(timings_np, entry) {
> +		struct display_timing *dt;
> +
> +		dt = of_get_display_timing(entry);
> +		if (!dt) {
> +			/* to not encourage wrong devicetrees, fail in case of an error */
> +			pr_err("%s: error in timing %d\n", __func__, disp->num_timings+1);
> +			return NULL;
> +		}

In case of a parsing error, of_get_display_timing() already shows an
error message, so I don't think we need another one here.

> +/**
> + * of_display_timings_exists - check if a display-timings node is provided
> + * @np: device_node with the timing
> + **/
> +int of_display_timings_exists(struct device_node *np)
> +{
> +	struct device_node *timings_np;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	timings_np = of_parse_phandle(np, "display-timings", 0);
> +	if (!timings_np)
> +		return -EINVAL;
> +
> +	return 1;

I think this is missing of_node_put(timings_np) in both failure and
success cases. Also, maybe this should really return a bool instead?

Also, why do you use of_parse_phandle() for this? Aren't the
display-timings nodes expected to be children of some other node like an
output/display device?

> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
[...]
> +
> +int of_get_videomode(struct device_node *np, struct videomode *vm, int index);
> +#endif /* __LINUX_OF_VIDEOMODE_H */

There should be a blank line between the last two lines I think.

Thierry
Steffen Trumtrar Nov. 13, 2012, 12:59 p.m. UTC | #7
On Mon, Nov 12, 2012 at 01:40:12PM -0700, Stephen Warren wrote:
> On 11/12/2012 08:37 AM, Steffen Trumtrar wrote:
> > This adds support for reading display timings from DT or/and convert one of those
> > timings to a videomode.
> > The of_display_timing implementation supports multiple children where each
> > property can have up to 3 values. All children are read into an array, that
> > can be queried.
> > of_get_videomode converts exactly one of that timings to a struct videomode.
> 
> > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> 
> The device tree bindings look reasonable to me, so,
> 
> Acked-by: Stephen Warren <swarren@nvidia.com>
> 
> 

Thanks,

Steffen
Stephen Warren Nov. 13, 2012, 5:46 p.m. UTC | #8
On 11/13/2012 04:08 AM, Thierry Reding wrote:
> On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
>> This adds support for reading display timings from DT or/and
>> convert one of those timings to a videomode. The
>> of_display_timing implementation supports multiple children where
>> each property can have up to 3 values. All children are read into
>> an array, that can be queried. of_get_videomode converts exactly
>> one of that timings to a struct videomode.

>> diff --git
>> a/Documentation/devicetree/bindings/video/display-timings.txt
>> b/Documentation/devicetree/bindings/video/display-timings.txt

>> + - clock-frequency: displayclock in Hz
> 
> "display clock"?

I /think/ I had suggested naming this clock-frequency before so that
the property name would be more standardized; other bindings use that
same name. But I'm not too attached to the name I guess.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Nov. 13, 2012, 5:51 p.m. UTC | #9
On Tue, Nov 13, 2012 at 10:46:53AM -0700, Stephen Warren wrote:
> On 11/13/2012 04:08 AM, Thierry Reding wrote:
> > On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
> >> This adds support for reading display timings from DT or/and
> >> convert one of those timings to a videomode. The
> >> of_display_timing implementation supports multiple children where
> >> each property can have up to 3 values. All children are read into
> >> an array, that can be queried. of_get_videomode converts exactly
> >> one of that timings to a struct videomode.
> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/video/display-timings.txt
> >> b/Documentation/devicetree/bindings/video/display-timings.txt
> 
> >> + - clock-frequency: displayclock in Hz
> > 
> > "display clock"?
> 
> I /think/ I had suggested naming this clock-frequency before so that
> the property name would be more standardized; other bindings use that
> same name. But I'm not too attached to the name I guess.

That's not what I meant. I think "displayclock" should be two words in
the description of the property. The property name is fine.

Thierry
Mitch Bradley Nov. 13, 2012, 6:13 p.m. UTC | #10
On 11/13/2012 7:51 AM, Thierry Reding wrote:
> On Tue, Nov 13, 2012 at 10:46:53AM -0700, Stephen Warren wrote:
>> On 11/13/2012 04:08 AM, Thierry Reding wrote:
>>> On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
>>>> This adds support for reading display timings from DT or/and
>>>> convert one of those timings to a videomode. The
>>>> of_display_timing implementation supports multiple children where
>>>> each property can have up to 3 values. All children are read into
>>>> an array, that can be queried. of_get_videomode converts exactly
>>>> one of that timings to a struct videomode.
>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/video/display-timings.txt
>>>> b/Documentation/devicetree/bindings/video/display-timings.txt
>>
>>>> + - clock-frequency: displayclock in Hz
>>>
>>> "display clock"?
>>
>> I /think/ I had suggested naming this clock-frequency before so that
>> the property name would be more standardized; other bindings use that
>> same name. But I'm not too attached to the name I guess.
> 
> That's not what I meant. I think "displayclock" should be two words in
> the description of the property. The property name is fine.

Given that modern display engines often have numerous clocks, perhaps it
would be better to use a more specific name, like for example "pixel-clock".

> 
> Thierry
> 
> 
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Nov. 13, 2012, 7:17 p.m. UTC | #11
On Tue, Nov 13, 2012 at 08:13:31AM -1000, Mitch Bradley wrote:
> On 11/13/2012 7:51 AM, Thierry Reding wrote:
> > On Tue, Nov 13, 2012 at 10:46:53AM -0700, Stephen Warren wrote:
> >> On 11/13/2012 04:08 AM, Thierry Reding wrote:
> >>> On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
> >>>> This adds support for reading display timings from DT or/and
> >>>> convert one of those timings to a videomode. The
> >>>> of_display_timing implementation supports multiple children where
> >>>> each property can have up to 3 values. All children are read into
> >>>> an array, that can be queried. of_get_videomode converts exactly
> >>>> one of that timings to a struct videomode.
> >>
> >>>> diff --git
> >>>> a/Documentation/devicetree/bindings/video/display-timings.txt
> >>>> b/Documentation/devicetree/bindings/video/display-timings.txt
> >>
> >>>> + - clock-frequency: displayclock in Hz
> >>>
> >>> "display clock"?
> >>
> >> I /think/ I had suggested naming this clock-frequency before so that
> >> the property name would be more standardized; other bindings use that
> >> same name. But I'm not too attached to the name I guess.
> > 
> > That's not what I meant. I think "displayclock" should be two words in
> > the description of the property. The property name is fine.
> 
> Given that modern display engines often have numerous clocks, perhaps it
> would be better to use a more specific name, like for example "pixel-clock".

This binding is only about defining display modes. Are any of the clocks
that you're referring to relevant to the actual display modes?

Thierry
Thierry Reding Nov. 14, 2012, 10:59 a.m. UTC | #12
On Mon, Nov 12, 2012 at 04:37:02PM +0100, Steffen Trumtrar wrote:
[...]
> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
[...]
> +#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_timings(struct device_node *np);
> +int of_display_timings_exists(struct device_node *np);

This either needs to include linux/of.h or a forward declaration of
struct device_node. Otherwise this will fail to compile if the file
where this is included from doesn't pull linux/of.h in explicitly.

> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
[...]
> +#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);

Same here.

Thierry
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
new file mode 100644
index 0000000..e22e2fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,107 @@ 
+display-timings bindings
+========================
+
+display-timings-node
+--------------------
+
+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-frequency = <52000000>;
+			hactive = <1920>;
+			vactive = <1080>;
+			hfront-porch = <25>;
+			hback-porch = <25>;
+			hsync-len = <25>;
+			vback-porch = <2>;
+			vfront-porch = <2>;
+			vsync-len = <2>;
+			hsync-active = <1>;
+		};
+	};
+
+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-frequency = <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>;
+	};
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 2a23b18..a324457 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -39,6 +39,19 @@  config DISPLAY_TIMING
 config VIDEOMODE
        bool
 
+config OF_DISPLAY_TIMING
+	def_bool y
+	select DISPLAY_TIMING
+	help
+	  helper to parse display timings from the devicetree
+
+config OF_VIDEOMODE
+	def_bool y
+	select VIDEOMODE
+	select OF_DISPLAY_TIMING
+	help
+	  helper to get videomodes from the devicetree
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index fc30439..b936b00 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -168,4 +168,6 @@  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_OF_DISPLAY_TIMING) += of_display_timing.o
 obj-$(CONFIG_VIDEOMODE) += videomode.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
new file mode 100644
index 0000000..0bd30cc
--- /dev/null
+++ b/drivers/video/of_display_timing.c
@@ -0,0 +1,186 @@ 
+/*
+ * 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, cells, 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(np, name, &result->typ);
+		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
+ **/
+static 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;
+}
+
+/**
+ * of_get_display_timings - parse all display_timing entries from a device_node
+ * @np: device_node with the subnodes
+ **/
+struct display_timings *of_get_display_timings(struct device_node *np)
+{
+	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);
+	if (!disp)
+		return -ENOMEM;
+
+	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);
+	if (!disp->timings)
+		return -ENOMEM;
+
+	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_timings);
+
+/**
+ * of_display_timings_exists - check if a display-timings node is provided
+ * @np: device_node with the timing
+ **/
+int of_display_timings_exists(struct device_node *np)
+{
+	struct device_node *timings_np;
+
+	if (!np)
+		return -EINVAL;
+
+	timings_np = of_parse_phandle(np, "display-timings", 0);
+	if (!timings_np)
+		return -EINVAL;
+
+	return 1;
+}
+EXPORT_SYMBOL_GPL(of_display_timings_exists);
diff --git a/drivers/video/of_videomode.c b/drivers/video/of_videomode.c
new file mode 100644
index 0000000..7051701
--- /dev/null
+++ b/drivers/video/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_timings instead.
+ **/
+int of_get_videomode(struct device_node *np, struct videomode *vm, int index)
+{
+	struct display_timings *disp;
+	int ret;
+
+	disp = of_get_display_timings(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_display_timings.h b/include/linux/of_display_timings.h
new file mode 100644
index 0000000..ccf6814
--- /dev/null
+++ b/include/linux/of_display_timings.h
@@ -0,0 +1,19 @@ 
+/*
+ * 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_timings(struct device_node *np);
+int of_display_timings_exists(struct device_node *np);
+
+#endif
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 */