[PATCH 0/4] video: of: display_timing: Adjust err printing of of_get_display_timing()

[PATCH 0/4] video: of: display_timing: Adjust err printing of of_get_display_timing()

[Douglas Anderson]

As reported by Sam Ravnborg [1], after commit b8a2948fa2b3
("drm/panel: simple: Add ability to override typical timing") we now
see a pointless error message printed on every boot for many systems.
Let's fix that by adjusting who is responsible for printing error
messages when of_get_display_timing() is used.

Most certainly we can bikeshed the topic about whether this is the
right fix or we should instead add logic to panel_simple_probe() to
avoid calling of_get_display_timing() in the case where there is no
"panel-timing" sub-node.  If there is consensus that I should move the
fix to panel_simple_probe() I'm happy to spin this series.  In that
case we probably should _remove_ the extra prints that were already
present in the other two callers of of_get_display_timing().

While at it, fix a missing of_node_put() found by code inspection.

NOTE: amba-clcd and panel-lvds were only compile-tested.

[1] https://lkml.kernel.org/r/20190721093815.GA4375@ravnborg.org


Douglas Anderson (4):
  video: of: display_timing: Add of_node_put() in
    of_get_display_timing()
  video: of: display_timing: Don't yell if no timing node is present
  drm: panel-lvds: Spout an error if of_get_display_timing() gives an
    error
  video: amba-clcd: Spout an error if of_get_display_timing() gives an
    error

 drivers/gpu/drm/panel/panel-lvds.c |  5 ++++-
 drivers/video/fbdev/amba-clcd.c    |  4 +++-
 drivers/video/of_display_timing.c  | 11 +++++++----
 3 files changed, 14 insertions(+), 6 deletions(-)

-- 
2.22.0.657.g960e92d24f-goog

[PATCH 1/4] video: of: display_timing: Add of_node_put() in of_get_display_timing()

[Douglas Anderson]

From code inspection it can be seen that of_get_display_timing() is
lacking an of_node_put().  Add it.

Fixes: ffa3fd21de8a ("videomode: implement public of_get_display_timing()")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/video/of_display_timing.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
index f5c1c469c0af..5eedae0799f0 100644
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c
@@ -119,6 +119,7 @@ int of_get_display_timing(const struct device_node *np, const char *name,
 		struct display_timing *dt)
 {
 	struct device_node *timing_np;
+	int ret;
 
 	if (!np)
 		return -EINVAL;
@@ -129,7 +130,11 @@ int of_get_display_timing(const struct device_node *np, const char *name,
 		return -ENOENT;
 	}
 
-	return of_parse_display_timing(timing_np, dt);
+	ret = of_parse_display_timing(timing_np, dt);
+
+	of_node_put(timing_np);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(of_get_display_timing);
 
-- 
2.22.0.657.g960e92d24f-goog

Re: [PATCH 1/4] video: of: display_timing: Add of_node_put() in of_get_display_timing()

[Laurent Pinchart]

Hi Douglas,

Thank you for the patch.

On Mon, Jul 22, 2019 at 11:24:36AM -0700, Douglas Anderson wrote:
> From code inspection it can be seen that of_get_display_timing() is
> lacking an of_node_put().  Add it.
> 
> Fixes: ffa3fd21de8a ("videomode: implement public of_get_display_timing()")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
>  drivers/video/of_display_timing.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
> index f5c1c469c0af..5eedae0799f0 100644
> --- a/drivers/video/of_display_timing.c
> +++ b/drivers/video/of_display_timing.c
> @@ -119,6 +119,7 @@ int of_get_display_timing(const struct device_node *np, const char *name,
>  		struct display_timing *dt)
>  {
>  	struct device_node *timing_np;
> +	int ret;
>  
>  	if (!np)
>  		return -EINVAL;
> @@ -129,7 +130,11 @@ int of_get_display_timing(const struct device_node *np, const char *name,
>  		return -ENOENT;
>  	}
>  
> -	return of_parse_display_timing(timing_np, dt);
> +	ret = of_parse_display_timing(timing_np, dt);
> +
> +	of_node_put(timing_np);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_get_display_timing);
>  

-- 
Regards,

Laurent Pinchart

[PATCH 2/4] video: of: display_timing: Don't yell if no timing node is present

[Douglas Anderson]

There may be cases (like in panel-simple.c) where we have a sane
fallback if no timings are specified in the device tree.  Let's get
rid of the unconditional pr_err().  We can add error messages in
individual drivers if it makes sense.

NOTE: we'll still print errors if the node is present but there are
problems parsing the timings.

Fixes: b8a2948fa2b3 ("drm/panel: simple: Add ability to override typical timing")
Reported-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/video/of_display_timing.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
index 5eedae0799f0..abc9ada798ee 100644
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c
@@ -125,10 +125,8 @@ int of_get_display_timing(const struct device_node *np, const char *name,
 		return -EINVAL;
 
 	timing_np = of_get_child_by_name(np, name);
-	if (!timing_np) {
-		pr_err("%pOF: could not find node '%s'\n", np, name);
+	if (!timing_np)
 		return -ENOENT;
-	}
 
 	ret = of_parse_display_timing(timing_np, dt);
 
-- 
2.22.0.657.g960e92d24f-goog

[PATCH 3/4] drm: panel-lvds: Spout an error if of_get_display_timing() gives an error

[Douglas Anderson]

In the patch ("video: of: display_timing: Don't yell if no timing node
is present") we'll stop spouting an error directly in
of_get_display_timing() if no node is present.  Presumably panel-lvds
should take charge of spouting its own error now.

NOTE: we'll print two errors if the node was present but there were
problems parsing the timing node (one in of_parse_display_timing() and
this new one).  Since this is a fatal error for the driver's probe
(and presumably someone will be debugging), this should be OK.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/panel/panel-lvds.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
index 1ec57d0806a8..ad47cc95459e 100644
--- a/drivers/gpu/drm/panel/panel-lvds.c
+++ b/drivers/gpu/drm/panel/panel-lvds.c
@@ -147,8 +147,11 @@ static int panel_lvds_parse_dt(struct panel_lvds *lvds)
 	int ret;
 
 	ret = of_get_display_timing(np, "panel-timing", &timing);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(lvds->dev, "%pOF: problems parsing panel-timing (%d)\n",
+			np, ret);
 		return ret;
+	}
 
 	videomode_from_timing(&timing, &lvds->video_mode);
 
-- 
2.22.0.657.g960e92d24f-goog

[PATCH 4/4] video: amba-clcd: Spout an error if of_get_display_timing() gives an error

[Douglas Anderson]

In the patch ("video: of: display_timing: Don't yell if no timing node
is present") we'll stop spouting an error directly in
of_get_display_timing() if no node is present.  Presumably amba-clcd
should take charge of spouting its own error now.

NOTE: we'll print two errors if the node was present but there were
problems parsing the timing node (one in of_parse_display_timing() and
this new one).  Since this is a fatal error for the driver's probe
(and presumably someone will be debugging), this should be OK.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/video/fbdev/amba-clcd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 89324e42a033..7de43be6ef2c 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -561,8 +561,10 @@ static int clcdfb_of_get_dpi_panel_mode(struct device_node *node,
 	struct videomode video;
 
 	err = of_get_display_timing(node, "panel-timing", &timing);
-	if (err)
+	if (err) {
+		pr_err("%pOF: problems parsing panel-timing (%d)\n", node, err);
 		return err;
+	}
 
 	videomode_from_timing(&timing, &video);
 
-- 
2.22.0.657.g960e92d24f-goog

Re: [PATCH 4/4] video: amba-clcd: Spout an error if of_get_display_timing() gives an error

[Linus Walleij]

On Mon, Jul 22, 2019 at 8:25 PM Douglas Anderson <dianders@chromium.org> wrote:

> In the patch ("video: of: display_timing: Don't yell if no timing node
> is present") we'll stop spouting an error directly in
> of_get_display_timing() if no node is present.  Presumably amba-clcd
> should take charge of spouting its own error now.
>
> NOTE: we'll print two errors if the node was present but there were
> problems parsing the timing node (one in of_parse_display_timing() and
> this new one).  Since this is a fatal error for the driver's probe
> (and presumably someone will be debugging), this should be OK.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 0/4] video: of: display_timing: Adjust err printing of of_get_display_timing()

[Sam Ravnborg]

Hi Dough.

On Mon, Jul 22, 2019 at 11:24:35AM -0700, Douglas Anderson wrote:
> As reported by Sam Ravnborg [1], after commit b8a2948fa2b3
> ("drm/panel: simple: Add ability to override typical timing") we now
> see a pointless error message printed on every boot for many systems.
> Let's fix that by adjusting who is responsible for printing error
> messages when of_get_display_timing() is used.
> 
> Most certainly we can bikeshed the topic about whether this is the
> right fix or we should instead add logic to panel_simple_probe() to
> avoid calling of_get_display_timing() in the case where there is no
> "panel-timing" sub-node.  If there is consensus that I should move the
> fix to panel_simple_probe() I'm happy to spin this series.  In that
> case we probably should _remove_ the extra prints that were already
> present in the other two callers of of_get_display_timing().
> 
> While at it, fix a missing of_node_put() found by code inspection.
> 
> NOTE: amba-clcd and panel-lvds were only compile-tested.
> 
> [1] https://lkml.kernel.org/r/20190721093815.GA4375@ravnborg.org
> 
> 
> Douglas Anderson (4):
>   video: of: display_timing: Add of_node_put() in
>     of_get_display_timing()
>   video: of: display_timing: Don't yell if no timing node is present
>   drm: panel-lvds: Spout an error if of_get_display_timing() gives an
>     error
>   video: amba-clcd: Spout an error if of_get_display_timing() gives an
>     error

Series looks good - thanks.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

You could consider silencing display_timing as the last patch, but thats
a very small detail.

How do we apply these fixes - to drm-misc-next? Bartlomiej?

No need to go in via drm-misc-fixes as the offending commit is only in
drm-misc-next.

	Sam

Re: [PATCH 0/4] video: of: display_timing: Adjust err printing of of_get_display_timing()

[Bartlomiej Zolnierkiewicz]

Hi,

On 7/23/19 10:38 AM, Sam Ravnborg wrote:
> Hi Dough.
> 
> On Mon, Jul 22, 2019 at 11:24:35AM -0700, Douglas Anderson wrote:
>> As reported by Sam Ravnborg [1], after commit b8a2948fa2b3
>> ("drm/panel: simple: Add ability to override typical timing") we now
>> see a pointless error message printed on every boot for many systems.
>> Let's fix that by adjusting who is responsible for printing error
>> messages when of_get_display_timing() is used.
>>
>> Most certainly we can bikeshed the topic about whether this is the
>> right fix or we should instead add logic to panel_simple_probe() to
>> avoid calling of_get_display_timing() in the case where there is no
>> "panel-timing" sub-node.  If there is consensus that I should move the
>> fix to panel_simple_probe() I'm happy to spin this series.  In that
>> case we probably should _remove_ the extra prints that were already
>> present in the other two callers of of_get_display_timing().
>>
>> While at it, fix a missing of_node_put() found by code inspection.
>>
>> NOTE: amba-clcd and panel-lvds were only compile-tested.
>>
>> [1] https://lkml.kernel.org/r/20190721093815.GA4375@ravnborg.org
>>
>>
>> Douglas Anderson (4):
>>   video: of: display_timing: Add of_node_put() in
>>     of_get_display_timing()
>>   video: of: display_timing: Don't yell if no timing node is present
>>   drm: panel-lvds: Spout an error if of_get_display_timing() gives an
>>     error
>>   video: amba-clcd: Spout an error if of_get_display_timing() gives an
>>     error
> 
> Series looks good - thanks.
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> You could consider silencing display_timing as the last patch, but thats
> a very small detail.
> 
> How do we apply these fixes - to drm-misc-next? Bartlomiej?
> 
> No need to go in via drm-misc-fixes as the offending commit is only in
> drm-misc-next.
I've merged the whole series to drm-misc-next, thanks!

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics