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

On Fri, Oct 05, 2012 at 10:21:37AM -0600, Stephen Warren wrote:

On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:

quoted

On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:

quoted

On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:

...

quoted

quoted

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.

I did mean return an error; if we try to parse something and can't,
shouldn't we return an error?

I suppose it may be possible to limp on and use whatever subset of modes
could be parsed and drop the others, which is what this code does, but
the code after the loop would definitely return an error if zero timings
were parseable.

If a display supports multiple modes, I think it is better to have a working
mode (even if it is not the preferred one) than have none at all.
If there is no mode at all, that should be an error, 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-5555 |