@@ -0,0 +1,183 @@+/*+*OFhelpersforparsingdisplaytimings+*+*Copyright(c)2012SteffenTrumtrar<s.trumtrar@pengutronix.de>,Pengutronix+*+*basedonof_videomode.cbySaschaHauer<s.hauer@pengutronix.de>+*+*ThisfileisreleasedundertheGPLv2+*/+#include<linux/of.h>+#include<linux/slab.h>+#include<linux/export.h>+#include<linux/of_display_timings.h>++/* every signal_timing can be specified with either+*justthetypicalvalueorarangeconsistingof+*min/typ/max.+*Thisfunctionhelpshandlingthis+*/+staticintparse_property(structdevice_node*np,char*name,+structtiming_entry*result)+{+structproperty*prop;+intlength;+intcells;+intret;++prop=of_find_property(np,name,&length);+if(!prop){+pr_err("%s: could not find property %s\n",__func__,name);+return-EINVAL;+}++cells=length/sizeof(u32);++if(cells==1)+ret=of_property_read_u32_array(np,name,&result->typ,cells);+elseif(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;+}++returnret;+}++structsignal_timing*of_get_display_timing(structdevice_node*np)+{+structsignal_timing*st;+intret=0;++st=kzalloc(sizeof(*st),GFP_KERNEL);++if(!st){+pr_err("%s: could not allocate signal_timing struct\n",__func__);+returnNULL;+}++ret|=parse_property(np,"hback-porch",&st->hback_porch);+ret|=parse_property(np,"hfront-porch",&st->hfront_porch);+ret|=parse_property(np,"hactive",&st->hactive);+ret|=parse_property(np,"hsync-len",&st->hsync_len);+ret|=parse_property(np,"vback-porch",&st->vback_porch);+ret|=parse_property(np,"vfront-porch",&st->vfront_porch);+ret|=parse_property(np,"vactive",&st->vactive);+ret|=parse_property(np,"vsync-len",&st->vsync_len);+ret|=parse_property(np,"clock",&st->pixelclock);++st->vsync_pol_active_high=of_property_read_bool(np,"vsync-active-high");+st->hsync_pol_active_high=of_property_read_bool(np,"hsync-active-high");+st->de_pol_active_high=of_property_read_bool(np,"de-active-high");+st->pixelclk_pol_inverted=of_property_read_bool(np,"pixelclk-inverted");+st->interlaced=of_property_read_bool(np,"interlaced");+st->doublescan=of_property_read_bool(np,"doublescan");++if(ret){+pr_err("%s: error reading timing properties\n",__func__);+returnNULL;+}++returnst;+}+EXPORT_SYMBOL_GPL(of_get_display_timing);++structdisplay_timings*of_get_display_timing_list(structdevice_node*np)+{+structdevice_node*timings_np;+structdevice_node*entry;+structdisplay_timings*disp;+char*default_timing;++if(!np){+pr_err("%s: no devicenode given\n",__func__);+returnNULL;+}++timings_np=of_find_node_by_name(np,"display-timings");++if(!timings_np){+pr_err("%s: could not find display-timings node\n",__func__);+returnNULL;+}++disp=kzalloc(sizeof(*disp),GFP_KERNEL);++entry=of_parse_phandle(timings_np,"default-timing",0);++if(!entry){+pr_info("%s: no default-timing specified\n",__func__);+entry=of_find_node_by_name(np,"timing");+}++if(!entry){+pr_info("%s: no timing specifications given\n",__func__);+returndisp;+}++pr_info("%s: using %s as default timing\n",__func__,entry->name);++default_timing=(char*)entry->full_name;++disp->num_timings=0;++for_each_child_of_node(timings_np,entry){+disp->num_timings++;+}++disp->timings=kzalloc(sizeof(structsignal_timing*)*disp->num_timings,+GFP_KERNEL);++disp->num_timings=0;++for_each_child_of_node(timings_np,entry){+structsignal_timing*st;++st=of_get_display_timing(entry);++if(!st)+continue;++if(strcmp(default_timing,entry->full_name)==0)+disp->default_timing=disp->num_timings;++disp->timings[disp->num_timings]=st;+disp->num_timings++;+}+++of_node_put(timings_np);++if(disp->num_timings>=0)+pr_info("%s: got %d timings. Using timing #%d as default\n",__func__,+disp->num_timings,disp->default_timing+1);+else+pr_info("%s: no timings specified\n",__func__);++returndisp;+}+EXPORT_SYMBOL_GPL(of_get_display_timing_list);++intof_display_timings_exists(structdevice_node*np)+{+structdevice_node*timings_np;+structdevice_node*default_np;++if(!np)+return-EINVAL;++timings_np=of_parse_phandle(np,"display-timings",0);++if(!timings_np)+return-EINVAL;++default_np=of_parse_phandle(np,"default-timing",0);++if(default_np)+return0;++return-EINVAL;+}+EXPORT_SYMBOL_GPL(of_display_timings_exists);
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
Signed-off-by: Steffen Trumtrar <redacted>
---
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 212 ++++++++++++++++++++++++++++++++++++++++++
include/linux/of_videomode.h | 41 ++++++++
4 files changed, 259 insertions(+)
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
Everything before that point in the file looks fine to me.
Everything after this point in the file seems to be Linux-specific
implementation details. Does it really belong in the DT binding
documentation, rather than some Linux-specific documentation file?
I suspect the entire horizontal box above (and the entire vertical box
all the way down the left-hand side) should be on the bottom/right
instead of top/left. The reason I think this is because all of
vsync_start, vsync_end, vdisplay have to be referenced to some known
point, which is usually zero or the start of the timing definition, /or/
there would be some value indicating the size of the top marging/porch
in order to say where those other values are referenced to.
+ if (cells == 1)
+ ret = of_property_read_u32_array(np, name, &result->typ, cells);
Should that branch not just set result->min/max to typ as well?
Presumably it'd prevent any code that interprets struct timing_entry
from having to check if those values were 0 or not?
+ else if (cells == 3)
+ ret = of_property_read_u32_array(np, name, &result->min, cells);
I don't think you want to require the node have an explicit name; I
don't recall the DT binding documentation making that a requirement.
Instead, can't you either just leave the default unset, or pick the
first DT child node, irrespective of name?
+ if (!entry) {
+ pr_info("%s: no timing specifications given\n", __func__);
+ return disp;
+ }
The DT bindings don't state that it's mandatory to have some timing
specified, although I agree that it makes sense in practice.
+ for_each_child_of_node(timings_np, entry) {
+ struct signal_timing *st;
+
+ st = of_get_display_timing(entry);
+
+ if (!st)
+ continue;
I wonder if that shouldn't be an error?
+ if (strcmp(default_timing, entry->full_name) == 0)
+ disp->default_timing = disp->num_timings;
Hmm. Why not compare the node pointers rather than the name? Also, if
the parsing failed, then this can lead to default_timing being
uninitialized anyway...
From: Stephen Warren <hidden> Date: 2012-10-04 18:51:00
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
+ st = display_timings_get(disp, index);
+
+ if (!st) {
It's a little odd to leave a blank line between those two lines.
Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.
On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
quoted
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
+ st = display_timings_get(disp, index);
+
+ if (!st) {
It's a little odd to leave a blank line between those two lines.
Hm, well okay. That can be remedied
Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.
I am not sure, what the appropriate way to do this is. I can split it up (again).
--
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-5555 |
Hi Steffen,
On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
quoted
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
quoted
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
videomode *vm,
+ st = display_timings_get(disp, index);
+
+ if (!st) {
It's a little odd to leave a blank line between those two lines.
Hm, well okay. That can be remedied
quoted
Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.
I am not sure, what the appropriate way to do this is. I can split it up
(again).
I think it would make sense to move them to their respective subsystems.
--
Regards,
Laurent Pinchart
@@ -0,0 +1,222 @@+display-timings bindings+==================++display-timings-node+------------++required properties:+ - none++optional properties:+ - default-timing: the default timing value++timings-subnode+---------------++required properties:+ - hactive, vactive: Display resolution+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters+ in pixels+ vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in+ lines+ - clock: displayclock in Hz++optional properties:+ - hsync-active-high (bool): Hsync pulse is active high+ - vsync-active-high (bool): Vsync pulse is active high+ - de-active-high (bool): Data-Enable pulse is active high+ - pixelclk-inverted (bool): pixelclock is inverted+ - interlaced (bool)+ - doublescan (bool)
I think bool should be generally used for things that are on/off, like
interlace. For hsync-active-high & others I'd rather have 0/1 values as
others already suggested.
+There are different ways of describing the capabilities of a display. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+If a display supports multiple signal timings, the default-timing can be specified.
+
+The parameters are defined as
+
+struct signal_timing
+===================
+
+ +----------+---------------------------------------------+----------+-------+
+ | | ↑ | | |
+ | | |vback_porch | | |
+ | | ↓ | | |
+ +----------###############################################----------+-------+
+ | # ↑ # | |
+ | # | # | |
+ | hback # | # hfront | hsync |
+ | porch # | hactive # porch | len |
+ |<-------->#<---------------+--------------------------->#<-------->|<----->|
+ | # | # | |
+ | # |vactive # | |
+ | # | # | |
+ | # ↓ # | |
+ +----------###############################################----------+-------+
+ | | ↑ | | |
+ | | |vfront_porch | | |
+ | | ↓ | | |
+ +----------+---------------------------------------------+----------+-------+
+ | | ↑ | | |
+ | | |vsync_len | | |
+ | | ↓ | | |
+ +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+ display-timings {
+ default-timing = <&timing0>;
+ timing0: 1920p24 {
+ /* 1920x1080p24 */
@@ -0,0 +1,183 @@+/*+*OFhelpersforparsingdisplaytimings+*+*Copyright(c)2012SteffenTrumtrar<s.trumtrar@pengutronix.de>,Pengutronix+*+*basedonof_videomode.cbySaschaHauer<s.hauer@pengutronix.de>+*+*ThisfileisreleasedundertheGPLv2+*/+#include<linux/of.h>+#include<linux/slab.h>+#include<linux/export.h>+#include<linux/of_display_timings.h>++/* every signal_timing can be specified with either+*justthetypicalvalueorarangeconsistingof+*min/typ/max.+*Thisfunctionhelpshandlingthis+*/
The comment is not according to kernel coding style. And I'd start the
sentence with a capital letter =).
+static int parse_property(struct device_node *np, char *name,
+ struct timing_entry *result)
+{
+ struct property *prop;
+ int length;
+ int cells;
+ int ret;
+
+ prop = of_find_property(np, name, &length);
+ if (!prop) {
+ pr_err("%s: could not find property %s\n", __func__, name);
+ return -EINVAL;
+ }
+
+ cells = length / sizeof(u32);
+
+ if (cells == 1)
+ ret = of_property_read_u32_array(np, name, &result->typ, cells);
+ else if (cells == 3)
+ ret = of_property_read_u32_array(np, name, &result->min, cells);
+ else {
+ pr_err("%s: illegal timing specification in %s\n", __func__,
+ name);
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+struct signal_timing *of_get_display_timing(struct device_node *np)
+{
+ struct signal_timing *st;
+ int ret = 0;
+
+ st = kzalloc(sizeof(*st), GFP_KERNEL);
+
+ if (!st) {
+ pr_err("%s: could not allocate signal_timing struct\n", __func__);
+ return NULL;
+ }
+
+ ret |= parse_property(np, "hback-porch", &st->hback_porch);
+ ret |= parse_property(np, "hfront-porch", &st->hfront_porch);
+ ret |= parse_property(np, "hactive", &st->hactive);
+ ret |= parse_property(np, "hsync-len", &st->hsync_len);
+ ret |= parse_property(np, "vback-porch", &st->vback_porch);
+ ret |= parse_property(np, "vfront-porch", &st->vfront_porch);
+ ret |= parse_property(np, "vactive", &st->vactive);
+ ret |= parse_property(np, "vsync-len", &st->vsync_len);
+ ret |= parse_property(np, "clock", &st->pixelclock);
+
+ st->vsync_pol_active_high = of_property_read_bool(np, "vsync-active-high");
+ st->hsync_pol_active_high = of_property_read_bool(np, "hsync-active-high");
+ st->de_pol_active_high = of_property_read_bool(np, "de-active-high");
+ st->pixelclk_pol_inverted = of_property_read_bool(np, "pixelclk-inverted");
+ st->interlaced = of_property_read_bool(np, "interlaced");
+ st->doublescan = of_property_read_bool(np, "doublescan");
+
+ if (ret) {
+ pr_err("%s: error reading timing properties\n", __func__);
+ return NULL;
+ }
+
+ return st;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timing);
+
+struct display_timings *of_get_display_timing_list(struct device_node *np)
+{
+ struct device_node *timings_np;
+ struct device_node *entry;
+ struct display_timings *disp;
+ char *default_timing;
+
+ if (!np) {
+ pr_err("%s: no devicenode given\n", __func__);
+ return NULL;
+ }
+
+ timings_np = of_find_node_by_name(np, "display-timings");
+
+ if (!timings_np) {
+ pr_err("%s: could not find display-timings node\n", __func__);
+ return NULL;
+ }
+
+ disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+
+ entry = of_parse_phandle(timings_np, "default-timing", 0);
+
+ if (!entry) {
+ pr_info("%s: no default-timing specified\n", __func__);
+ entry = of_find_node_by_name(np, "timing");
+ }
If "default-timing" property is optional, I don't see any need for the
pr_info above, as it should be business as usual if the property doesn't
exist.
If the default-timing property doesn't exist, wouldn't it be simpler to
get the first subnode, instead of looking one with "timing" name?
+
+ if (!entry) {
+ pr_info("%s: no timing specifications given\n", __func__);
+ return disp;
+ }
Again, I don't think the pr_info is needed if this is a normal case.
Then again, perhaps this could be an error? Why would there be a display
node without any timings?
+
+ pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+ default_timing = (char *)entry->full_name;
I don't see you setting disp->default_timing to OF_DEFAULT_TIMING in
case there's no default_timing found.
Or, at least I presume OF_DEFAULT_TIMING is meant to mark non-existing
default timing. The name OF_DEFAULT_TIMING is not very descriptive to
me.
Would it make more sense to have the disp->default_timing as a pointer
to the timing, instead of index? Then a NULL value would mark a
non-existing default timing.
From: Tomi Valkeinen <hidden> Date: 2012-10-08 07:12:38
On Mon, 2012-10-08 at 10:07 +0300, Tomi Valkeinen wrote:
Hi,
I don't see you setting disp->default_timing to OF_DEFAULT_TIMING in
case there's no default_timing found.
Or, at least I presume OF_DEFAULT_TIMING is meant to mark non-existing
default timing. The name OF_DEFAULT_TIMING is not very descriptive to
me.
Ah, I see now from the second patch how this is meant to be used. So if
there's no default timing in DT data, disp->default_timing is 0, meaning
the first entry. And the caller of of_get_videomode() will use
OF_DEFAULT_TIMING as index to get the default mode.
So I think it's ok.
Tomi
From: Tomi Valkeinen <hidden> Date: 2012-10-08 07:21:53
On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
quoted hunk
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
Signed-off-by: Steffen Trumtrar <redacted>
---
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 212 ++++++++++++++++++++++++++++++++++++++++++
include/linux/of_videomode.h | 41 ++++++++
4 files changed, 259 insertions(+)
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
This is not really of related. And actually, neither is the struct
signal_timing in the previous patch. It would be nice to have these in a
common header that fb, drm, and others could use instead of each having
their own timing structs.
But that's probably out of scope for this series =). Did you check the
timing structs from the video related frameworks in the kernel to see if
your structs contain all the info the others have, so that, at least in
theory, everybody could use these common structs?
Tomi
@@ -0,0 +1,222 @@+display-timings bindings+==================++display-timings-node+------------++required properties:+ - none++optional properties:+ - default-timing: the default timing value++timings-subnode+---------------++required properties:+ - hactive, vactive: Display resolution+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters+ in pixels+ vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in+ lines+ - clock: displayclock in Hz++optional properties:+ - hsync-active-high (bool): Hsync pulse is active high+ - vsync-active-high (bool): Vsync pulse is active high+ - de-active-high (bool): Data-Enable pulse is active high+ - pixelclk-inverted (bool): pixelclock is inverted+ - interlaced (bool)+ - doublescan (bool)
I think bool should be generally used for things that are on/off, like
interlace. For hsync-active-high & others I'd rather have 0/1 values as
others already suggested.
quoted
+There are different ways of describing the capabilities of a display. The devicetree
+representation corresponds to the one commonly found in datasheets for displays.
+If a display supports multiple signal timings, the default-timing can be specified.
+
+The parameters are defined as
+
+struct signal_timing
+===================
+
+ +----------+---------------------------------------------+----------+-------+
+ | | ↑ | | |
+ | | |vback_porch | | |
+ | | ↓ | | |
+ +----------###############################################----------+-------+
+ | # ↑ # | |
+ | # | # | |
+ | hback # | # hfront | hsync |
+ | porch # | hactive # porch | len |
+ |<-------->#<---------------+--------------------------->#<-------->|<----->|
+ | # | # | |
+ | # |vactive # | |
+ | # | # | |
+ | # ↓ # | |
+ +----------###############################################----------+-------+
+ | | ↑ | | |
+ | | |vfront_porch | | |
+ | | ↓ | | |
+ +----------+---------------------------------------------+----------+-------+
+ | | ↑ | | |
+ | | |vsync_len | | |
+ | | ↓ | | |
+ +----------+---------------------------------------------+----------+-------+
+
+
+Example:
+
+ display-timings {
+ default-timing = <&timing0>;
+ timing0: 1920p24 {
+ /* 1920x1080p24 */
@@ -0,0 +1,183 @@+/*+*OFhelpersforparsingdisplaytimings+*+*Copyright(c)2012SteffenTrumtrar<s.trumtrar@pengutronix.de>,Pengutronix+*+*basedonof_videomode.cbySaschaHauer<s.hauer@pengutronix.de>+*+*ThisfileisreleasedundertheGPLv2+*/+#include<linux/of.h>+#include<linux/slab.h>+#include<linux/export.h>+#include<linux/of_display_timings.h>++/* every signal_timing can be specified with either+*justthetypicalvalueorarangeconsistingof+*min/typ/max.+*Thisfunctionhelpshandlingthis+*/
The comment is not according to kernel coding style. And I'd start the
sentence with a capital letter =).
quoted
+static int parse_property(struct device_node *np, char *name,
+ struct timing_entry *result)
+{
+ struct property *prop;
+ int length;
+ int cells;
+ int ret;
+
+ prop = of_find_property(np, name, &length);
+ if (!prop) {
+ pr_err("%s: could not find property %s\n", __func__, name);
+ return -EINVAL;
+ }
+
+ cells = length / sizeof(u32);
+
+ if (cells == 1)
+ ret = of_property_read_u32_array(np, name, &result->typ, cells);
+ else if (cells == 3)
+ ret = of_property_read_u32_array(np, name, &result->min, cells);
+ else {
+ pr_err("%s: illegal timing specification in %s\n", __func__,
+ name);
+ return -EINVAL;
+ }
+
+ return ret;
+}
+
+struct signal_timing *of_get_display_timing(struct device_node *np)
+{
+ struct signal_timing *st;
+ int ret = 0;
+
+ st = kzalloc(sizeof(*st), GFP_KERNEL);
+
+ if (!st) {
+ pr_err("%s: could not allocate signal_timing struct\n", __func__);
+ return NULL;
+ }
+
+ ret |= parse_property(np, "hback-porch", &st->hback_porch);
+ ret |= parse_property(np, "hfront-porch", &st->hfront_porch);
+ ret |= parse_property(np, "hactive", &st->hactive);
+ ret |= parse_property(np, "hsync-len", &st->hsync_len);
+ ret |= parse_property(np, "vback-porch", &st->vback_porch);
+ ret |= parse_property(np, "vfront-porch", &st->vfront_porch);
+ ret |= parse_property(np, "vactive", &st->vactive);
+ ret |= parse_property(np, "vsync-len", &st->vsync_len);
+ ret |= parse_property(np, "clock", &st->pixelclock);
+
+ st->vsync_pol_active_high = of_property_read_bool(np, "vsync-active-high");
+ st->hsync_pol_active_high = of_property_read_bool(np, "hsync-active-high");
+ st->de_pol_active_high = of_property_read_bool(np, "de-active-high");
+ st->pixelclk_pol_inverted = of_property_read_bool(np, "pixelclk-inverted");
+ st->interlaced = of_property_read_bool(np, "interlaced");
+ st->doublescan = of_property_read_bool(np, "doublescan");
+
+ if (ret) {
+ pr_err("%s: error reading timing properties\n", __func__);
+ return NULL;
+ }
+
+ return st;
+}
+EXPORT_SYMBOL_GPL(of_get_display_timing);
+
+struct display_timings *of_get_display_timing_list(struct device_node *np)
+{
+ struct device_node *timings_np;
+ struct device_node *entry;
+ struct display_timings *disp;
+ char *default_timing;
+
+ if (!np) {
+ pr_err("%s: no devicenode given\n", __func__);
+ return NULL;
+ }
+
+ timings_np = of_find_node_by_name(np, "display-timings");
+
+ if (!timings_np) {
+ pr_err("%s: could not find display-timings node\n", __func__);
+ return NULL;
+ }
+
+ disp = kzalloc(sizeof(*disp), GFP_KERNEL);
+
+ entry = of_parse_phandle(timings_np, "default-timing", 0);
+
+ if (!entry) {
+ pr_info("%s: no default-timing specified\n", __func__);
+ entry = of_find_node_by_name(np, "timing");
+ }
If "default-timing" property is optional, I don't see any need for the
pr_info above, as it should be business as usual if the property doesn't
exist.
If the default-timing property doesn't exist, wouldn't it be simpler to
get the first subnode, instead of looking one with "timing" name?
Yes. I will fix that.
quoted
+
+ if (!entry) {
+ pr_info("%s: no timing specifications given\n", __func__);
+ return disp;
+ }
Again, I don't think the pr_info is needed if this is a normal case.
Then again, perhaps this could be an error? Why would there be a display
node without any timings?
This should be an error. It is a relict from trying to define the display instead
of just the timings. I need to rework the whole no/wrong timings thing.
quoted
+
+ pr_info("%s: using %s as default timing\n", __func__, entry->name);
+
+ default_timing = (char *)entry->full_name;
I don't see you setting disp->default_timing to OF_DEFAULT_TIMING in
case there's no default_timing found.
Or, at least I presume OF_DEFAULT_TIMING is meant to mark non-existing
default timing. The name OF_DEFAULT_TIMING is not very descriptive to
me.
Would it make more sense to have the disp->default_timing as a pointer
to the timing, instead of index? Then a NULL value would mark a
non-existing default timing.
On Mon, Oct 08, 2012 at 10:21:53AM +0300, Tomi Valkeinen wrote:
On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
quoted
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
Signed-off-by: Steffen Trumtrar <redacted>
---
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 212 ++++++++++++++++++++++++++++++++++++++++++
include/linux/of_videomode.h | 41 ++++++++
4 files changed, 259 insertions(+)
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
This is not really of related. And actually, neither is the struct
signal_timing in the previous patch. It would be nice to have these in a
common header that fb, drm, and others could use instead of each having
their own timing structs.
But that's probably out of scope for this series =). Did you check the
timing structs from the video related frameworks in the kernel to see if
your structs contain all the info the others have, so that, at least in
theory, everybody could use these common structs?
Tomi
Yes. Stephen and Laurent already suggested to split it up.
No, all info is not contained. That starts with drm, which has width-mm,..
If time permits, I will go over that.
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-5555 |
Hi Steffen,
Thanks for the patch.
On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
quoted hunk
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
Signed-off-by: Steffen Trumtrar <redacted>
---
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 212 +++++++++++++++++++++++++++++++++++++++
include/linux/of_videomode.h | 41 ++++++++
4 files changed, 259 insertions(+)
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/fb.h>
+#include <linux/slab.h>
+#include <drm/drm_mode.h>
+#include <linux/of_display_timings.h>
+#include <linux/of_videomode.h>
+
+void dump_fb_videomode(struct fb_videomode *m)
+{
+ pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
That's going to be pretty difficult to read :-) Would it make sense to group
several attributes logically (for instance using %ux%u for m->xres, m->yres) ?
I wonder how to avoid abuse of this functions. It's a useful helper for
drivers that need to get a video mode once only, but would result in lower
performances if a driver calls it for every mode. Drivers must call
of_get_display_timing_list instead in that case and case the display timings.
I'm wondering whether we should really expose of_get_videomode.
+{
+ struct display_timings *disp;
+ int ret = 0;
No need to assign ret to 0 here.
+
+ disp = of_get_display_timing_list(np);
+
You can remove the blank line.
+ if (!disp) {
+ pr_err("%s: no timings specified\n", __func__);
+ return -EINVAL;
+ }
+
+ if (index == OF_DEFAULT_TIMING)
+ index = disp->default_timing;
+
+ ret = videomode_from_timing(disp, vm, index);
+
No need for a blank line.
+ if (ret)
+ return ret;
+
+ display_timings_release(disp);
+
+ if (!vm) {
+ pr_err("%s: could not get videomode %d\n", __func__, index);
+ return -EINVAL;
+ }
This can't happen. If vm is NULL the videomode_from_timing call above will
return -EINVAL, and this function will then return immediately without
reaching this code block.
Same as above, do we really need to expose this helper function ? If so we
should at least clearly document (using kerneldoc for instance) that drivers
should only use it if they need to get a single mode once.
What about not defining those if CONFIG_DRM is not set ? No driver should call
these functions in that case. If we really need those stubs they should return
an error.
This is not really of related. And actually, neither is the struct
signal_timing in the previous patch. It would be nice to have these in a
common header that fb, drm, and others could use instead of each having
their own timing structs.
But that's probably out of scope for this series =). Did you check the
timing structs from the video related frameworks in the kernel to see if
your structs contain all the info the others have, so that, at least in
theory, everybody could use these common structs?
Tomi
Yes. Stephen and Laurent already suggested to split it up.
No, all info is not contained. That starts with drm, which has width-mm,..
If time permits, I will go over that.
Just to make sure we won't forget it, the V4L2 version of the timings
structure is struct v4l2_bt_timings in include/linux/videodev2.h.
--
Regards,
Laurent Pinchart
On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
Hi Steffen,
Thanks for the patch.
On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
quoted
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
Signed-off-by: Steffen Trumtrar <redacted>
---
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 212 +++++++++++++++++++++++++++++++++++++++
include/linux/of_videomode.h | 41 ++++++++
4 files changed, 259 insertions(+)
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/fb.h>
+#include <linux/slab.h>
+#include <drm/drm_mode.h>
+#include <linux/of_display_timings.h>
+#include <linux/of_videomode.h>
+
+void dump_fb_videomode(struct fb_videomode *m)
+{
+ pr_debug("fb_videomode = %d %d %d %d %d %d %d %d %d %d %d %d %d\n",
That's going to be pretty difficult to read :-) Would it make sense to group
several attributes logically (for instance using %ux%u for m->xres, m->yres) ?
I wonder how to avoid abuse of this functions. It's a useful helper for
drivers that need to get a video mode once only, but would result in lower
performances if a driver calls it for every mode. Drivers must call
of_get_display_timing_list instead in that case and case the display timings.
I'm wondering whether we should really expose of_get_videomode.
The intent was to let the driver decide. That way all the other overhead may
be skipped.
quoted
+{
+ struct display_timings *disp;
+ int ret = 0;
No need to assign ret to 0 here.
Ah, yes. Unneeded in this case.
quoted
+
+ disp = of_get_display_timing_list(np);
+
You can remove the blank line.
quoted
+ if (!disp) {
+ pr_err("%s: no timings specified\n", __func__);
+ return -EINVAL;
+ }
+
+ if (index == OF_DEFAULT_TIMING)
+ index = disp->default_timing;
+
+ ret = videomode_from_timing(disp, vm, index);
+
No need for a blank line.
quoted
+ if (ret)
+ return ret;
+
+ display_timings_release(disp);
+
+ if (!vm) {
+ pr_err("%s: could not get videomode %d\n", __func__, index);
+ return -EINVAL;
+ }
This can't happen. If vm is NULL the videomode_from_timing call above will
return -EINVAL, and this function will then return immediately without
reaching this code block.
Same as above, do we really need to expose this helper function ? If so we
should at least clearly document (using kerneldoc for instance) that drivers
should only use it if they need to get a single mode once.
What about not defining those if CONFIG_DRM is not set ? No driver should call
these functions in that case. If we really need those stubs they should return
an error.
Hi Steffen,
On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
quoted
On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
quoted
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
Signed-off-by: Steffen Trumtrar <redacted>
---
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 212 +++++++++++++++++++++++++++++++++++
include/linux/of_videomode.h | 41 ++++++++
4 files changed, 259 insertions(+)
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
I wonder how to avoid abuse of this functions. It's a useful helper for
drivers that need to get a video mode once only, but would result in lower
performances if a driver calls it for every mode. Drivers must call
of_get_display_timing_list instead in that case and case the display
timings. I'm wondering whether we should really expose of_get_videomode.
The intent was to let the driver decide. That way all the other overhead may
be skipped.
My point is that driver writers might just call of_get_videomode() in a loop,
not knowing that it's expensive. I want to avoid that. We need to at least add
kerneldoc to the function stating that this shouldn't be done.
quoted
quoted
+{
+ struct display_timings *disp;
+ int ret = 0;
No need to assign ret to 0 here.
Ah, yes. Unneeded in this case.
quoted
quoted
+
+ disp = of_get_display_timing_list(np);
+
You can remove the blank line.
quoted
+ if (!disp) {
+ pr_err("%s: no timings specified\n", __func__);
+ return -EINVAL;
+ }
+
+ if (index == OF_DEFAULT_TIMING)
+ index = disp->default_timing;
+
+ ret = videomode_from_timing(disp, vm, index);
+
No need for a blank line.
quoted
+ if (ret)
+ return ret;
+
+ display_timings_release(disp);
+
+ if (!vm) {
+ pr_err("%s: could not get videomode %d\n", __func__, index);
+ return -EINVAL;
+ }
This can't happen. If vm is NULL the videomode_from_timing call above will
return -EINVAL, and this function will then return immediately without
reaching this code block.
Hi Laurent,
On Mon, Oct 08, 2012 at 10:52:04PM +0200, Laurent Pinchart wrote:
Hi Steffen,
On Monday 08 October 2012 14:48:01 Steffen Trumtrar wrote:
quoted
On Mon, Oct 08, 2012 at 02:13:50PM +0200, Laurent Pinchart wrote:
quoted
On Thursday 04 October 2012 19:59:20 Steffen Trumtrar wrote:
quoted
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
Signed-off-by: Steffen Trumtrar <redacted>
---
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 212 +++++++++++++++++++++++++++++++++++
include/linux/of_videomode.h | 41 ++++++++
4 files changed, 259 insertions(+)
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
I wonder how to avoid abuse of this functions. It's a useful helper for
drivers that need to get a video mode once only, but would result in lower
performances if a driver calls it for every mode. Drivers must call
of_get_display_timing_list instead in that case and case the display
timings. I'm wondering whether we should really expose of_get_videomode.
The intent was to let the driver decide. That way all the other overhead may
be skipped.
My point is that driver writers might just call of_get_videomode() in a loop,
not knowing that it's expensive. I want to avoid that. We need to at least add
kerneldoc to the function stating that this shouldn't be done.
You're right. That should be made clear in the code.
quoted
quoted
quoted
+{
+ struct display_timings *disp;
+ int ret = 0;
No need to assign ret to 0 here.
Ah, yes. Unneeded in this case.
quoted
quoted
+
+ disp = of_get_display_timing_list(np);
+
You can remove the blank line.
quoted
+ if (!disp) {
+ pr_err("%s: no timings specified\n", __func__);
+ return -EINVAL;
+ }
+
+ if (index == OF_DEFAULT_TIMING)
+ index = disp->default_timing;
+
+ ret = videomode_from_timing(disp, vm, index);
+
No need for a blank line.
quoted
+ if (ret)
+ return ret;
+
+ display_timings_release(disp);
+
+ if (!vm) {
+ pr_err("%s: could not get videomode %d\n", __func__, index);
+ return -EINVAL;
+ }
This can't happen. If vm is NULL the videomode_from_timing call above will
return -EINVAL, and this function will then return immediately without
reaching this code block.
On Sun, Oct 07, 2012 at 03:38:33PM +0200, Laurent Pinchart wrote:
Hi Steffen,
On Friday 05 October 2012 17:51:21 Steffen Trumtrar wrote:
quoted
On Thu, Oct 04, 2012 at 12:51:00PM -0600, Stephen Warren wrote:
quoted
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
quoted
Get videomode from devicetree in a format appropriate for the
backend. drm_display_mode and fb_videomode are supported atm.
Uses the display signal timings from of_display_timings
videomode *vm,
+ st = display_timings_get(disp, index);
+
+ if (!st) {
It's a little odd to leave a blank line between those two lines.
Hm, well okay. That can be remedied
quoted
Only half of the code in this file seems OF-related; the routines to
convert a timing to a videomode or drm display mode seem like they'd be
useful outside device tree, so I wonder if putting them into
of_videomode.c is the correct thing to do. Still, it's probably not a
big deal.
I am not sure, what the appropriate way to do this is. I can split it up
(again).
I think it would make sense to move them to their respective subsystems.
I agree. While looking at integrating this for Tegra DRM, I came across
the issue that if I build DRM as a module, linking with this code will
fail. The reason for that was that it was that the code, itself builtin,
uses drm_mode_set_name(), which would be exported by the drm module. So
I had to modifiy the Kconfig entries to be "def_tristate DRM". That
obviously isn't very nice since the code can also be used without DRM.
Moving the subsystem specific conversion routines to the respective
subsystems should solve any of these issues.
Thierry
This file needs to include linux/slab.h because it uses kfree() in the
inline functions. Alternatively I think I'd rather see the inline
functions moved out of the header, with the exception of the
signal_timing_get_value() function perhaps.
Moreover there should be a forward declaration of struct display_node
to avoid the need to include linux/of.h.
Thierry
This should be:
#if IS_ENABLED(CONFIG_DRM)
or the code below won't be included if DRM is built as a module. But see
my other replies as to how we can probably handle this better by moving
this into the DRM subsystem.
It appears the usual method to obtain a drm_display_mode to allocate it
using drm_mode_create(), which will allocate it and associate it with
the struct drm_device.
Now, if you do a memset() on the structure you'll overwrite a number of
fields that have previously been initialized and are actually required
to get everything cleaned up properly later on.
So I think we should remove the call to memset().
I'm slightly confused by the naming here. signal_timing seems overly
generic in this context. Is there any reason why this isn't called
display_timing or even display_mode?
This should be:
#if IS_ENABLED(CONFIG_DRM)
or the code below won't be included if DRM is built as a module. But see
my other replies as to how we can probably handle this better by moving
this into the DRM subsystem.
I already started with moving...now I only need some time to finish with it.
It appears the usual method to obtain a drm_display_mode to allocate it
using drm_mode_create(), which will allocate it and associate it with
the struct drm_device.
Now, if you do a memset() on the structure you'll overwrite a number of
fields that have previously been initialized and are actually required
to get everything cleaned up properly later on.
So I think we should remove the call to memset().
I was not aware of that. The memset has to go than, of course.
I'm slightly confused by the naming here. signal_timing seems overly
generic in this context. Is there any reason why this isn't called
display_timing or even display_mode?
You are right. I actually already changed that, for the same reasons.
It will be called display_timing in the next version, as I think that's what it really
is.
--
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-5555 |