On Mon, 29 Jan 2018 13:56:21 +0200
Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
> > +
> > +static void cdns_dsi_init_link(struct cdns_dsi *dsi)
> > +{
> > + struct cdns_dsi_output *output = &dsi->output;
> > + unsigned long sysclk_period, ulpout;
> > + u32 val;
> > + int i;
> > +
> > + if (dsi->link_initialized)
> > + return;
> > +
> > + val = 0;
> > + for (i = 1; i < output->dev->lanes; i++)
> > + val |= DATA_LANE_EN(i);
> > +
> > + if (!(output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> > + val |= CLK_CONTINUOUS;
> > +
> > + writel(val, dsi->regs + MCTL_MAIN_PHY_CTL);
> > +
> > + /* ULPOUT should be set to 1ms and is expressed in sysclk cycles. */
> > + sysclk_period = NSEC_PER_SEC / clk_get_rate(dsi->dsi_sys_clk);
> > + ulpout = DIV_ROUND_UP(NSEC_PER_MSEC, sysclk_period);
> > + writel(CLK_LANE_ULPOUT_TIME(ulpout) | DATA_LANE_ULPOUT_TIME(ulpout),
> > + dsi->regs + MCTL_ULPOUT_TIME);
> > +
> > + writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL);
> > +
> > + val = CLK_LANE_EN | PLL_START;
> > + for (i = 0; i < output->dev->lanes; i++)
> > + val |= DATA_LANE_START(i);
> > +
> > + writel(val, dsi->regs + MCTL_MAIN_EN);
> > +
> > + ndelay(100);
>
> It's good to have a comment about each sleep/delay on what is for and
> where does the value come from.
Yep, I'll figure this out.
>
> > + dsi->link_initialized = true;
> > +}
> > +
> > +static int cdns_dsi_drm_probe(struct platform_device *pdev)
> > +{
> > + struct cdns_dsi *dsi;
> > + struct cdns_dsi_input *input;
> > + struct resource *res;
> > + int ret, irq;
> > + u32 val;
> > +
> > + dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
> > + if (!dsi)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, dsi);
> > +
> > + input = &dsi->input;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + dsi->regs = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(dsi->regs))
> > + return PTR_ERR(dsi->regs);
> > +
> > + dsi->dsi_p_clk = devm_clk_get(&pdev->dev, "dsi_p_clk");
> > + if (IS_ERR(dsi->dsi_p_clk))
> > + return PTR_ERR(dsi->dsi_p_clk);
> > +
> > + dsi->dsi_p_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
> > + "dsi_p_rst");
> > + if (IS_ERR(dsi->dsi_p_rst))
> > + return PTR_ERR(dsi->dsi_p_rst);
> > +
> > + dsi->dsi_sys_clk = devm_clk_get(&pdev->dev, "dsi_sys_clk");
> > + if (IS_ERR(dsi->dsi_sys_clk))
> > + return PTR_ERR(dsi->dsi_sys_clk);
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return irq;
> > +
> > + ret = clk_prepare_enable(dsi->dsi_p_clk);
> > + if (ret)
> > + return ret;
> > +
> > + ret = clk_prepare_enable(dsi->dsi_sys_clk);
> > + if (ret)
> > + goto err_disable_pclk;
> > +
> > + val = readl(dsi->regs + ID_REG);
> > + if (REV_VENDOR_ID(val) != 0xcad) {
> > + dev_err(&pdev->dev, "invalid vendor id\n");
> > + return -EINVAL;
> > + }
> > +
> > + val = readl(dsi->regs + IP_CONF);
> > + dsi->direct_cmd_fifo_depth = 1 << (DIRCMD_FIFO_DEPTH(val) + 2);
> > + dsi->rx_fifo_depth = RX_FIFO_DEPTH(val);
> > + init_completion(&dsi->direct_cmd_comp);
> > +
> > + writel(0, dsi->regs + MCTL_MAIN_DATA_CTL);
> > + writel(0, dsi->regs + MCTL_MAIN_EN);
> > + writel(0, dsi->regs + MCTL_MAIN_PHY_CTL);
> > +
> > + /*
> > + * We only support the DPI input, so force input->id to
> > + * CDNS_DPI_INPUT.
> > + */
> > + input->id = CDNS_DPI_INPUT;
> > + input->bridge.funcs = &cdns_dsi_bridge_funcs;
> > + input->bridge.of_node = pdev->dev.of_node;
> > +
> > + /* Mask all interrupts before registering the IRQ handler. */
> > + writel(0, dsi->regs + MCTL_MAIN_STS_CTL);
> > + writel(0, dsi->regs + MCTL_DPHY_ERR_CTL1);
> > + writel(0, dsi->regs + CMD_MODE_STS_CTL);
> > + writel(0, dsi->regs + DIRECT_CMD_STS_CTL);
> > + writel(0, dsi->regs + DIRECT_CMD_RD_STS_CTL);
> > + writel(0, dsi->regs + VID_MODE_STS_CTL);
> > + writel(0, dsi->regs + TVG_STS_CTL);
> > + writel(0, dsi->regs + DPI_IRQ_EN);
> > + ret = devm_request_irq(&pdev->dev, irq, cdns_dsi_interrupt, 0,
> > + dev_name(&pdev->dev), dsi);
> > + if (ret)
> > + goto err_disable_pclk;
> > +
> > + pm_runtime_enable(&pdev->dev);
> > + dsi->base.dev = &pdev->dev;
> > + dsi->base.ops = &cdns_dsi_ops;
> > +
> > + ret = mipi_dsi_host_register(&dsi->base);
> > + if (ret)
> > + goto err_disable_runtime_pm;
> > +
> > + clk_disable_unprepare(dsi->dsi_p_clk);
> > +
> > + return 0;
> > +
> > +err_disable_runtime_pm:
> > + pm_runtime_disable(&pdev->dev);
> > +
> > +err_disable_pclk:
> > + clk_disable_unprepare(dsi->dsi_p_clk);
> > +
> > + return ret;
> > +}
>
> You don't disable the dsi_sys_clk neither in the ok nor in the error paths.
Hm, it shouldn't be enabled in the first place: the runtime resume
hook takes care of enabling it, and we don't need this clock to access
IP registers (which is all we do in the probe).
I'll fix that.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
