diff mbox

[v2,06/16] drm/sun4i: Don't process LVDS if TCON doesn't support it

Message ID 20180227222701.9716-7-jernej.skrabec@siol.net (mailing list archive)
State New, archived
Headers show

Commit Message

Jernej Škrabec Feb. 27, 2018, 10:26 p.m. UTC 
TCON checks for LVDS properties even if it doesn't support it. Add a
check to skip that part of the code if TCON doesn't support channel 0.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 120 +++++++++++++++++++------------------
 1 file changed, 63 insertions(+), 57 deletions(-)

Comments

Jernej Škrabec Feb. 28, 2018, 9:43 p.m. UTC | #1 
Hi Maxime,

Dne sreda, 28. februar 2018 ob 08:36:08 CET je Maxime Ripard napisal(a):
> On Tue, Feb 27, 2018 at 11:26:51PM +0100, Jernej Skrabec wrote:
> > TCON checks for LVDS properties even if it doesn't support it. Add a
> > check to skip that part of the code if TCON doesn't support channel 0.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> I have already sent a similar patch here:
> https://lists.freedesktop.org/archives/dri-devel/2018-February/166665.html

Right. However, check last chunk in my patch. There is no need to call 
sun4i_rgb_init() if TCON doesn't support channel 0. It doesn't do anything, 
except producing warning. Will you add that this change to your patch and then 
I can remove this patch from next revision?

BTW, your patch won't apply cleanly, since you didn't base it on latest code 
(every TCON variant has at least one entry now).

Best regards,
Jernej
Maxime Ripard March 2, 2018, 8:12 a.m. UTC | #2 
Hi,

On Wed, Feb 28, 2018 at 10:43:30PM +0100, Jernej Škrabec wrote:
> Dne sreda, 28. februar 2018 ob 08:36:08 CET je Maxime Ripard napisal(a):
> > On Tue, Feb 27, 2018 at 11:26:51PM +0100, Jernej Skrabec wrote:
> > > TCON checks for LVDS properties even if it doesn't support it. Add a
> > > check to skip that part of the code if TCON doesn't support channel 0.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > 
> > I have already sent a similar patch here:
> > https://lists.freedesktop.org/archives/dri-devel/2018-February/166665.html
> 
> Right. However, check last chunk in my patch. There is no need to call 
> sun4i_rgb_init() if TCON doesn't support channel 0. It doesn't do anything, 
> except producing warning. Will you add that this change to your patch and then 
> I can remove this patch from next revision?

They are orthogonal to me though. Mine fixes the spurious LVDS error
messages that you were mentionning in your commit log. Your point here
is that we shouldn't need to even register the LVDS and RGB output
when there's no channel 0. This is obviously true, but it should be in
a separate patch.

> BTW, your patch won't apply cleanly, since you didn't base it on latest code 
> (every TCON variant has at least one entry now).

I'll fix it when applying.

Maxime
diff mbox

Patch

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index 0d6c5ed44795..3fae4f6e695a 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -850,7 +850,7 @@  static int sun4i_tcon_bind(struct device *dev, struct device *master,
 	struct sunxi_engine *engine;
 	struct device_node *remote;
 	struct sun4i_tcon *tcon;
-	bool has_lvds_rst, has_lvds_alt, can_lvds;
+	bool has_lvds_rst, has_lvds_alt, can_lvds = false;
 	int ret;
 
 	engine = sun4i_tcon_find_engine(drv, dev->of_node);
@@ -881,52 +881,56 @@  static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		return ret;
 	}
 
-	/*
-	 * This can only be made optional since we've had DT nodes
-	 * without the LVDS reset properties.
-	 *
-	 * If the property is missing, just disable LVDS, and print a
-	 * warning.
-	 */
-	tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds");
-	if (IS_ERR(tcon->lvds_rst)) {
-		dev_err(dev, "Couldn't get our reset line\n");
-		return PTR_ERR(tcon->lvds_rst);
-	} else if (tcon->lvds_rst) {
-		has_lvds_rst = true;
-		reset_control_reset(tcon->lvds_rst);
-	} else {
-		has_lvds_rst = false;
-	}
+	if (tcon->quirks->has_channel_0) {
+		/*
+		 * This can only be made optional since we've had DT nodes
+		 * without the LVDS reset properties.
+		 *
+		 * If the property is missing, just disable LVDS, and print a
+		 * warning.
+		 */
+		tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds");
+		if (IS_ERR(tcon->lvds_rst)) {
+			dev_err(dev, "Couldn't get our reset line\n");
+			return PTR_ERR(tcon->lvds_rst);
+		} else if (tcon->lvds_rst) {
+			has_lvds_rst = true;
+			reset_control_reset(tcon->lvds_rst);
+		} else {
+			has_lvds_rst = false;
+		}
 
-	/*
-	 * This can only be made optional since we've had DT nodes
-	 * without the LVDS reset properties.
-	 *
-	 * If the property is missing, just disable LVDS, and print a
-	 * warning.
-	 */
-	if (tcon->quirks->has_lvds_alt) {
-		tcon->lvds_pll = devm_clk_get(dev, "lvds-alt");
-		if (IS_ERR(tcon->lvds_pll)) {
-			if (PTR_ERR(tcon->lvds_pll) == -ENOENT) {
-				has_lvds_alt = false;
+		/*
+		 * This can only be made optional since we've had DT nodes
+		 * without the LVDS reset properties.
+		 *
+		 * If the property is missing, just disable LVDS, and print a
+		 * warning.
+		 */
+		if (tcon->quirks->has_lvds_alt) {
+			tcon->lvds_pll = devm_clk_get(dev, "lvds-alt");
+			if (IS_ERR(tcon->lvds_pll)) {
+				if (PTR_ERR(tcon->lvds_pll) == -ENOENT) {
+					has_lvds_alt = false;
+				} else {
+					dev_err(dev,
+						"Couldn't get the LVDS PLL\n");
+					return PTR_ERR(tcon->lvds_pll);
+				}
 			} else {
-				dev_err(dev, "Couldn't get the LVDS PLL\n");
-				return PTR_ERR(tcon->lvds_pll);
+				has_lvds_alt = true;
 			}
-		} else {
-			has_lvds_alt = true;
 		}
-	}
 
-	if (!has_lvds_rst || (tcon->quirks->has_lvds_alt && !has_lvds_alt)) {
-		dev_warn(dev,
-			 "Missing LVDS properties, Please upgrade your DT\n");
-		dev_warn(dev, "LVDS output disabled\n");
-		can_lvds = false;
-	} else {
-		can_lvds = true;
+		if (!has_lvds_rst ||
+		    (tcon->quirks->has_lvds_alt && !has_lvds_alt)) {
+			dev_warn(dev,
+				 "Missing LVDS properties, Please upgrade your DT\n");
+			dev_warn(dev, "LVDS output disabled\n");
+			can_lvds = false;
+		} else {
+			can_lvds = true;
+		}
 	}
 
 	ret = sun4i_tcon_init_clocks(dev, tcon);
@@ -962,23 +966,25 @@  static int sun4i_tcon_bind(struct device *dev, struct device *master,
 		goto err_free_dotclock;
 	}
 
-	/*
-	 * If we have an LVDS panel connected to the TCON, we should
-	 * just probe the LVDS connector. Otherwise, just probe RGB as
-	 * we used to.
-	 */
-	remote = of_graph_get_remote_node(dev->of_node, 1, 0);
-	if (of_device_is_compatible(remote, "panel-lvds"))
-		if (can_lvds)
-			ret = sun4i_lvds_init(drm, tcon);
+	if (tcon->quirks->has_channel_0) {
+		/*
+		 * If we have an LVDS panel connected to the TCON, we should
+		 * just probe the LVDS connector. Otherwise, just probe RGB as
+		 * we used to.
+		 */
+		remote = of_graph_get_remote_node(dev->of_node, 1, 0);
+		if (of_device_is_compatible(remote, "panel-lvds"))
+			if (can_lvds)
+				ret = sun4i_lvds_init(drm, tcon);
+			else
+				ret = -EINVAL;
 		else
-			ret = -EINVAL;
-	else
-		ret = sun4i_rgb_init(drm, tcon);
-	of_node_put(remote);
+			ret = sun4i_rgb_init(drm, tcon);
+		of_node_put(remote);
 
-	if (ret < 0)
-		goto err_free_dotclock;
+		if (ret < 0)
+			goto err_free_dotclock;
+	}
 
 	if (tcon->quirks->needs_de_be_mux) {
 		/*