Re: [PATCH 4/8] OMAPDSS: HDMI: use vdda_hdmi_dac

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]