[PATCH 0/2] video: of: display_timing: fixes

Hi,

Those two independent patches fixes DT display_timings related code.
The first patch replaces of_find_node_by_name by of_get_child_by_name.
Usage of of_find_node_by_name in such context is incorrect:
- we need only direct child, and this function looks for following nodes
  on implementation internal list regardless of the hierarchy,
- there is no warranty that child nodes are after parent nodes in
  this search order, it happens to be true for DT created from FDT,
  but it is not true for dynamic DT.

The second patch just removes unused/broken function.

Regards
Andrzej

Andrzej Hajda (2):
  video: of: display_timing: correct display-timings node finding
  video: of: display_timing: remove broken of_display_timings_exist

 drivers/video/of_display_timing.c | 26 +++-----------------------
 include/video/of_display_timing.h |  1 -
 2 files changed, 3 insertions(+), 24 deletions(-)

-- 
1.8.1.2

of_get_display_timing(s) use of_find_node_by_name
to get child node, this is incorrect, of_get_child_by_name
should be used instead. The patch fixes it.
Small typo is also corrected.

Signed-off-by: Andrzej Hajda <redacted>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/video/of_display_timing.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
index 171821d..ba5b40f 100644
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c

@@ -120,7 +120,7 @@ int of_get_display_timing(struct device_node *np, const char *name,
 		return -EINVAL;
 	}
 
-	timing_np = of_find_node_by_name(np, name);
+	timing_np = of_get_child_by_name(np, name);
 	if (!timing_np) {
 		pr_err("%s: could not find node '%s'\n",
 			of_node_full_name(np), name);

@@ -143,11 +143,11 @@ struct display_timings *of_get_display_timings(struct device_node *np)
 	struct display_timings *disp;
 
 	if (!np) {
-		pr_err("%s: no devicenode given\n", of_node_full_name(np));
+		pr_err("%s: no device node given\n", of_node_full_name(np));
 		return NULL;
 	}
 
-	timings_np = of_find_node_by_name(np, "display-timings");
+	timings_np = of_get_child_by_name(np, "display-timings");
 	if (!timings_np) {
 		pr_err("%s: could not find display-timings node\n",
 			of_node_full_name(np));

-- 
1.8.1.2

of_display_timings_exist is implemented incorrectly.
It tries to find property instead of node.
The function is not used anyway so the patch removes it.

Signed-off-by: Andrzej Hajda <redacted>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/video/of_display_timing.c | 20 --------------------
 include/video/of_display_timing.h |  1 -
 2 files changed, 21 deletions(-)

diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
index ba5b40f..b423bb9 100644
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c

@@ -247,23 +247,3 @@ dispfail:
 	return NULL;
 }
 EXPORT_SYMBOL_GPL(of_get_display_timings);
-
-/**
- * of_display_timings_exist - check if a display-timings node is provided
- * @np: device_node with the timing
- **/
-int of_display_timings_exist(struct device_node *np)
-{
-	struct device_node *timings_np;
-
-	if (!np)
-		return -EINVAL;
-
-	timings_np = of_parse_phandle(np, "display-timings", 0);
-	if (!timings_np)
-		return -EINVAL;
-
-	of_node_put(timings_np);
-	return 1;
-}
-EXPORT_SYMBOL_GPL(of_display_timings_exist);

diff --git a/include/video/of_display_timing.h b/include/video/of_display_timing.h
index 79e6697..16cde75 100644
--- a/include/video/of_display_timing.h
+++ b/include/video/of_display_timing.h

@@ -18,6 +18,5 @@ struct display_timings;
 int of_get_display_timing(struct device_node *np, const char *name,
 		struct display_timing *dt);
 struct display_timings *of_get_display_timings(struct device_node *np);
-int of_display_timings_exist(struct device_node *np);
 
 #endif

-- 
1.8.1.2

On 25/09/13 14:51, Andrzej Hajda wrote:

of_display_timings_exist is implemented incorrectly.
It tries to find property instead of node.
The function is not used anyway so the patch removes it.

Signed-off-by: Andrzej Hajda <redacted>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/video/of_display_timing.c | 20 --------------------
 include/video/of_display_timing.h |  1 -
 2 files changed, 21 deletions(-)

Fix by removal? =).

Wouldn't it be better to fix the function? It's not used currently, but
the whole display-timings stuff is still quite new. One could use
of_get_display_timings() to check for the existence of timings, but that
function will print an error if no timings are found.

Then again, what would be the case where you want to check if the
timings exist... I'd presume that there either has to be timings or
there are no timings.

So, I don't know. Any else has opinions whether to remove or fix the
function?

 Tomi

On 25/09/13 14:51, Andrzej Hajda wrote:

of_get_display_timing(s) use of_find_node_by_name
to get child node, this is incorrect, of_get_child_by_name
should be used instead. The patch fixes it.
Small typo is also corrected.

Signed-off-by: Andrzej Hajda <redacted>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/video/of_display_timing.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Thanks, queuing for 3.12 fixes.

 Tomi