Re: [PATCH v12 2/6] video: add of helper for videomode
Hi!
On Wed, Nov 21, 2012 at 09:03:38AM -0600, Rob Herring wrote:
> On 11/21/2012 05:52 AM, Thierry Reding wrote:
> > On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
> >> Hi!
> >>
> >> On Wed, Nov 21, 2012 at 10:12:43AM +, Manjunathappa, Prakash wrote:
> >>> Hi Steffen,
> >>>
> >>> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> +/**
> + * 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(const 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");
> >>>
> >>> I get below build warnings on this line
> >>> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> >>> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of
> >>> 'of_find_node_by_name' discards qualifiers from pointer target type
> >>> include/linux/of.h:167:28: note: expected 'struct device_node *' but
> >>> argument is of type 'const struct device_node *'
> >>>
> + * of_display_timings_exists - check if a display-timings node is
> provided
> + * @np: device_node with the timing
> + **/
> +int of_display_timings_exists(const struct device_node *np)
> +{
> +struct device_node *timings_np;
> +
> +if (!np)
> +return -EINVAL;
> +
> +timings_np = of_parse_phandle(np, "display-timings", 0);
> >>>
> >>> Also here:
> >>> drivers/video/of_display_timing.c: In function
> >>> 'of_display_timings_exists':
> >>> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of
> >>> 'of_parse_phandle' discards qualifiers from pointer target type
> >>> include/linux/of.h:258:28: note: expected 'struct device_node *' but
> >>> argument is of type 'const struct device_node *'
> >>>
> >>
> >> The warnings are because the of-functions do not use const pointers where
> >> they
> >> should. I had two options: don't use const pointers even if they should be
> >> and
> >> have no warnings or use const pointers and have a correct API. (Third
> >> option:
> >> send patches for of-functions). I chose the second option.
> >
> > Maybe a better approach would be a combination of 1 and 3: don't use
> > const pointers for struct device_node for now and bring the issue up
> > with the OF maintainers, possibly with patches attached that fix the
> > problematic functions.
>
> Why does this need to be const? Since some DT functions increment
> refcount the node, I'm not sure that making struct device_node const in
> general is right thing to do. I do think it should be okay for
> of_parse_phandle.
>
Okay, that seems right. I went a little to far with const'ing.
I will send a patch for of_parse_phandle as this function does
not seem to change the pointer it is given, but returns a new one
on which the refcount gets incremented.
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 v12 2/6] video: add of helper for videomode
On 11/21/2012 05:52 AM, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
>> Hi!
>>
>> On Wed, Nov 21, 2012 at 10:12:43AM +, Manjunathappa, Prakash wrote:
>>> Hi Steffen,
>>>
>>> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
+/**
+ * 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(const 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");
>>>
>>> I get below build warnings on this line
>>> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
>>> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of
>>> 'of_find_node_by_name' discards qualifiers from pointer target type
>>> include/linux/of.h:167:28: note: expected 'struct device_node *' but
>>> argument is of type 'const struct device_node *'
>>>
+ * of_display_timings_exists - check if a display-timings node is provided
+ * @np: device_node with the timing
+ **/
+int of_display_timings_exists(const struct device_node *np)
+{
+ struct device_node *timings_np;
+
+ if (!np)
+ return -EINVAL;
+
+ timings_np = of_parse_phandle(np, "display-timings", 0);
>>>
>>> Also here:
>>> drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
>>> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of
>>> 'of_parse_phandle' discards qualifiers from pointer target type
>>> include/linux/of.h:258:28: note: expected 'struct device_node *' but
>>> argument is of type 'const struct device_node *'
>>>
>>
>> The warnings are because the of-functions do not use const pointers where
>> they
>> should. I had two options: don't use const pointers even if they should be
>> and
>> have no warnings or use const pointers and have a correct API. (Third option:
>> send patches for of-functions). I chose the second option.
>
> Maybe a better approach would be a combination of 1 and 3: don't use
> const pointers for struct device_node for now and bring the issue up
> with the OF maintainers, possibly with patches attached that fix the
> problematic functions.
Why does this need to be const? Since some DT functions increment
refcount the node, I'm not sure that making struct device_node const in
general is right thing to do. I do think it should be okay for
of_parse_phandle.
Rob
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v12 2/6] video: add of helper for videomode
On 2012-11-20 17:54, Steffen Trumtrar wrote:
> +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: display clock 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
Inverted related to what? And isn't this a bool? Pixel clock is either
normal (whatever that is), or inverted. It can't be "not used".
I guess normal case is "pixel data is driven on the rising edge of pixel
clock"? If that's common knowledge, I guess it doesn't need to be
mentioned. But I always have to verify from the documentation what
"normal" means on this particular panel/soc =).
> + - 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
Perhaps it's obvious, but no harm being explicit: mention that the bool
properties are off is omitted.
And I didn't read the rest of the patches yet, so perhaps this is
already correct, but as I think this framework is usable without DT
also, the meanings of the fields in the structs should be explained in
the header files also in case they are not obvious.
> +Example:
> +
> + display-timings {
> + native-mode = <&timing0>;
> + timing0: 1920p24 {
This should still be 1080p24, not 1920p24 =).
> + /* 1920x1080p24 */
> + clock-frequency = <5200>;
> + 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>;
> + };
> + };
> +
> diff --git a/include/linux/of_display_timings.h
> b/include/linux/of_display_timings.h
> new file mode 100644
> index 000..2b4fa0a
> --- /dev/null
> +++ b/include/linux/of_display_timings.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar
> + *
> + * display timings of helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> +#define __LINUX_OF_DISPLAY_TIMINGS_H
> +
> +#include
> +#include
No need to include these, just add "struct ...;".
> +#define OF_USE_NATIVE_MODE -1
> +
> +struct display_timings *of_get_display_timings(const struct device_node *np);
> +int of_display_timings_exists(const struct device_node *np);
> +
> +#endif
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 000..4de5fcc
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar
> + *
> + * videomode of-helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#include
> +#include
Same here.
> +int of_get_videomode(const struct device_node *np, struct videomode *vm,
> + int index);
> +
> +#endif /* __LINUX_OF_VIDEOMODE_H */
>
Tomi
signature.asc
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v12 2/6] video: add of helper for videomode
Hi Steffen, On Tue, Nov 20, 2012 at 21:24:52, 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 > Acked-by: Stephen Warren > Reviewed-by: Thierry Reding > Acked-by: Thierry Reding > Tested-by: Thierry Reding > Tested-by: Philipp Zabel > Reviewed-by: Laurent Pinchart > --- > .../devicetree/bindings/video/display-timings.txt | 107 ++ > drivers/video/Kconfig | 13 ++ > drivers/video/Makefile |2 + > drivers/video/of_display_timing.c | 216 > > drivers/video/of_videomode.c | 48 + > include/linux/of_display_timings.h | 20 ++ > include/linux/of_videomode.h | 18 ++ > 7 files changed, 424 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..a05cade > --- /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: display clock 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 v12 2/6] video: add of helper for videomode
On Wed, Nov 21, 2012 at 09:03:38AM -0600, Rob Herring wrote:
> On 11/21/2012 05:52 AM, Thierry Reding wrote:
> > On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
> >> Hi!
> >>
> >> On Wed, Nov 21, 2012 at 10:12:43AM +, Manjunathappa, Prakash wrote:
> >>> Hi Steffen,
> >>>
> >>> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> +/**
> + * 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(const 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");
> >>>
> >>> I get below build warnings on this line
> >>> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> >>> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of
> >>> 'of_find_node_by_name' discards qualifiers from pointer target type
> >>> include/linux/of.h:167:28: note: expected 'struct device_node *' but
> >>> argument is of type 'const struct device_node *'
> >>>
> + * of_display_timings_exists - check if a display-timings node is
> provided
> + * @np: device_node with the timing
> + **/
> +int of_display_timings_exists(const struct device_node *np)
> +{
> +struct device_node *timings_np;
> +
> +if (!np)
> +return -EINVAL;
> +
> +timings_np = of_parse_phandle(np, "display-timings", 0);
> >>>
> >>> Also here:
> >>> drivers/video/of_display_timing.c: In function
> >>> 'of_display_timings_exists':
> >>> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of
> >>> 'of_parse_phandle' discards qualifiers from pointer target type
> >>> include/linux/of.h:258:28: note: expected 'struct device_node *' but
> >>> argument is of type 'const struct device_node *'
> >>>
> >>
> >> The warnings are because the of-functions do not use const pointers where
> >> they
> >> should. I had two options: don't use const pointers even if they should be
> >> and
> >> have no warnings or use const pointers and have a correct API. (Third
> >> option:
> >> send patches for of-functions). I chose the second option.
> >
> > Maybe a better approach would be a combination of 1 and 3: don't use
> > const pointers for struct device_node for now and bring the issue up
> > with the OF maintainers, possibly with patches attached that fix the
> > problematic functions.
>
> Why does this need to be const? Since some DT functions increment
> refcount the node, I'm not sure that making struct device_node const in
> general is right thing to do. I do think it should be okay for
> of_parse_phandle.
I wasn't proposing to do it everywhere but only where possible. If the
node is modified in some way then obviously it shouldn't be const.
Thierry
pgpUHANk5zvAE.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v12 2/6] video: add of helper for videomode
On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
> Hi!
>
> On Wed, Nov 21, 2012 at 10:12:43AM +, Manjunathappa, Prakash wrote:
> > Hi Steffen,
> >
> > On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> > > +/**
> > > + * 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(const 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");
> >
> > I get below build warnings on this line
> > drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> > drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of
> > 'of_find_node_by_name' discards qualifiers from pointer target type
> > include/linux/of.h:167:28: note: expected 'struct device_node *' but
> > argument is of type 'const struct device_node *'
> >
> > > + * of_display_timings_exists - check if a display-timings node is
> > > provided
> > > + * @np: device_node with the timing
> > > + **/
> > > +int of_display_timings_exists(const struct device_node *np)
> > > +{
> > > + struct device_node *timings_np;
> > > +
> > > + if (!np)
> > > + return -EINVAL;
> > > +
> > > + timings_np = of_parse_phandle(np, "display-timings", 0);
> >
> > Also here:
> > drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
> > drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of
> > 'of_parse_phandle' discards qualifiers from pointer target type
> > include/linux/of.h:258:28: note: expected 'struct device_node *' but
> > argument is of type 'const struct device_node *'
> >
>
> The warnings are because the of-functions do not use const pointers where they
> should. I had two options: don't use const pointers even if they should be and
> have no warnings or use const pointers and have a correct API. (Third option:
> send patches for of-functions). I chose the second option.
Maybe a better approach would be a combination of 1 and 3: don't use
const pointers for struct device_node for now and bring the issue up
with the OF maintainers, possibly with patches attached that fix the
problematic functions.
Thierry
pgpkCDqzVPoyR.pgp
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v12 2/6] video: add of helper for videomode
Hi!
On Wed, Nov 21, 2012 at 10:12:43AM +, Manjunathappa, Prakash wrote:
> Hi Steffen,
>
> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> > +/**
> > + * 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(const 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");
>
> I get below build warnings on this line
> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of
> 'of_find_node_by_name' discards qualifiers from pointer target type
> include/linux/of.h:167:28: note: expected 'struct device_node *' but argument
> is of type 'const struct device_node *'
>
> > + * of_display_timings_exists - check if a display-timings node is provided
> > + * @np: device_node with the timing
> > + **/
> > +int of_display_timings_exists(const struct device_node *np)
> > +{
> > + struct device_node *timings_np;
> > +
> > + if (!np)
> > + return -EINVAL;
> > +
> > + timings_np = of_parse_phandle(np, "display-timings", 0);
>
> Also here:
> drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of
> 'of_parse_phandle' discards qualifiers from pointer target type
> include/linux/of.h:258:28: note: expected 'struct device_node *' but argument
> is of type 'const struct device_node *'
>
The warnings are because the of-functions do not use const pointers where they
should. I had two options: don't use const pointers even if they should be and
have no warnings or use const pointers and have a correct API. (Third option:
send patches for of-functions). I chose the second option.
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
