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

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

Hi!

On Fri, Nov 02, 2012 at 10:49:47PM +0530, Leela Krishna Amudala wrote:
> Hello Steffen,
> 
> On Wed, Oct 31, 2012 at 2:58 PM, Steffen Trumtrar
> > +static int parse_property(struct device_node *np, char *name,
> > +                               struct timing_entry *result)
> > +{
> > +       struct property *prop;
> > +       int length;
> > +       int cells;
> > +       int ret;
> > +
> > +       prop = of_find_property(np, name, &length);
> > +       if (!prop) {
> > +               pr_err("%s: could not find property %s\n", __func__, name);
> > +               return -EINVAL;
> > +       }
> > +
> > +       cells = length / sizeof(u32);
> > +       if (cells == 1) {
> > +               ret = of_property_read_u32_array(np, name, &result->typ, cells);
> 
> As you are reading only one vaue, you can use of_property_read_u32 instead.
> 

Yes, thats copypasta, no need for _array here.

> > +               result->min = result->typ;
> > +               result->max = result->typ;
> > +       } else if (cells == 3) {
> > +               ret = of_property_read_u32_array(np, name, &result->min, cells);
> 
> You are considering only min element, what about typ and max elements?
> 

I start at the address of result->min and read three u32-values, therefore all
three (min,typ,max) are filled with values.

> > +
> > +/**
> > + * of_display_timings_exists - check if a display-timings node is provided
> > + * @np: device_node with the timing
> > + **/
> > +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;
> > +
> > +       return -EINVAL;
> 
> Here it should return success instead of -EINVAL.
> 

Yes.

> And one query.. are the binding properties names and "display-timings"
> node structure template  finalized..?
> 

I sure hope so. There actually is one error in the examples though.
The property clock is called clock-frequency. I included it correctly
at the top of display-timings.txt, but overlooked it in the examples.

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 |