diff mbox

[4/8] OMAPDSS: HDMI: use vdda_hdmi_dac

Message ID 1345729514-2441-5-git-send-email-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomi Valkeinen Aug. 23, 2012, 1:45 p.m. UTC
The HDMI driver requires vdda_hdmi_dac power for operation, but does not
enable it. This has worked because the regulator has been always
enabled.

But this may not always be the case, as I encountered when implementing
HDMI device tree support.

This patch changes the HDMI driver to use the vdda_hdmi_dac.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/hdmi.c |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Archit Taneja Aug. 24, 2012, 6:20 a.m. UTC | #1
On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> The HDMI driver requires vdda_hdmi_dac power for operation, but does not
> enable it. This has worked because the regulator has been always
> enabled.
>
> But this may not always be the case, as I encountered when implementing
> HDMI device tree support.
>
> This patch changes the HDMI driver to use the vdda_hdmi_dac.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/dss/hdmi.c |   23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
>
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 96a6e29..ccfc677 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -33,6 +33,7 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/clk.h>
>   #include <linux/gpio.h>
> +#include <linux/regulator/consumer.h>
>   #include <video/omapdss.h>
>
>   #include "ti_hdmi.h"
> @@ -62,6 +63,7 @@ static struct {
>   	struct hdmi_ip_data ip_data;
>
>   	struct clk *sys_clk;
> +	struct regulator *vdda_hdmi_dac_reg;
>
>   	int ct_cp_hpd_gpio;
>   	int ls_oe_gpio;
> @@ -331,6 +333,19 @@ static int __init hdmi_init_display(struct omap_dss_device *dssdev)
>
>   	dss_init_hdmi_ip_ops(&hdmi.ip_data);
>
> +	if (hdmi.vdda_hdmi_dac_reg == NULL) {
> +		struct regulator *reg;
> +
> +		reg = devm_regulator_get(&hdmi.pdev->dev, "vdda_hdmi_dac");

There is no corresponding devm_regulator_put() call here, I guess that's 
what devm_* calls are supposed to help us with. But the only place I saw 
the usage of dev_regulator_get() is here:

sound/soc/ux500/ux500_msp_dai.c

And here, they are doing devm_regulator_put() calls to, so I was 
wondering what the deal is.

Archit

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Aug. 24, 2012, 6:41 a.m. UTC | #2
On Fri, 2012-08-24 at 11:50 +0530, Archit Taneja wrote:
> On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> > The HDMI driver requires vdda_hdmi_dac power for operation, but does not
> > enable it. This has worked because the regulator has been always
> > enabled.
> >
> > But this may not always be the case, as I encountered when implementing
> > HDMI device tree support.
> >
> > This patch changes the HDMI driver to use the vdda_hdmi_dac.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> >   drivers/video/omap2/dss/hdmi.c |   23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> > index 96a6e29..ccfc677 100644
> > --- a/drivers/video/omap2/dss/hdmi.c
> > +++ b/drivers/video/omap2/dss/hdmi.c
> > @@ -33,6 +33,7 @@
> >   #include <linux/pm_runtime.h>
> >   #include <linux/clk.h>
> >   #include <linux/gpio.h>
> > +#include <linux/regulator/consumer.h>
> >   #include <video/omapdss.h>
> >
> >   #include "ti_hdmi.h"
> > @@ -62,6 +63,7 @@ static struct {
> >   	struct hdmi_ip_data ip_data;
> >
> >   	struct clk *sys_clk;
> > +	struct regulator *vdda_hdmi_dac_reg;
> >
> >   	int ct_cp_hpd_gpio;
> >   	int ls_oe_gpio;
> > @@ -331,6 +333,19 @@ static int __init hdmi_init_display(struct omap_dss_device *dssdev)
> >
> >   	dss_init_hdmi_ip_ops(&hdmi.ip_data);
> >
> > +	if (hdmi.vdda_hdmi_dac_reg == NULL) {
> > +		struct regulator *reg;
> > +
> > +		reg = devm_regulator_get(&hdmi.pdev->dev, "vdda_hdmi_dac");
> 
> There is no corresponding devm_regulator_put() call here, I guess that's 
> what devm_* calls are supposed to help us with. But the only place I saw 
> the usage of dev_regulator_get() is here:
> 
> sound/soc/ux500/ux500_msp_dai.c
> 
> And here, they are doing devm_regulator_put() calls to, so I was 
> wondering what the deal is.

No idea. But there are other places also: sound/soc/codecs/wm5100.c,
sound/soc/codecs/wm8996.c, sound/soc/soc-dapm.c, and those don't use
put().

Anyway, I think it's quite clear how devm_* functions are used, so the
put call in ux500_msp_dai.c is probably just a mistake.

I also want to mention that doing the regulator_get in
hdmi_init_display() is somewhat strange, but the point is to do the
regulator_get only if a hdmi display has been defined in the board file.
If we did it unconditionally in hdmi's probe, we'd try to get the
regulator always, and it could be that there's no regulator if the hdmi
is not used on a particular board. This is again something that feels
hackish, and I'd like to find a cleaner solution to it.

 Tomi
diff mbox

Patch

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 96a6e29..ccfc677 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -33,6 +33,7 @@ 
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
 #include <video/omapdss.h>
 
 #include "ti_hdmi.h"
@@ -62,6 +63,7 @@  static struct {
 	struct hdmi_ip_data ip_data;
 
 	struct clk *sys_clk;
+	struct regulator *vdda_hdmi_dac_reg;
 
 	int ct_cp_hpd_gpio;
 	int ls_oe_gpio;
@@ -331,6 +333,19 @@  static int __init hdmi_init_display(struct omap_dss_device *dssdev)
 
 	dss_init_hdmi_ip_ops(&hdmi.ip_data);
 
+	if (hdmi.vdda_hdmi_dac_reg == NULL) {
+		struct regulator *reg;
+
+		reg = devm_regulator_get(&hdmi.pdev->dev, "vdda_hdmi_dac");
+
+		if (IS_ERR(reg)) {
+			DSSERR("can't get VDDA_HDMI_DAC regulator\n");
+			return PTR_ERR(reg);
+		}
+
+		hdmi.vdda_hdmi_dac_reg = reg;
+	}
+
 	r = gpio_request_array(gpios, ARRAY_SIZE(gpios));
 	if (r)
 		return r;
@@ -495,6 +510,10 @@  static int hdmi_power_on(struct omap_dss_device *dssdev)
 	/* wait 300us after CT_CP_HPD for the 5V power output to reach 90% */
 	udelay(300);
 
+	r = regulator_enable(hdmi.vdda_hdmi_dac_reg);
+	if (r)
+		goto err_vdac_enable;
+
 	r = hdmi_runtime_get();
 	if (r)
 		goto err_runtime_get;
@@ -562,6 +581,8 @@  err_phy_enable:
 err_pll_enable:
 	hdmi_runtime_put();
 err_runtime_get:
+	regulator_disable(hdmi.vdda_hdmi_dac_reg);
+err_vdac_enable:
 	gpio_set_value(hdmi.ct_cp_hpd_gpio, 0);
 	gpio_set_value(hdmi.ls_oe_gpio, 0);
 	return -EIO;
@@ -576,6 +597,8 @@  static void hdmi_power_off(struct omap_dss_device *dssdev)
 	hdmi.ip_data.ops->pll_disable(&hdmi.ip_data);
 	hdmi_runtime_put();
 
+	regulator_disable(hdmi.vdda_hdmi_dac_reg);
+
 	gpio_set_value(hdmi.ct_cp_hpd_gpio, 0);
 	gpio_set_value(hdmi.ls_oe_gpio, 0);
 }