Re: [PATCH v7 2/8] of: add helper to parse display timings

[Steffen Trumtrar <s.trumtrar@pengutronix.de>]

On Thu, Nov 01, 2012 at 09:15:10PM +0100, Thierry Reding wrote:
> On Wed, Oct 31, 2012 at 10:28:02AM +0100, Steffen Trumtrar wrote:
> [...]
> > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt
> [...]
> > @@ -0,0 +1,139 @@
> > +display-timings bindings
> > +=========
> > +
> > +display-timings-node
> > +------------
> 
> Maybe extend the underline to the length of the section and subsection
> titles respectively?
> 
> > +struct display_timing
> > +=========> 
> Same here.
> 
> > +config OF_DISPLAY_TIMINGS
> > +	def_bool y
> > +	depends on DISPLAY_TIMING
> 
> Maybe this should be called OF_DISPLAY_TIMING to match DISPLAY_TIMING,
> or rename DISPLAY_TIMING to DISPLAY_TIMINGS for the sake of consistency?
> 

Yes, to all three above.

> > +/**
> > + * of_get_display_timing_list - parse all display_timing entries from a device_node
> > + * @np: device_node with the subnodes
> > + **/
> > +struct display_timings *of_get_display_timing_list(struct device_node *np)
> 
> Perhaps this would better be named of_get_display_timings() to match the
> return type?
> 

Hm, I'm not really sure about that. I found it to error prone, to have a function
of_get_display_timing and of_get_display_timings. That's why I chose
of_get_display_timing_list. But you are correct, that it doesn't match the return
value. Maybe I should just make the first function static and change the name as you
suggested.

> > +	disp = kzalloc(sizeof(*disp), GFP_KERNEL);
> 
> Shouldn't you be checking this for allocation failures?
> 
> > +	disp->timings = kzalloc(sizeof(struct display_timing *)*disp->num_timings,
> > +				GFP_KERNEL);
> 
> Same here.
> 

Yes, to both.

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 |