Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/6 v2] drm/msm/mdp4: Drop unused GPIO include
@ 2019-08-23  7:34 Linus Walleij
  2019-08-23  7:34 ` [PATCH 2/6 v2] drm/msm/dsi: Drop unused GPIO includes Linus Walleij
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Linus Walleij @ 2019-08-23  7:34 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, linux-arm-msm, freedreno
  Cc: dri-devel, Linus Walleij, Brian Masney

This file is not using any symbols from <linux/gpio.h> so just
drop this include.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Reviewed-by: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebased on v5.3-rc1
- Collected review tag
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
index ecef4f5b9f26..9262ed2dc8c3 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_connector.c
@@ -5,8 +5,6 @@
  * Author: Vinay Simha <vinaysimha@inforcecomputing.com>
  */
 
-#include <linux/gpio.h>
-
 #include "mdp4_kms.h"
 
 struct mdp4_lvds_connector {
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/6 v2] drm/msm/dsi: Drop unused GPIO includes
  2019-08-23  7:34 [PATCH 1/6 v2] drm/msm/mdp4: Drop unused GPIO include Linus Walleij
@ 2019-08-23  7:34 ` Linus Walleij
  2019-08-23  7:34 ` [PATCH 3/6 v2] drm/msm/dpu: Drop unused GPIO code Linus Walleij
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2019-08-23  7:34 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, linux-arm-msm, freedreno
  Cc: dri-devel, Linus Walleij, Brian Masney

This DSI driver uses the new descriptor API so these old
GPIO API includes are surplus.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Reviewed-by: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebased on v5.3-rc1
- Collected review tag
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index aa35d18ab43c..4b6f62138390 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -6,11 +6,9 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
-#include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
 #include <linux/of_device.h>
-#include <linux/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/of_graph.h>
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/6 v2] drm/msm/dpu: Drop unused GPIO code
  2019-08-23  7:34 [PATCH 1/6 v2] drm/msm/mdp4: Drop unused GPIO include Linus Walleij
  2019-08-23  7:34 ` [PATCH 2/6 v2] drm/msm/dsi: Drop unused GPIO includes Linus Walleij
@ 2019-08-23  7:34 ` Linus Walleij
  2019-08-23  7:34 ` [PATCH 4/6 v2] drm/msm/hdmi: Convert to use GPIO descriptors Linus Walleij
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2019-08-23  7:34 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, linux-arm-msm, freedreno
  Cc: dri-devel, Linus Walleij, Brian Masney

The DPU has some kind of idea that it wants to be able to
bring up power using GPIO lines. The struct dss_gpio is however
completely unused and should this be done, it should be done
using the GPIO descriptor framework rather than this API
which relies on the global GPIO numberspace. Delete this
code before anyone hurt themselves.

The inclusion of <linux/gpio.h> was abused to get some OF
and IRQ headers implicitly included into the DPU utilities,
make these includes explicit and push them down into the actual
implementation.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Reviewed-by: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebased on v5.3-rc1
- Collected review tag
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c | 1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h | 9 ---------
 drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c    | 4 ++++
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
index 71b6987bff1e..27fbeb504362 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.c
@@ -7,6 +7,7 @@
 #include <linux/clk/clk-conf.h>
 #include <linux/err.h>
 #include <linux/delay.h>
+#include <linux/of.h>
 
 #include <drm/drm_print.h>
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
index 09083e9f06bb..e6b5c772fa3b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_io_util.h
@@ -5,7 +5,6 @@
 #ifndef __DPU_IO_UTIL_H__
 #define __DPU_IO_UTIL_H__
 
-#include <linux/gpio.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
@@ -14,12 +13,6 @@
 #define DEV_WARN(fmt, args...)  pr_warn(fmt, ##args)
 #define DEV_ERR(fmt, args...)   pr_err(fmt, ##args)
 
-struct dss_gpio {
-	unsigned int gpio;
-	unsigned int value;
-	char gpio_name[32];
-};
-
 enum dss_clk_type {
 	DSS_CLK_AHB, /* no set rate. rate controlled through rpm */
 	DSS_CLK_PCLK,
@@ -34,8 +27,6 @@ struct dss_clk {
 };
 
 struct dss_module_power {
-	unsigned int num_gpio;
-	struct dss_gpio *gpio_config;
 	unsigned int num_clk;
 	struct dss_clk *clk_config;
 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index 986915bbbc02..c977baddfffd 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -3,6 +3,10 @@
  * Copyright (c) 2018, The Linux Foundation
  */
 
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqdesc.h>
+#include <linux/irqchip/chained_irq.h>
 #include "dpu_kms.h"
 #include <linux/interconnect.h>
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 4/6 v2] drm/msm/hdmi: Convert to use GPIO descriptors
  2019-08-23  7:34 [PATCH 1/6 v2] drm/msm/mdp4: Drop unused GPIO include Linus Walleij
  2019-08-23  7:34 ` [PATCH 2/6 v2] drm/msm/dsi: Drop unused GPIO includes Linus Walleij
  2019-08-23  7:34 ` [PATCH 3/6 v2] drm/msm/dpu: Drop unused GPIO code Linus Walleij
@ 2019-08-23  7:34 ` Linus Walleij
  2019-08-23  7:34 ` [PATCH 5/6 v2] drm/msm/hdmi: Bring up HDMI connector OFF Linus Walleij
  2019-08-23  7:34 ` [PATCH 6/6 v2] drm/msm/hdmi: Do not initialize HPD line value Linus Walleij
  4 siblings, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2019-08-23  7:34 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, linux-arm-msm, freedreno
  Cc: dri-devel, Linus Walleij, Brian Masney

This switches the MSM HDMI code to use GPIO descriptors.
Normally we would fetch the GPIOs from the device with the
flags GPIOD_IN or GPIOD_OUT_[LOW|HIGH] to set up the lines
immediately, but since the code seems eager to actively
drive the lines high/low when turning HDMI on and off, we
just fetch the GPIOs as-is and keep the code explicitly
driving them.

The old code would try legacy bindings (GPIOs without any
"-gpios" suffix) but this has been moved to the gpiolib
as a quirk by the previous patch.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Reviewed-by: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebased on v5.3-rc1
- Collected review tag
---
 drivers/gpu/drm/msm/hdmi/hdmi.c           | 66 +++++++++++------------
 drivers/gpu/drm/msm/hdmi/hdmi.h           |  4 +-
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 42 +++++----------
 3 files changed, 45 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 0e4217be3f00..355afb936401 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -425,38 +425,6 @@ static const struct {
 	{ "qcom,hdmi-tx-mux-lpm", true, 1, "HDMI_MUX_LPM" },
 };
 
-static int msm_hdmi_get_gpio(struct device_node *of_node, const char *name)
-{
-	int gpio;
-
-	/* try with the gpio names as in the table (downstream bindings) */
-	gpio = of_get_named_gpio(of_node, name, 0);
-	if (gpio < 0) {
-		char name2[32];
-
-		/* try with the gpio names as in the upstream bindings */
-		snprintf(name2, sizeof(name2), "%s-gpios", name);
-		gpio = of_get_named_gpio(of_node, name2, 0);
-		if (gpio < 0) {
-			char name3[32];
-
-			/*
-			 * try again after stripping out the "qcom,hdmi-tx"
-			 * prefix. This is mainly to match "hpd-gpios" used
-			 * in the upstream bindings
-			 */
-			if (sscanf(name2, "qcom,hdmi-tx-%s", name3))
-				gpio = of_get_named_gpio(of_node, name3, 0);
-		}
-
-		if (gpio < 0) {
-			DBG("failed to get gpio: %s (%d)", name, gpio);
-			gpio = -1;
-		}
-	}
-	return gpio;
-}
-
 /*
  * HDMI audio codec callbacks
  */
@@ -582,11 +550,39 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 	hdmi_cfg->qfprom_mmio_name = "qfprom_physical";
 
 	for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
-		hdmi_cfg->gpios[i].num = msm_hdmi_get_gpio(of_node,
-						msm_hdmi_gpio_pdata[i].name);
+		const char *name = msm_hdmi_gpio_pdata[i].name;
+		struct gpio_desc *gpiod;
+
+		/*
+		 * We are fetching the GPIO lines "as is" since the connector
+		 * code is enabling and disabling the lines. Until that point
+		 * the power-on default value will be kept.
+		 */
+		gpiod = devm_gpiod_get_optional(dev, name, GPIOD_ASIS);
+		/* This will catch e.g. -PROBE_DEFER */
+		if (IS_ERR(gpiod))
+			return PTR_ERR(gpiod);
+		if (!gpiod) {
+			/* Try a second time, stripping down the name */
+			char name3[32];
+
+			/*
+			 * Try again after stripping out the "qcom,hdmi-tx"
+			 * prefix. This is mainly to match "hpd-gpios" used
+			 * in the upstream bindings.
+			 */
+			if (sscanf(name, "qcom,hdmi-tx-%s", name3))
+				gpiod = devm_gpiod_get_optional(dev, name3, GPIOD_ASIS);
+			if (IS_ERR(gpiod))
+				return PTR_ERR(gpiod);
+			if (!gpiod)
+				DBG("failed to get gpio: %s", name);
+		}
+		hdmi_cfg->gpios[i].gpiod = gpiod;
+		if (gpiod)
+			gpiod_set_consumer_name(gpiod, msm_hdmi_gpio_pdata[i].label);
 		hdmi_cfg->gpios[i].output = msm_hdmi_gpio_pdata[i].output;
 		hdmi_cfg->gpios[i].value = msm_hdmi_gpio_pdata[i].value;
-		hdmi_cfg->gpios[i].label = msm_hdmi_gpio_pdata[i].label;
 	}
 
 	dev->platform_data = hdmi_cfg;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 982865866a29..bdac452b00fb 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -11,6 +11,7 @@
 #include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/gpio/consumer.h>
 #include <linux/hdmi.h>
 
 #include "msm_drv.h"
@@ -22,10 +23,9 @@ struct hdmi_phy;
 struct hdmi_platform_config;
 
 struct hdmi_gpio_data {
-	int num;
+	struct gpio_desc *gpiod;
 	bool output;
 	int value;
-	const char *label;
 };
 
 struct hdmi_audio {
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index 07b4cb877d82..d0575d4f747d 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -4,7 +4,7 @@
  * Author: Rob Clark <robdclark@gmail.com>
  */
 
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/pinctrl/consumer.h>
 
 #include "msm_kms.h"
@@ -68,30 +68,21 @@ static void msm_hdmi_phy_reset(struct hdmi *hdmi)
 
 static int gpio_config(struct hdmi *hdmi, bool on)
 {
-	struct device *dev = &hdmi->pdev->dev;
 	const struct hdmi_platform_config *config = hdmi->config;
-	int ret, i;
+	int i;
 
 	if (on) {
 		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
-			if (gpio.num != -1) {
-				ret = gpio_request(gpio.num, gpio.label);
-				if (ret) {
-					DRM_DEV_ERROR(dev,
-						"'%s'(%d) gpio_request failed: %d\n",
-						gpio.label, gpio.num, ret);
-					goto err;
-				}
-
+			if (gpio.gpiod) {
 				if (gpio.output) {
-					gpio_direction_output(gpio.num,
-							      gpio.value);
+					gpiod_direction_output(gpio.gpiod,
+							       gpio.value);
 				} else {
-					gpio_direction_input(gpio.num);
-					gpio_set_value_cansleep(gpio.num,
-								gpio.value);
+					gpiod_direction_input(gpio.gpiod);
+					gpiod_set_value_cansleep(gpio.gpiod,
+								 gpio.value);
 				}
 			}
 		}
@@ -101,29 +92,20 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
-			if (gpio.num == -1)
+			if (!gpio.gpiod)
 				continue;
 
 			if (gpio.output) {
 				int value = gpio.value ? 0 : 1;
 
-				gpio_set_value_cansleep(gpio.num, value);
+				gpiod_set_value_cansleep(gpio.gpiod, value);
 			}
-
-			gpio_free(gpio.num);
 		};
 
 		DBG("gpio off");
 	}
 
 	return 0;
-err:
-	while (i--) {
-		if (config->gpios[i].num != -1)
-			gpio_free(config->gpios[i].num);
-	}
-
-	return ret;
 }
 
 static void enable_hpd_clocks(struct hdmi *hdmi, bool enable)
@@ -311,7 +293,7 @@ static enum drm_connector_status detect_gpio(struct hdmi *hdmi)
 	const struct hdmi_platform_config *config = hdmi->config;
 	struct hdmi_gpio_data hpd_gpio = config->gpios[HPD_GPIO_INDEX];
 
-	return gpio_get_value(hpd_gpio.num) ?
+	return gpiod_get_value(hpd_gpio.gpiod) ?
 			connector_status_connected :
 			connector_status_disconnected;
 }
@@ -330,7 +312,7 @@ static enum drm_connector_status hdmi_connector_detect(
 	 * some platforms may not have hpd gpio. Rely only on the status
 	 * provided by REG_HDMI_HPD_INT_STATUS in this case.
 	 */
-	if (hpd_gpio.num == -1)
+	if (!hpd_gpio.gpiod)
 		return detect_reg(hdmi);
 
 	do {
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 5/6 v2] drm/msm/hdmi: Bring up HDMI connector OFF
  2019-08-23  7:34 [PATCH 1/6 v2] drm/msm/mdp4: Drop unused GPIO include Linus Walleij
                   ` (2 preceding siblings ...)
  2019-08-23  7:34 ` [PATCH 4/6 v2] drm/msm/hdmi: Convert to use GPIO descriptors Linus Walleij
@ 2019-08-23  7:34 ` Linus Walleij
  2019-09-01 16:28   ` Rob Clark
  2019-08-23  7:34 ` [PATCH 6/6 v2] drm/msm/hdmi: Do not initialize HPD line value Linus Walleij
  4 siblings, 1 reply; 8+ messages in thread
From: Linus Walleij @ 2019-08-23  7:34 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, linux-arm-msm, freedreno
  Cc: dri-devel, Linus Walleij, Brian Masney

There is elaborate code in the HDMI connector handling to
leave the connector in the state it was at power-on and
only touch the GPIOs when the connector .enable() and
.disable() callbacks are called.

I don't think this is what we normally want, initialize
the connector as OFF (possibly saving power?) using the
appropriate GPIO descriptor flags. It will still be
switched on/off in the enable()/disable() connector
callback as before, but we can drop some strange surplus
code.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Reviewed-by: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebased on v5.3-rc1
- Collected review tag
---
 drivers/gpu/drm/msm/hdmi/hdmi.c           | 19 ++++++++++++-----
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 25 ++++++-----------------
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 355afb936401..5739eec65659 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -552,13 +552,22 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 	for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 		const char *name = msm_hdmi_gpio_pdata[i].name;
 		struct gpio_desc *gpiod;
+		enum gpiod_flags flags;
 
 		/*
-		 * We are fetching the GPIO lines "as is" since the connector
-		 * code is enabling and disabling the lines. Until that point
-		 * the power-on default value will be kept.
+		 * Notice the inverse set up here: we initialize the connector
+		 * to OFF state.
 		 */
-		gpiod = devm_gpiod_get_optional(dev, name, GPIOD_ASIS);
+		if (msm_hdmi_gpio_pdata[i].output) {
+			if (msm_hdmi_gpio_pdata[i].value)
+				flags = GPIOD_OUT_LOW;
+			else
+				flags = GPIOD_OUT_HIGH;
+		} else {
+			flags = GPIOD_IN;
+		}
+
+		gpiod = devm_gpiod_get_optional(dev, name, flags);
 		/* This will catch e.g. -PROBE_DEFER */
 		if (IS_ERR(gpiod))
 			return PTR_ERR(gpiod);
@@ -572,7 +581,7 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 			 * in the upstream bindings.
 			 */
 			if (sscanf(name, "qcom,hdmi-tx-%s", name3))
-				gpiod = devm_gpiod_get_optional(dev, name3, GPIOD_ASIS);
+				gpiod = devm_gpiod_get_optional(dev, name3, flags);
 			if (IS_ERR(gpiod))
 				return PTR_ERR(gpiod);
 			if (!gpiod)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index d0575d4f747d..f006682935e9 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -75,16 +75,9 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
-			if (gpio.gpiod) {
-				if (gpio.output) {
-					gpiod_direction_output(gpio.gpiod,
-							       gpio.value);
-				} else {
-					gpiod_direction_input(gpio.gpiod);
-					gpiod_set_value_cansleep(gpio.gpiod,
-								 gpio.value);
-				}
-			}
+			/* The value indicates the value for turning things on */
+			if (gpio.gpiod)
+				gpiod_set_value_cansleep(gpio.gpiod, gpio.value);
 		}
 
 		DBG("gpio on");
@@ -92,16 +85,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 		for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
-			if (!gpio.gpiod)
-				continue;
-
-			if (gpio.output) {
-				int value = gpio.value ? 0 : 1;
-
-				gpiod_set_value_cansleep(gpio.gpiod, value);
-			}
+			/* The inverse value turns stuff off */
+			if (gpio.gpiod && gpio.output)
+				gpiod_set_value_cansleep(gpio.gpiod, !gpio.value);
 		};
-
 		DBG("gpio off");
 	}
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 6/6 v2] drm/msm/hdmi: Do not initialize HPD line value
  2019-08-23  7:34 [PATCH 1/6 v2] drm/msm/mdp4: Drop unused GPIO include Linus Walleij
                   ` (3 preceding siblings ...)
  2019-08-23  7:34 ` [PATCH 5/6 v2] drm/msm/hdmi: Bring up HDMI connector OFF Linus Walleij
@ 2019-08-23  7:34 ` Linus Walleij
  4 siblings, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2019-08-23  7:34 UTC (permalink / raw)
  To: Rob Clark, Sean Paul, linux-arm-msm, freedreno
  Cc: dri-devel, Linus Walleij, Brian Masney

After untangling the MSM HDMI GPIO code we see that the code
is deliberately setting the output value of the HPD (hot plug
detect) line to high, even though it is being used as input
which is of course the only viable use of a HPD pin.

This seems dubious: GPIO lines set up as input will have high
impedance (tristate) and the typical electronic construction
involves this line being used with a pull-down resistor around
10KOhm to keep it low (this is sometimes part of a levelshifter
component) and then an inserted connector will pull it up to
VDD and this asserts the HPD signal, as can be seen from the
code reading the HPD GPIO.

Stop try driving a value to the HPD input GPIO.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Reviewed-by: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebased on v5.3-rc1
- Collected review tag
---
 drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
index f006682935e9..bb1c49e3c9dd 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
@@ -76,7 +76,7 @@ static int gpio_config(struct hdmi *hdmi, bool on)
 			struct hdmi_gpio_data gpio = config->gpios[i];
 
 			/* The value indicates the value for turning things on */
-			if (gpio.gpiod)
+			if (gpio.gpiod && gpio.output)
 				gpiod_set_value_cansleep(gpio.gpiod, gpio.value);
 		}
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 5/6 v2] drm/msm/hdmi: Bring up HDMI connector OFF
  2019-08-23  7:34 ` [PATCH 5/6 v2] drm/msm/hdmi: Bring up HDMI connector OFF Linus Walleij
@ 2019-09-01 16:28   ` Rob Clark
  2019-09-02 13:01     ` Linus Walleij
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2019-09-01 16:28 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Sean Paul, linux-arm-msm, freedreno, dri-devel, Brian Masney

On Fri, Aug 23, 2019 at 12:35 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> There is elaborate code in the HDMI connector handling to
> leave the connector in the state it was at power-on and
> only touch the GPIOs when the connector .enable() and
> .disable() callbacks are called.
>
> I don't think this is what we normally want, initialize
> the connector as OFF (possibly saving power?) using the
> appropriate GPIO descriptor flags. It will still be
> switched on/off in the enable()/disable() connector
> callback as before, but we can drop some strange surplus
> code.

I'm picking up the earlier patches in this series.. although I won't
have a good way to test on devices which use the hdmi block for a few
weeks (when I complete my move, and get back a bunch of boards that
are in boxes right now) so I'm going to wait on the last two I think.

When we get to dealing with devices with hdmi enabled by bootloader,
we may actually want to preserve the pre-probe state of the GPIOs.
Although maybe not worth worrying about until we can handle DSI
enabled at boot.

BR,
-R

>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Reviewed-by: Brian Masney <masneyb@onstation.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Rebased on v5.3-rc1
> - Collected review tag
> ---
>  drivers/gpu/drm/msm/hdmi/hdmi.c           | 19 ++++++++++++-----
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 25 ++++++-----------------
>  2 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 355afb936401..5739eec65659 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -552,13 +552,22 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
>         for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>                 const char *name = msm_hdmi_gpio_pdata[i].name;
>                 struct gpio_desc *gpiod;
> +               enum gpiod_flags flags;
>
>                 /*
> -                * We are fetching the GPIO lines "as is" since the connector
> -                * code is enabling and disabling the lines. Until that point
> -                * the power-on default value will be kept.
> +                * Notice the inverse set up here: we initialize the connector
> +                * to OFF state.
>                  */
> -               gpiod = devm_gpiod_get_optional(dev, name, GPIOD_ASIS);
> +               if (msm_hdmi_gpio_pdata[i].output) {
> +                       if (msm_hdmi_gpio_pdata[i].value)
> +                               flags = GPIOD_OUT_LOW;
> +                       else
> +                               flags = GPIOD_OUT_HIGH;
> +               } else {
> +                       flags = GPIOD_IN;
> +               }
> +
> +               gpiod = devm_gpiod_get_optional(dev, name, flags);
>                 /* This will catch e.g. -PROBE_DEFER */
>                 if (IS_ERR(gpiod))
>                         return PTR_ERR(gpiod);
> @@ -572,7 +581,7 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
>                          * in the upstream bindings.
>                          */
>                         if (sscanf(name, "qcom,hdmi-tx-%s", name3))
> -                               gpiod = devm_gpiod_get_optional(dev, name3, GPIOD_ASIS);
> +                               gpiod = devm_gpiod_get_optional(dev, name3, flags);
>                         if (IS_ERR(gpiod))
>                                 return PTR_ERR(gpiod);
>                         if (!gpiod)
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> index d0575d4f747d..f006682935e9 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c
> @@ -75,16 +75,9 @@ static int gpio_config(struct hdmi *hdmi, bool on)
>                 for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>                         struct hdmi_gpio_data gpio = config->gpios[i];
>
> -                       if (gpio.gpiod) {
> -                               if (gpio.output) {
> -                                       gpiod_direction_output(gpio.gpiod,
> -                                                              gpio.value);
> -                               } else {
> -                                       gpiod_direction_input(gpio.gpiod);
> -                                       gpiod_set_value_cansleep(gpio.gpiod,
> -                                                                gpio.value);
> -                               }
> -                       }
> +                       /* The value indicates the value for turning things on */
> +                       if (gpio.gpiod)
> +                               gpiod_set_value_cansleep(gpio.gpiod, gpio.value);
>                 }
>
>                 DBG("gpio on");
> @@ -92,16 +85,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)
>                 for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) {
>                         struct hdmi_gpio_data gpio = config->gpios[i];
>
> -                       if (!gpio.gpiod)
> -                               continue;
> -
> -                       if (gpio.output) {
> -                               int value = gpio.value ? 0 : 1;
> -
> -                               gpiod_set_value_cansleep(gpio.gpiod, value);
> -                       }
> +                       /* The inverse value turns stuff off */
> +                       if (gpio.gpiod && gpio.output)
> +                               gpiod_set_value_cansleep(gpio.gpiod, !gpio.value);
>                 };
> -
>                 DBG("gpio off");
>         }
>
> --
> 2.21.0
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 5/6 v2] drm/msm/hdmi: Bring up HDMI connector OFF
  2019-09-01 16:28   ` Rob Clark
@ 2019-09-02 13:01     ` Linus Walleij
  0 siblings, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2019-09-02 13:01 UTC (permalink / raw)
  To: Rob Clark; +Cc: Sean Paul, linux-arm-msm, freedreno, dri-devel, Brian Masney

On Sun, Sep 1, 2019 at 6:28 PM Rob Clark <robdclark@gmail.com> wrote:

> I'm picking up the earlier patches in this series..

Thanks a lot :)

> although I won't
> have a good way to test on devices which use the hdmi block for a few
> weeks (when I complete my move, and get back a bunch of boards that
> are in boxes right now) so I'm going to wait on the last two I think.

That's fine. It is just occasional drive-by coding anyway.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23  7:34 [PATCH 1/6 v2] drm/msm/mdp4: Drop unused GPIO include Linus Walleij
2019-08-23  7:34 ` [PATCH 2/6 v2] drm/msm/dsi: Drop unused GPIO includes Linus Walleij
2019-08-23  7:34 ` [PATCH 3/6 v2] drm/msm/dpu: Drop unused GPIO code Linus Walleij
2019-08-23  7:34 ` [PATCH 4/6 v2] drm/msm/hdmi: Convert to use GPIO descriptors Linus Walleij
2019-08-23  7:34 ` [PATCH 5/6 v2] drm/msm/hdmi: Bring up HDMI connector OFF Linus Walleij
2019-09-01 16:28   ` Rob Clark
2019-09-02 13:01     ` Linus Walleij
2019-08-23  7:34 ` [PATCH 6/6 v2] drm/msm/hdmi: Do not initialize HPD line value Linus Walleij

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox