Hi Pankaj, On Mon, Dec 1, 2014 at 1:36 PM, Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx> wrote: > Hi Ajay, > > On 28 November 2014 at 16:45, Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> wrote: >> This series is based on exynos-drm-next branch of Inki Dae's tree at: >> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git >> >> DECON(Display and Enhancement Controller) is the new IP >> in exynos7 SOC for generating video signals using pixel data. >> >> DECON driver can be used to drive 2 different interfaces on Exynos7: >> DECON-INT(video controller) and DECON-EXT(Mixer for HDMI) >> >> The existing FIMD driver code was used as a template to create >> DECON driver. Only DECON-INT is supported as of now, and >> DECON-EXT support will be added later. >> >> Signed-off-by: Akshu Agrawal <akshua@xxxxxxxxx> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> >> --- >> Changes since V1: >> -- Address comments from Pankaj and do few cleanups. >> > > Thanks, but still I can see this needs modification, please see my comments: > >> .../devicetree/bindings/video/exynos7-decon.txt | 67 ++ >> drivers/gpu/drm/exynos/Kconfig | 13 +- >> drivers/gpu/drm/exynos/Makefile | 1 + >> drivers/gpu/drm/exynos/exynos7_drm_decon.c | 1037 ++++++++++++++++++++ >> drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 + >> drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 + >> include/video/exynos7_decon.h | 346 +++++++ >> 7 files changed, 1466 insertions(+), 3 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/video/exynos7-decon.txt >> create mode 100644 drivers/gpu/drm/exynos/exynos7_drm_decon.c >> create mode 100644 include/video/exynos7_decon.h >> > > [snip] > >> +static int decon_mgr_initialize(struct exynos_drm_manager *mgr, >> + struct drm_device *drm_dev) >> +{ >> + struct decon_context *ctx = mgr_to_decon(mgr); >> + struct exynos_drm_private *priv = drm_dev->dev_private; >> + int ret; >> + >> + mgr->drm_dev = drm_dev; >> + mgr->pipe = ctx->pipe = priv->pipe++; >> + > > Do we really need 'pipe' in exynos_drm_manager and decon_context both? Will fix this. >> + /* attach this sub driver to iommu mapping if supported. */ >> + if (is_drm_iommu_supported(mgr->drm_dev)) { > > [snip] > >> +static void decon_win_mode_set(struct exynos_drm_manager *mgr, >> + struct exynos_drm_overlay *overlay) >> +{ >> + struct decon_context *ctx = mgr_to_decon(mgr); >> + struct decon_win_data *win_data; >> + int win, padding; >> + >> + if (!overlay) { >> + DRM_ERROR("overlay is NULL\n"); >> + return; >> + } >> + >> + win = overlay->zpos; >> + if (win == DEFAULT_ZPOS) >> + win = ctx->default_win; >> + >> + if (win < 0 || win >= WINDOWS_NR) >> + return; >> + >> + >> + win_data = &ctx->win_data[win]; >> + > > As I mentioned in V1, since these 5 lines are getting repeating better > we move it in one static function. It will help in reducing code > footprint. I tried this, the code readability is gone. I think its better to leave this as it is. > > [snip] > >> + >> +/** >> + * shadow_protect_win() - disable updating values from shadow registers at vsync >> + * >> + * @win: window to protect registers for >> + * @protect: 1 to protect (disable updates) >> + */ >> +static void decon_shadow_protect_win(struct decon_context *ctx, >> + int win, bool protect) >> +{ >> + u32 reg, bits, val; >> + >> + reg = SHADOWCON; > > How about using SHADOWCON directly instead of using local variable? Ok. >> + bits = SHADOWCON_WINx_PROTECT(win); >> + >> + val = readl(ctx->regs + reg); >> + if (protect) >> + val |= bits; >> + else >> + val &= ~bits; >> + writel(val, ctx->regs + reg); >> +} >> + >> +static void decon_win_commit(struct exynos_drm_manager *mgr, int zpos) >> +{ >> + struct decon_context *ctx = mgr_to_decon(mgr); >> + struct decon_win_data *win_data; >> + int win = zpos; >> + unsigned long val, alpha, blendeq; >> + unsigned int last_x; >> + unsigned int last_y; >> + >> + if (ctx->suspended) >> + return; >> + >> + if (win == DEFAULT_ZPOS) >> + win = ctx->default_win; >> + >> + if (win < 0 || win >= WINDOWS_NR) >> + return; >> + >> + win_data = &ctx->win_data[win]; >> + >> + /* If suspended, enable this on resume */ >> + if (ctx->suspended) { >> + win_data->resume = true; >> + return; >> + } >> + >> + /* > > [snip] > >> +static int decon_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct decon_context *ctx; >> + struct resource *res; >> + int ret = -EINVAL; >> + >> + if (!dev->of_node) >> + return -ENODEV; >> + >> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >> + if (!ctx) >> + return -ENOMEM; >> + >> + ctx->manager.type = EXYNOS_DISPLAY_TYPE_LCD; >> + ctx->manager.ops = &decon_manager_ops; >> + >> + ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CRTC, >> + ctx->manager.type); >> + if (ret) >> + return ret; >> + >> + ctx->dev = dev; >> + ctx->suspended = true; >> + >> + ctx->pclk = devm_clk_get(dev, "pclk_decon0"); >> + if (IS_ERR(ctx->pclk)) { >> + dev_err(dev, "failed to get bus clock pclk\n"); >> + ret = PTR_ERR(ctx->pclk); >> + goto err_del_component; >> + } >> + >> + ctx->aclk = devm_clk_get(dev, "aclk_decon0"); >> + if (IS_ERR(ctx->aclk)) { >> + dev_err(dev, "failed to get bus clock aclk\n"); >> + ret = PTR_ERR(ctx->aclk); >> + goto err_del_component; >> + } >> + >> + ctx->eclk = devm_clk_get(dev, "decon0_eclk"); >> + if (IS_ERR(ctx->eclk)) { >> + dev_err(dev, "failed to get eclock\n"); >> + ret = PTR_ERR(ctx->eclk); >> + goto err_del_component; >> + } >> + >> + ctx->vclk = devm_clk_get(dev, "decon0_vclk"); >> + if (IS_ERR(ctx->vclk)) { >> + dev_err(dev, "failed to get vclock\n"); >> + ret = PTR_ERR(ctx->vclk); >> + goto err_del_component; >> + } >> + >> + ctx->regs = of_iomap(dev->of_node, 0); > > This is OK, but we should handle unmapping of ctx->regs in error > conditions below. Right, I will fix this. iounmap should also happen in decon_remove( ). >> + if (IS_ERR(ctx->regs)) { >> + ret = PTR_ERR(ctx->regs); >> + goto err_del_component; >> + } >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "vsync"); >> + if (!res) { >> + dev_err(dev, "irq request failed.\n"); >> + ret = -ENXIO; >> + goto err_del_component; >> + } >> + >> + ret = devm_request_irq(dev, res->start, decon_irq_handler, >> + 0, "drm_decon", ctx); >> + if (ret) { >> + dev_err(dev, "irq request failed.\n"); >> + goto err_del_component; >> + } >> + >> + init_waitqueue_head(&ctx->wait_vsync_queue); >> + atomic_set(&ctx->wait_vsync_event, 0); >> + >> + platform_set_drvdata(pdev, ctx); >> + >> + ctx->display = exynos_dpi_probe(dev); >> + if (IS_ERR(ctx->display)) { >> + ret = PTR_ERR(ctx->display); >> + goto err_del_component; >> + } >> + >> + pm_runtime_enable(dev); >> + >> + ret = component_add(dev, &decon_component_ops); >> + if (ret) >> + goto err_disable_pm_runtime; >> + >> + return ret; >> + >> +err_disable_pm_runtime: >> + pm_runtime_disable(dev); >> + >> +err_del_component: >> + exynos_drm_component_del(dev, EXYNOS_DEVICE_TYPE_CRTC); >> + return ret; >> +} >> + >> +static int decon_remove(struct platform_device *pdev) >> +{ >> + pm_runtime_disable(&pdev->dev); >> + >> + component_del(&pdev->dev, &decon_component_ops); >> + exynos_drm_component_del(&pdev->dev, EXYNOS_DEVICE_TYPE_CRTC); >> + >> + return 0; >> +} >> + >> +struct platform_driver decon_driver = { >> + .probe = decon_probe, >> + .remove = decon_remove, >> + .driver = { >> + .name = "exynos-decon", >> + .owner = THIS_MODULE, > > This is no more required. Ok. Ajay -- 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