Re: [PATCH v5 1/2] drm/bridge: Add Cadence DSI driver

On Mon, 29 Jan 2018 13:56:21 +0200 Tomi Valkeinen tomi.valkeinen@ti.com 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.