Re: [PATCH 1/2 v6] of: add helper to parse display timings
From: Steffen Trumtrar <hidden>
Date: 2012-10-05 16:16:20
On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote: A patch description would be useful for something like this.
Yes. Shame on me.
quoted
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt new file mode 100644...quoted
+Usage in backend +========Everything before that point in the file looks fine to me.
\o/
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 guess you are right about that.
quoted
+struct drm_display_mode +===========> > + + +----------+---------------------------------------------+----------+-------+ + | | | | | ↑ + | | | | | | + | | | | | | + +----------###############################################----------+-------+ |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.quoted
+ | # ↑ ↑ ↑ # | | | + | # | | | # | | | + | # | | | # | | | + | # | | | # | | | + | # | | | # | | | + | # | | | hdisplay # | | | + | #<--+--------------------+-------------------># | | | + | # | | | # | | | + | # |vsync_start | # | | | + | # | | | # | | | + | # | |vsync_end | # | | | + | # | | |vdisplay # | | | + | # | | | # | | | + | # | | | # | | | + | # | | | # | | | vtotal + | # | | | # | | | + | # | | | hsync_start # | | | + | #<--+---------+----------+------------------------------>| | | + | # | | | # | | | + | # | | | hsync_end # | | | + | #<--+---------+----------+-------------------------------------->| | + | # | | ↓ # | | | + +----------####|#########|################################----------+-------+ | + | | | | | | | | + | | | | | | | | + | | ↓ | | | | | + +----------+-------------+-------------------------------+----------+-------+ | + | | | | | | | + | | | | | | | + | | ↓ | | | ↓ + +----------+---------------------------------------------+----------+-------+ + htotal + <------------------------------------------------------------------------->quoted
diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.cquoted
+static int parse_property(struct device_node *np, char *name, + struct timing_entry *result)quoted
+ 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?
Yes, okay.
quoted
+ else if (cells = 3) + ret = of_property_read_u32_array(np, name, &result->min, cells);quoted
+struct display_timings *of_get_display_timing_list(struct device_node *np)quoted
+ 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");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?
Ah, yes. I will set the first child then.
quoted
+ 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.
The definition of timings in dt doesn't make much sense, when there are no timings defined, does it ? Maybe I should state the obvious in the binding, just in case.
quoted
+ 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?
In the sense of a pr_err not a -EINVAL I presume?! It is a little bit quiet in case of a faulty spec, that is right.
quoted
+ 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...
.. and we don't want that. Will fix.
quoted
+ disp->timings[disp->num_timings] = st; + disp->num_timings++; + }quoted
+ 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__);The message in the else clause is not necessarily true; there may have been some specified, but they just couldn't be parsed.
Right.
quoted
+int of_display_timings_exists(struct device_node *np) +{ + struct device_node *timings_np; + struct device_node *default_np; + + if (!np) + return -EINVAL; + + timings_np = of_parse_phandle(np, "display-timings", 0); + + if (!timings_np) + return -EINVAL; + + default_np = of_parse_phandle(np, "default-timing", 0); + + if (default_np) + return 0;If this function checks that a default-timing property exists, shouldn't the function be named of_display_default_timing_exists()?
Maybe I should split this in two functions. Thanks for your feedback. 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 |