Re: [PATCH 6/6] ARM: mach-shmobile: add HDMI support to the ap4evb

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

On Mon, 5 Jul 2010, Magnus Damm wrote:

> On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Support HDMI in 720p mode.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  arch/arm/mach-shmobile/board-ap4evb.c |  128 ++++++++++++++++++++++++++++++++-
> >  1 files changed, 127 insertions(+), 1 deletions(-)
> 
> Hi Guennadi,
> 
> Thanks for your work on this!
> 
> > --- a/arch/arm/mach-shmobile/board-ap4evb.c
> > +++ b/arch/arm/mach-shmobile/board-ap4evb.c
> > @@ -645,8 +732,36 @@ static void __init gpio_no_direction(u32 addr)
> >  #define GPIO_PORT9CR   0xE6051009
> >  #define GPIO_PORT10CR  0xE605100A
> >
> > +static int __init hdmi_init_pm_clock(void)
> > +{
> > +       struct clk *hdmi_pm = clk_get(NULL, "sh-hdmi.0"),
> > +               *lcdc1_pm = clk_get(&lcdc1_device.dev, "sh_mobile_lcdc_fb.1");
> > +       int ret;
> > +
> > +       if (!IS_ERR(hdmi_pm)) {
> > +               ret = clk_enable(hdmi_pm);
> > +               if (ret < 0)
> > +                       pr_err("Cannot enable clock: %d\n", ret);
> > +       } else {
> > +               pr_err("Cannot get HDMI PM: %ld\n", PTR_ERR(hdmi_pm));
> > +       }
> > +
> > +       if (!IS_ERR(lcdc1_pm)) {
> > +               ret = clk_enable(lcdc1_pm);
> > +               if (ret < 0)
> > +                       pr_err("Cannot enable clock: %d\n", ret);
> > +       } else {
> > +               pr_err("Cannot get LCDC1 PM: %ld\n", PTR_ERR(lcdc1_pm));
> > +       }
> > +
> > +       return 0;
> > +}
> 
> This part I'm not so happy with.
> 
> Question 1:
> 
> Why do you need to statically enable the LCDC clock here? I guess it
> works around the fact that Runtime PM is missing at this point? The
> LCDC driver should manage its own clocks. With this in place the
> clocks will never be disabled and your Runtime PM changes to the LCDC
> driver will never actually stop the clocks.

Not yet on ARM, no. The change to the lcdcfb runtime-pm, that actually 
does anything at all with HDMI on ap4evb is the addition of ->display_on 
and ->display_off calls.

> Question 2:
> 
> And why do we need to enable the HDMI clock here? Same reason as
> above? I imagine that the HDMI driver already does Runtime PM?

Exactly, just like with MIPI (see ap4evb_init_display_clk()).

> Also, you should not need to pass any strings to clk_get(). clkdev
> should be able to figure out the binding from the device name.

Ok

Thanks
Guennadi
---
Guennadi Liakhovetski