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

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

quoted

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

quoted

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

quoted

+
+/**
+ * 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 |