On Wed, 2011-03-02 at 02:30 -0600, John S wrote:
> On Wed, Mar 2, 2011 at 1:50 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
> >
> > When using OMAP2_DSS_USE_DSI_PLL, which selects DSI PLL as source clock
> > for DISPC, the DSI needs the vdds_dsi regulator. Latest regulator
> > changes broke this, causing the the code to not acquire the regulator
> > when using OMAP2_DSS_USE_DSI_PLL.
> >
> > This patch acquires the vdds_dsi regulator in dsi_pll_init(), fixing the
> > issue. This is is just a quick hack to get the OMAP2_DSS_USE_DSI_PLL
> > option working. There shouldn't be any other downside in this solution
> > than some extra lines of code.
> >
> > OMAP2_DSS_USE_DSI_PLL is itself a big hack, and should be removed, and
> > the feature itself should be implemented in a more sane way. However,
> > the solution is not trivial, and people are using DSI PLL to get more
> > exact pixel clocks, so this hack is an acceptable temporary solution for
> > the time being.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> > ---
> > drivers/video/omap2/dss/dsi.c | 20 ++++++++++++++++++++
> > 1 files changed, 20 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> > index c3019d9..6f4f19d 100644
> > --- a/drivers/video/omap2/dss/dsi.c
> > +++ b/drivers/video/omap2/dss/dsi.c
> > @@ -1105,6 +1105,26 @@ int dsi_pll_init(struct omap_dss_device *dssdev, bool enable_hsclk,
> >
> > DSSDBG("PLL init\n");
> >
> > +#ifdef CONFIG_OMAP2_DSS_USE_DSI_PLL
> > + /*
> > + * HACK: this is just a quick hack to get the USE_DSI_PLL
> > + * option working. USE_DSI_PLL is itself a big hack, and
> > + * should be removed.
> > + */
> > + if (dsi.vdds_dsi_reg == NULL) {
> > + struct regulator *vdds_dsi;
> > +
> > + vdds_dsi = regulator_get(&dsi.pdev->dev, "vdds_dsi");
> > +
> > + if (IS_ERR(vdds_dsi)) {
> > + DSSERR("can't get VDDS_DSI regulator\n");
> > + return PTR_ERR(vdds_dsi);
> > + }
> > +
> > + dsi.vdds_dsi_reg = vdds_dsi;
> > + }
> > +#endif
> > +
> > enable_clocks(1);
> > dsi_enable_pll_clock(1);
> >
>
> The hack might be temporarily acceptable.
> But I think there should be a matching regulator_put in dsi_pll_uinit.
The regulator_put() is in dsi_exit().
The reasoning here is that we only want to get the regulator if the DSI
is actually used. Otherwise every board file should define all the
regulators, even if the board in question wouldn't even use DSI. Thus we
cannot regulator_get() in the probe of DSI driver, but only later when a
display actually uses DSI.
But when we have the regulator, we keep a reference to it until the DSI
driver is unloaded (dsi_exit). It is asymmetric, but I don't see any
reason to release the regulator when, for example, the display is
blanked.
Tomi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
