diff mbox series

[02/12] drm: sun4i: Add support for enabling DDC I2C bus to dw_hdmi glue

Message ID 20190405234514.6183-3-megous@megous.com (mailing list archive)
State New, archived
Headers show
Series Add support for Orange Pi 3 | expand

Commit Message

Ondřej Jirman April 5, 2019, 11:45 p.m. UTC
From: Ondrej Jirman <megous@megous.com>

Orange Pi 3 board requires enabling DDC I2C bus via some GPIO connected
transistors, before it can be used. Model this as a power supply for DDC
(via regulator framework).

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 17 ++++++++++++++++-
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Maxime Ripard April 8, 2019, 7:23 a.m. UTC | #1
On Sat, Apr 06, 2019 at 01:45:04AM +0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> Orange Pi 3 board requires enabling DDC I2C bus via some GPIO connected
> transistors, before it can be used. Model this as a power supply for DDC
> (via regulator framework).
>
> Signed-off-by: Ondrej Jirman <megous@megous.com>

The DDC bus itself is usually attached to the HDMI connector, so it
would make sense to make the supply also a property of the connector.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Chen-Yu Tsai April 8, 2019, 7:28 a.m. UTC | #2
On Mon, Apr 8, 2019 at 3:23 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Sat, Apr 06, 2019 at 01:45:04AM +0200, megous@megous.com wrote:
> > From: Ondrej Jirman <megous@megous.com>
> >
> > Orange Pi 3 board requires enabling DDC I2C bus via some GPIO connected
> > transistors, before it can be used. Model this as a power supply for DDC
> > (via regulator framework).
> >
> > Signed-off-by: Ondrej Jirman <megous@megous.com>
>
> The DDC bus itself is usually attached to the HDMI connector, so it
> would make sense to make the supply also a property of the connector.

I believe these are separate things. What this patch covers is power for
a voltage shifter between the SoC and HDMI DDC pins. The HDMI connector's
5V supply to power the remote DDC chip is something else. And on the
Orange Pi 3 they are indeed separate supplies.

ChenYu
Maxime Ripard April 8, 2019, 8:47 a.m. UTC | #3
On Mon, Apr 08, 2019 at 03:28:24PM +0800, Chen-Yu Tsai wrote:
> On Mon, Apr 8, 2019 at 3:23 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Sat, Apr 06, 2019 at 01:45:04AM +0200, megous@megous.com wrote:
> > > From: Ondrej Jirman <megous@megous.com>
> > >
> > > Orange Pi 3 board requires enabling DDC I2C bus via some GPIO connected
> > > transistors, before it can be used. Model this as a power supply for DDC
> > > (via regulator framework).
> > >
> > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> >
> > The DDC bus itself is usually attached to the HDMI connector, so it
> > would make sense to make the supply also a property of the connector.
>
> I believe these are separate things. What this patch covers is power for
> a voltage shifter between the SoC and HDMI DDC pins. The HDMI connector's
> 5V supply to power the remote DDC chip is something else. And on the
> Orange Pi 3 they are indeed separate supplies.

Then maybe the endpoint link between the two would be the best place
to put this?

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Ondřej Jirman April 8, 2019, 12:17 p.m. UTC | #4
On Mon, Apr 08, 2019 at 10:47:14AM +0200, Maxime Ripard wrote:
> On Mon, Apr 08, 2019 at 03:28:24PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Apr 8, 2019 at 3:23 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Sat, Apr 06, 2019 at 01:45:04AM +0200, megous@megous.com wrote:
> > > > From: Ondrej Jirman <megous@megous.com>
> > > >
> > > > Orange Pi 3 board requires enabling DDC I2C bus via some GPIO connected
> > > > transistors, before it can be used. Model this as a power supply for DDC
> > > > (via regulator framework).
> > > >
> > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > >
> > > The DDC bus itself is usually attached to the HDMI connector, so it
> > > would make sense to make the supply also a property of the connector.
> >
> > I believe these are separate things. What this patch covers is power for
> > a voltage shifter between the SoC and HDMI DDC pins. The HDMI connector's
> > 5V supply to power the remote DDC chip is something else. And on the
> > Orange Pi 3 they are indeed separate supplies.
> 
> Then maybe the endpoint link between the two would be the best place
> to put this?

Interestingly &hdmi node configures the DDC bus pins via pinctrl on the SoC
side, so I put this there too, because it's related to those pins. I'm not sure
if that changes anything in the discussion.

regards,
   o.

> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Maxime Ripard April 9, 2019, 7:45 a.m. UTC | #5
On Mon, Apr 08, 2019 at 02:17:27PM +0200, Ondřej Jirman wrote:
> On Mon, Apr 08, 2019 at 10:47:14AM +0200, Maxime Ripard wrote:
> > On Mon, Apr 08, 2019 at 03:28:24PM +0800, Chen-Yu Tsai wrote:
> > > On Mon, Apr 8, 2019 at 3:23 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Sat, Apr 06, 2019 at 01:45:04AM +0200, megous@megous.com wrote:
> > > > > From: Ondrej Jirman <megous@megous.com>
> > > > >
> > > > > Orange Pi 3 board requires enabling DDC I2C bus via some GPIO connected
> > > > > transistors, before it can be used. Model this as a power supply for DDC
> > > > > (via regulator framework).
> > > > >
> > > > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > >
> > > > The DDC bus itself is usually attached to the HDMI connector, so it
> > > > would make sense to make the supply also a property of the connector.
> > >
> > > I believe these are separate things. What this patch covers is power for
> > > a voltage shifter between the SoC and HDMI DDC pins. The HDMI connector's
> > > 5V supply to power the remote DDC chip is something else. And on the
> > > Orange Pi 3 they are indeed separate supplies.
> >
> > Then maybe the endpoint link between the two would be the best place
> > to put this?
>
> Interestingly &hdmi node configures the DDC bus pins via pinctrl on the SoC
> side, so I put this there too, because it's related to those pins. I'm not sure
> if that changes anything in the discussion.

It's kind of different though. The DDC controller is within the HDMI
controller, which is inside the SoC. Just like the pin muxer. As far
as the hardware goes, even on your board, you don't need that supply
so that the signal gets out of the SoC.

If the regulator is to power up some component on the path between the
SoC and the connector, then it should be attached there.

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 dc47720c99ba..a1518525fa2f 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
@@ -146,16 +146,28 @@  static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
 		return PTR_ERR(hdmi->regulator);
 	}
 
+	hdmi->ddc_regulator = devm_regulator_get(dev, "ddc");
+	if (IS_ERR(hdmi->ddc_regulator)) {
+		dev_err(dev, "Couldn't get ddc regulator\n");
+		return PTR_ERR(hdmi->ddc_regulator);
+	}
+
 	ret = regulator_enable(hdmi->regulator);
 	if (ret) {
 		dev_err(dev, "Failed to enable regulator\n");
 		return ret;
 	}
 
+	ret = regulator_enable(hdmi->ddc_regulator);
+	if (ret) {
+		dev_err(dev, "Failed to enable ddc regulator\n");
+		goto err_disable_regulator;
+	}
+
 	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_regulator;
 	}
 
 	ret = clk_prepare_enable(hdmi->clk_tmds);
@@ -208,6 +220,8 @@  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_ddc_regulator:
+	regulator_disable(hdmi->ddc_regulator);
 err_disable_regulator:
 	regulator_disable(hdmi->regulator);
 
@@ -223,6 +237,7 @@  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);
+	regulator_disable(hdmi->ddc_regulator);
 	regulator_disable(hdmi->regulator);
 }
 
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
index 720c5aa8adc1..6e93d55560b6 100644
--- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
+++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h
@@ -188,6 +188,7 @@  struct sun8i_dw_hdmi {
 	struct sun8i_hdmi_phy		*phy;
 	struct dw_hdmi_plat_data	plat_data;
 	struct regulator		*regulator;
+	struct regulator		*ddc_regulator;
 	const struct sun8i_dw_hdmi_quirks *quirks;
 	struct reset_control		*rst_ctrl;
 };