Re: [PATCH 1/2 v6] of: add helper to parse display timings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> > new file mode 100644
> ...
> > +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.

> > +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.
> 
> > +  |          #   ↑         ↑          ↑                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |       hdisplay     #          |       |  |
> > +  |          #<--+--------------------+------------------->#          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |vsync_start         |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |vsync_end |                    #          |       |  |
> > +  |          #   |         |          |vdisplay            #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |                    #          |       |  | vtotal
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |     hsync_start    #          |       |  |
> > +  |          #<--+---------+----------+------------------------------>|       |  |
> > +  |          #   |         |          |                    #          |       |  |
> > +  |          #   |         |          |     hsync_end      #          |       |  |
> > +  |          #<--+---------+----------+-------------------------------------->|  |
> > +  |          #   |         |          ↓                    #          |       |  |
> > +  +----------####|#########|################################----------+-------+  |
> > +  |          |   |         |                               |          |       |  |
> > +  |          |   |         |                               |          |       |  |
> > +  |          |   ↓         |                               |          |       |  |
> > +  +----------+-------------+-------------------------------+----------+-------+  |
> > +  |          |             |                               |          |       |  |
> > +  |          |             |                               |          |       |  |
> > +  |          |             ↓                               |          |       |  ↓
> > +  +----------+---------------------------------------------+----------+-------+
> > +                                   htotal
> > +   <------------------------------------------------------------------------->
> 
> > diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c
> 
> > +static int parse_property(struct device_node *np, char *name,
> > +				struct timing_entry *result)
> 
> > +	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.

> > +	else if (cells == 3)
> > +		ret = of_property_read_u32_array(np, name, &result->min, cells);
> 
> > +struct display_timings *of_get_display_timing_list(struct device_node *np)
> 
> > +	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.

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

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

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

> > +		disp->timings[disp->num_timings] = st;
> > +		disp->num_timings++;
> > +	}
> 
> > +	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.

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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux