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

[Thierry Reding]

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 
> +
> +#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 
> +
> +int of_get_videomode(struct device_node *np, struct videomode *vm, int 
> index);

Same here.

Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Thierry Reding]

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 
> +
> +#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 
> +
> +int of_get_videomode(struct device_node *np, struct videomode *vm, int 
> index);

Same here.

Thierry


pgp9uT9rQOMpN.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Mitch Bradley]

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-disc...@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Thierry Reding]

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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

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

[Steffen Trumtrar]

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

Thanks,

Steffen

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

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

[Thierry Reding]

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 
> Signed-off-by: Philipp Zabel 
> ---
>  .../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 000..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;
> +

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

[Steffen Trumtrar]

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
>  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 
> > Signed-off-by: Philipp Zabel 
> > ---
> >  .../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 000..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|  |
> >|
> > +  |  |?

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

[Thierry Reding]

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


pgp4ftBG2mSB3.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

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.

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

[Thierry Reding]

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


pgpy9L3k0mzRQ.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Warren]

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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Steffen Trumtrar]

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

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

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

[Mitch Bradley]

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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

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

[Steffen Trumtrar]

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

Thanks,

Steffen

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

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 
> Signed-off-by: Philipp Zabel 
> ---
>  .../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 000..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;
> +

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

[Steffen Trumtrar]

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
>  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 
> > Signed-off-by: Philipp Zabel 
> > ---
> >  .../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 000..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|  |
> >|
> > +  |  |↓

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

[Steffen Trumtrar]

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

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alexey Klimov]

Hello Steffen,

On Mon, Nov 12, 2012 at 7:37 PM, 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 
> Signed-off-by: Philipp Zabel 
> ---
>  .../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 000..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   |  |  
>  |
> +  |  |   

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

[Alexey Klimov]

Hello Steffen,

On Mon, Nov 12, 2012 at 7:37 PM, 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 
> Signed-off-by: Philipp Zabel 
> ---
>  .../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 000..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   |  |  
>  |
> +  |  |   

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

[Sascha Hauer]

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

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |

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

[Steffen Trumtrar]

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 
Signed-off-by: Philipp Zabel 
---
 .../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 000..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 = <52000

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

[Stephen Warren]

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 

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

[Stephen Warren]

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 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sascha Hauer]

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

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Steffen Trumtrar]

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 
Signed-off-by: Philipp Zabel 
---
 .../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 000..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 = <52000