diff mbox series

[v5,5/6] drm: sun4i: Add support for enabling DDC I2C bus to sun8i_dw_hdmi glue

Message ID 20190520235009.16734-6-megous@megous.com
State Changes Requested
Delegated to: David Miller
Headers show
Series Add support for Orange Pi 3 | expand

Commit Message

Ondřej Jirman May 20, 2019, 11:50 p.m. UTC 
From: Ondrej Jirman <megous@megous.com>

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 <megous@megous.com>
---
 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(-)

Comments

Maxime Ripard May 21, 2019, 11:46 a.m. UTC | #1 
Hi,

On Tue, May 21, 2019 at 01:50:08AM +0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> 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 <megous@megous.com>
> ---
>  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)
> +{
> +	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);
> +		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);
> +

Do you really need this to be done all the time? I'm guessing you
would only need this when running .get_modes, right?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Ondřej Jirman May 21, 2019, 12:15 p.m. UTC | #2 
Hi Maxime,

On Tue, May 21, 2019 at 01:46:11PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Tue, May 21, 2019 at 01:50:08AM +0200, megous@megous.com wrote:
> > From: Ondrej Jirman <megous@megous.com>
> >
> > 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 <megous@megous.com>
> > ---
> >  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)
> > +{
> > +	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);
> > +		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);
> > +
> 
> Do you really need this to be done all the time? I'm guessing you
> would only need this when running .get_modes, right?

I don't think it hurts anything. Enabled voltage shifting circuit doesn't
draw any current, unless DDC is actually transmitting data. On most boards
I'd imagine this circuit is always on anyway (Orange Pi 3 schematic even has
an option to tie this signal to VCC-IO instead of GPIO).

Schematic: https://megous.com/dl/tmp/bfcdd32d655aaa76.png

thank you and regards,
	o.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Maxime Ripard May 23, 2019, 12:27 p.m. UTC | #3 
On Tue, May 21, 2019 at 02:15:19PM +0200, Ondřej Jirman wrote:
> Hi Maxime,
>
> On Tue, May 21, 2019 at 01:46:11PM +0200, Maxime Ripard wrote:
> > Hi,
> >
> > On Tue, May 21, 2019 at 01:50:08AM +0200, megous@megous.com wrote:
> > > From: Ondrej Jirman <megous@megous.com>
> > >
> > > 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 <megous@megous.com>
> > > ---
> > >  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)
> > > +{
> > > +	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);
> > > +		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);
> > > +
> >
> > Do you really need this to be done all the time? I'm guessing you
> > would only need this when running .get_modes, right?
>
> I don't think it hurts anything. Enabled voltage shifting circuit doesn't
> draw any current, unless DDC is actually transmitting data. On most boards
> I'd imagine this circuit is always on anyway (Orange Pi 3 schematic even has
> an option to tie this signal to VCC-IO instead of GPIO).

Ok, it works for me then

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
diff mbox series

Patch

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)
+{
+	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);
+		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);
+
 	ret = reset_control_deassert(hdmi->rst_ctrl);
 	if (ret) {
 		dev_err(dev, "Could not deassert ctrl reset control\n");
-		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;
+	struct gpio_desc		*ddc_en;
 };
 
 static inline struct sun8i_dw_hdmi *