On Wed, Jun 30, 2010 at 6:55 PM, Guennadi Liakhovetski
<g.liakhovetski@xxxxxx> wrote:
> Some SH-Mobile SoCs have an HDMI controller and a PHY, attached to one of their
> LCDC interfaces. This patch adds a preliminary static support for such
> controllers, this means, that only the 720p mode is handled ATM. Support for
> more modes and a dynamic switching between them will be added by a follow up
> patch.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> ---
> arch/arm/mach-shmobile/clock-sh7372.c | 2 +-
> drivers/video/Kconfig | 8 +
> drivers/video/Makefile | 1 +
> drivers/video/sh_mobile_hdmi.c | 1070 +++++++++++++++++++++++++++++++++
> drivers/video/sh_mobile_lcdcfb.c | 190 +++++--
> include/video/sh_mobile_hdmi.h | 22 +
> include/video/sh_mobile_lcdc.h | 1 +
> 7 files changed, 1243 insertions(+), 51 deletions(-)
> create mode 100644 drivers/video/sh_mobile_hdmi.c
> create mode 100644 include/video/sh_mobile_hdmi.h
>
> diff --git a/arch/arm/mach-shmobile/clock-sh7372.c b/arch/arm/mach-shmobile/clock-sh7372.c
> index 21cb629..506a8c5 100644
> --- a/arch/arm/mach-shmobile/clock-sh7372.c
> +++ b/arch/arm/mach-shmobile/clock-sh7372.c
> @@ -484,7 +484,7 @@ static struct clk_lookup lookups[] = {
> CLKDEV_CON_ID("sub_clk", &div6_clks[DIV6_SUB]),
> CLKDEV_CON_ID("spu_clk", &div6_clks[DIV6_SPU]),
> CLKDEV_CON_ID("vou_clk", &div6_clks[DIV6_VOU]),
> - CLKDEV_CON_ID("hdmi_clk", &div6_reparent_clks[DIV6_HDMI]),
> + CLKDEV_DEV_ID("sh-mobile-hdmi", &div6_reparent_clks[DIV6_HDMI]),
> CLKDEV_CON_ID("dsit_clk", &div6_clks[DIV6_DSIT]),
> CLKDEV_CON_ID("dsi0p_clk", &div6_clks[DIV6_DSI0P]),
> CLKDEV_CON_ID("dsi1p_clk", &div6_clks[DIV6_DSI1P]),
No. =)
You're changing the name of the clock specified in the data sheet. The
clock name comes from the data sheet. It's not supposed to be tied in
the the device name on this level.
The MSTP bit should be a CLKDEV_DEV_ID().
> +static int __init sh_hdmi_probe(struct platform_device *pdev)
> +{
> + struct sh_mobile_hdmi_info *pdata = pdev->dev.platform_data;
> + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + int irq = platform_get_irq(pdev, 0), ret;
> + struct sh_hdmi *hdmi;
> + struct clk *pllc2, *dv_clki_div2;
> + long rate, prate;
> +
> + if (!res || !pdata || irq < 0)
> + return -ENODEV;
> +
> + hdmi = kzalloc(sizeof(*hdmi), GFP_KERNEL);
> + if (!hdmi) {
> + dev_err(&pdev->dev, "Cannot allocate device data\n");
> + return -ENOMEM;
> + }
> +
> + hdmi->pdata = pdata;
> +
> + hdmi->hdmi_clk = clk_get(&pdev->dev, "sh-mobile-hdmi");
> + if (IS_ERR(hdmi->hdmi_clk)) {
> + ret = PTR_ERR(hdmi->hdmi_clk);
> + dev_err(&pdev->dev, "Unable to get clock: %d\n", ret);
> + goto egetclk;
> + }
>
So clkdev already should figure out that "sh-mobile-hdmi" is needed by
the device name. The string should be used to specify interface clock
or function clock. For most drivers you do not need to pass any
string. Make sure your platform device id is setup correctly so the
platform device name matches properly with your clkdev table.
> + dv_clki_div2 = clk_get(NULL, "dv_clki_div2_clk");
> + if (IS_ERR(dv_clki_div2)) {
> + ret = PTR_ERR(dv_clki_div2);
> + dev_err(&pdev->dev, "Unable to get clock: %d\n", ret);
> + goto egetdvclki;
> + }
> +
> + pllc2 = clk_get(NULL, "pllc2_clk");
> + if (IS_ERR(pllc2)) {
> + ret = PTR_ERR(pllc2);
> + dev_err(&pdev->dev, "Unable to get clock: %d\n", ret);
> + goto egetpllc2;
> + }
> +
> + clk_disable(pllc2);
You do realize that "pllc2" is shared between a bunch of different
clocks? You most likely do not want to disable it here. Would you like
to disable all other drivers that use this clock? =)
Also, that "pllc2" may be a good fit for HDMI is really a cpu-specific
property, and this driver is for the HDMI block that will be present
on multiple processors. So we really don't want any cpu-specific bits
in here.
Remember to pass the device to clk_get(). No need to pass NULL here.
> + ret = clk_set_parent(pllc2, dv_clki_div2);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Cannot set parent: %d\n", ret);
> + goto esetdvclki;
> + }
> +
> + pr_debug("PLLC2 initial frequency %lu\n", clk_get_rate(pllc2));
> +
> + prate = clk_round_rate(pllc2, 594000000);
> + if (prate < 0) {
> + dev_err(&pdev->dev, "Cannot get suitable rate: %ld\n", prate);
> + ret = prate;
> + goto eprate;
> + }
> +
> + ret = clk_set_rate(pllc2, prate);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Cannot set rate %ld: %d\n", prate, ret);
> + goto eprate;
> + }
> +
> + pr_debug("PLLC2 set frequency %lu\n", prate);
So you're setting the pllc2 rate here...
> + ret = clk_enable(pllc2);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Cannot enable clock: %d\n", ret);
> + goto epclkenable;
> + }
> +
> + ret = clk_set_parent(hdmi->hdmi_clk, pllc2);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Cannot set parent: %d\n", ret);
> + goto esetpllc2;
> + }
> +
> + rate = PICOS2KHZ(pdata->lcd_chan->lcd_cfg.pixclock) * 1000;
> +
> + rate = clk_round_rate(hdmi->hdmi_clk, rate);
> + if (rate < 0) {
> + ret = rate;
> + dev_err(&pdev->dev, "Cannot get suitable rate: %ld\n", rate);
> + goto erate;
> + }
> +
> + clk_enable(hdmi->hdmi_clk);
> + clk_disable(hdmi->hdmi_clk);
(This is far from pretty)
> + ret = clk_set_rate(hdmi->hdmi_clk, rate);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Cannot set rate %ld: %d\n", rate, ret);
> + goto erate;
> + }
... and then you set the rate of the "hdmi_clk" here...
> + pr_debug("HDMI set frequency %lu\n", rate);
> +
> + ret = clk_enable(hdmi->hdmi_clk);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "Cannot enable clock: %d\n", ret);
> + goto eclkenable;
> + }
> +
> + dev_info(&pdev->dev, "Enabled HDMI clock at %luHz\n", rate);
Isn't "pllc2_clk" the parent of "hdmi_clk"? Most of this needs to be
dealt with from the processor or board specific code IMO.
Perhaps this is a pretty clear case when we should use function clocks
and interface clocks?
Paul [CC:ed] may have different ideas about this but below is how I
think it should be mapped out:
The function clock should be managed by Runtime PM and MSTP. After
thinking a bit about it I believe that the correct parent of the MSTP
bit should be "hp_clk" (see "Clock Assignment to Modules" table in the
data sheet).
The interface clock should be "hdmi_clk", and the parent of this clock
should be setup by the processor specific code. It should not be part
of the HDMI driver.
I think the mapping between the driver and the clock should be handled
by the clkdev tables. So this HDMI driver should not ask for any
"hdmi_clk".
We need to work a bit more on this part I think. =)
Cheers,
/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
