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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Steffen,

On Friday 05 October 2012 18:38:58 Steffen Trumtrar wrote:
> On Fri, Oct 05, 2012 at 10:21:37AM -0600, Stephen Warren wrote:
> > On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> > > On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
> > >> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > ...
> > 
> > >>> +	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.

If we fail completely in case of an error, DT writers will notice their bugs. 
If we ignore errors silently they won't, and we'll end up with buggy DTs (or, 
to be accurate, even more buggy DTs :-)). I'd rather fail completely in the 
first implementation and add workarounds later only if we need to.

-- 
Regards,

Laurent Pinchart