Hi Jernej,

On Sun, Jun 16, 2019 at 01:05:13PM +0200, Jernej Škrabec wrote:

Hi Ondrej!

Dne ponedeljek, 27. maj 2019 ob 18:22:36 CEST je megous via linux-sunxi 
napisal(a):

quoted

From: Ondrej Jirman <redacted>

Orange Pi 3 board requires enabling a voltage shifting circuit via GPIO
for the DDC bus to be usable.

Add support for hdmi-connector node's optional ddc-en-gpios property to
support this use case.

Signed-off-by: Ondrej Jirman <redacted>
---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 55 +++++++++++++++++++++++++--
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h |  3 ++
 2 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c

b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index 39d8509d96a0..59b81ba02d96
100644

--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c

@@ -98,6 +98,30 @@ static u32 sun8i_dw_hdmi_find_possible_crtcs(struct

drm_device *drm, return crtcs;
 }

+static int sun8i_dw_hdmi_find_connector_pdev(struct device *dev,
+					     struct 

platform_device **pdev_out)

quoted

+{
+	struct platform_device *pdev;
+	struct device_node *remote;
+
+	remote = of_graph_get_remote_node(dev->of_node, 1, -1);
+	if (!remote)
+		return -ENODEV;
+
+	if (!of_device_is_compatible(remote, "hdmi-connector")) {
+		of_node_put(remote);
+		return -ENODEV;
+	}
+
+	pdev = of_find_device_by_node(remote);
+	of_node_put(remote);
+	if (!pdev)
+		return -ENODEV;
+
+	*pdev_out = pdev;
+	return 0;
+}
+
 static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 			      void *data)
 {

@@ -151,16 +175,29 @@ static int sun8i_dw_hdmi_bind(struct device *dev,

struct device *master, return PTR_ERR(hdmi->regulator);
 	}

+	ret = sun8i_dw_hdmi_find_connector_pdev(dev, &hdmi->connector_pdev);
+	if (!ret) {
+		hdmi->ddc_en = gpiod_get_optional(&hdmi->connector_pdev-
dev,
+						  "ddc-en", 

GPIOD_OUT_HIGH);

quoted

+		if (IS_ERR(hdmi->ddc_en)) {
+			platform_device_put(hdmi->connector_pdev);
+			dev_err(dev, "Couldn't get ddc-en gpio\n");
+			return PTR_ERR(hdmi->ddc_en);
+		}
+	}
+
 	ret = regulator_enable(hdmi->regulator);
 	if (ret) {
 		dev_err(dev, "Failed to enable regulator\n");
-		return ret;
+		goto err_unref_ddc_en;
 	}

+	gpiod_set_value(hdmi->ddc_en, 1);

Why don't you do that inside if clause where hdmi->ddc_en is assigned? It's 
not useful otherwise anyway.

Besides, you would then only need to adjust one goto label in error path.

The idea is to enable DDC after enabling the regulator. I don't think it matters
for the particular HW that's on Orange Pi 3, and similar Xunlong boards, but
this is a fairly generic binding and it makes more sense to power the bus, and
then enable whatever aditional circuitry might be there for the IO.

I can move sun8i_dw_hdmi_find_connector_pdev lower, but I would then need to
disable the regulator in the error path, and I like to keep this order:

- parsing DT
- enabling actual HW stuff

Because parsing is likely to fail with DEFERED_PROBE, because GPIO or whatever
else is not yet ready, and this approach avoids enabling/disabling the HW
needlessly.

quoted

+
 	ret = reset_control_deassert(hdmi->rst_ctrl);
 	if (ret) {
 		dev_err(dev, "Could not deassert ctrl reset 

control\n");

quoted

-		goto err_disable_regulator;
+		goto err_disable_ddc_en;
 	}

 	ret = clk_prepare_enable(hdmi->clk_tmds);

@@ -213,8 +250,14 @@ static int sun8i_dw_hdmi_bind(struct device *dev,

struct device *master, clk_disable_unprepare(hdmi->clk_tmds);
 err_assert_ctrl_reset:
 	reset_control_assert(hdmi->rst_ctrl);
-err_disable_regulator:
+err_disable_ddc_en:
+	gpiod_set_value(hdmi->ddc_en, 0);
 	regulator_disable(hdmi->regulator);
+err_unref_ddc_en:
+	if (hdmi->ddc_en)
+		gpiod_put(hdmi->ddc_en);
+
+	platform_device_put(hdmi->connector_pdev);

 	return ret;
 }

@@ -228,7 +271,13 @@ static void sun8i_dw_hdmi_unbind(struct device *dev,

struct device *master, sun8i_hdmi_phy_remove(hdmi);
 	clk_disable_unprepare(hdmi->clk_tmds);
 	reset_control_assert(hdmi->rst_ctrl);
+	gpiod_set_value(hdmi->ddc_en, 0);
 	regulator_disable(hdmi->regulator);
+
+	if (hdmi->ddc_en)
+		gpiod_put(hdmi->ddc_en);
+
+	platform_device_put(hdmi->connector_pdev);
 }

 static const struct component_ops sun8i_dw_hdmi_ops = {

diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h

b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 720c5aa8adc1..dad66b8301c2
100644

--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h

@@ -9,6 +9,7 @@
 #include <drm/bridge/dw_hdmi.h>
 #include <drm/drm_encoder.h>
 #include <linux/clk.h>
+#include <linux/gpio/consumer.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>

@@ -190,6 +191,8 @@ struct sun8i_dw_hdmi {
 	struct regulator		*regulator;
 	const struct sun8i_dw_hdmi_quirks *quirks;
 	struct reset_control		*rst_ctrl;
+	struct platform_device		*connector_pdev;

It seems that connector_pdev is needed only during intialization. Why do you 
store it?

For some reason I thought that I need to keep it to keep the GPIO available,
but that's not true. I'll drop it.

thank you,
	Ondrej

Best regards,
Jernej

quoted

+	struct gpio_desc		*ddc_en;
 };

 static inline struct sun8i_dw_hdmi *