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? > + /* 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. [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? > + 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. > + 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. > + .of_match_table = decon_driver_dt_match, > + }, > +}; > -- Thanks, Pankaj Dubey > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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