> -----Original Message----- > From: Joonyoung Shim [mailto:jy0922.shim@xxxxxxxxxxx] > Sent: Thursday, September 13, 2012 10:44 AM > To: Rahul Sharma > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; sw0312.kim@xxxxxxxxxxx; > inki.dae@xxxxxxxxxxx; kyungmin.park@xxxxxxxxxxx; prashanth.g@xxxxxxxxxxx; > joshi@xxxxxxxxxxx; s.shirish@xxxxxxxxxxx; fahad.k@xxxxxxxxxxx; > l.krishna@xxxxxxxxxxx; r.sh.open@xxxxxxxxx > Subject: Re: [PATCH 1/3] drm: exynos: hdmi: add exynos5 support to mixer > driver > > Hi, Rahul. > > On 09/12/2012 09:08 PM, Rahul Sharma wrote: > > Added support for exynos5 to drm mixer driver. Exynos5 works > > with dt enabled while in exynos4 mixer device information can > > be passed either way (dt or plf data). This situation is taken > > cared. > > > > Signed-off-by: Rahul Sharma <rahul.sharma@xxxxxxxxxxx> > > Signed-off-by: Shirish S <s.shirish@xxxxxxxxxxx> > > Signed-off-by: Fahad Kunnathadi <fahad.k@xxxxxxxxxxx> > > --- > > drivers/gpu/drm/exynos/exynos_mixer.c | 153 > ++++++++++++++++++++++++++++++--- > > drivers/gpu/drm/exynos/regs-mixer.h | 2 + > > 2 files changed, 142 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c > b/drivers/gpu/drm/exynos/exynos_mixer.c > > index 8a43ee1..7d04a40 100644 > > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > > @@ -71,6 +71,7 @@ struct mixer_resources { > > struct clk *sclk_mixer; > > struct clk *sclk_hdmi; > > struct clk *sclk_dac; > > + bool is_soc_exynos5; > > }; > > > > struct mixer_context { > > @@ -251,7 +252,8 @@ static void mixer_vsync_set_update(struct > mixer_context *ctx, bool enable) > > mixer_reg_writemask(res, MXR_STATUS, enable ? > > MXR_STATUS_SYNC_ENABLE : 0, MXR_STATUS_SYNC_ENABLE); > > > > - vp_reg_write(res, VP_SHADOW_UPDATE, enable ? > > + if (!res->is_soc_exynos5) > > + vp_reg_write(res, VP_SHADOW_UPDATE, enable ? > > VP_SHADOW_UPDATE_ENABLE : 0); > > } > > > > @@ -615,15 +617,21 @@ static void mixer_win_reset(struct mixer_context > *ctx) > > val = MXR_GRP_CFG_ALPHA_VAL(0); > > mixer_reg_write(res, MXR_VIDEO_CFG, val); > > > > - /* configuration of Video Processor Registers */ > > - vp_win_reset(ctx); > > - vp_default_filter(res); > > + if (!res->is_soc_exynos5) { > > + /* configuration of Video Processor Registers */ > > + vp_win_reset(ctx); > > + vp_default_filter(res); > > + } > > > > /* disable all layers */ > > mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP0_ENABLE); > > mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_GRP1_ENABLE); > > mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE); > > > > + /* enable vsync interrupt after mixer reset*/ > > + mixer_reg_writemask(res, MXR_INT_EN, MXR_INT_EN_VSYNC, > > + MXR_INT_EN_VSYNC); > > + > > mixer_vsync_set_update(ctx, true); > > spin_unlock_irqrestore(&res->reg_slock, flags); > > } > > @@ -645,7 +653,8 @@ static void mixer_poweron(struct mixer_context *ctx) > > pm_runtime_get_sync(ctx->dev); > > > > clk_enable(res->mixer); > > - clk_enable(res->vp); > > + if (!res->is_soc_exynos5) > > + clk_enable(res->vp); > > clk_enable(res->sclk_mixer); > > > > mixer_reg_write(res, MXR_INT_EN, ctx->int_en); > > @@ -666,7 +675,8 @@ static void mixer_poweroff(struct mixer_context *ctx) > > ctx->int_en = mixer_reg_read(res, MXR_INT_EN); > > > > clk_disable(res->mixer); > > - clk_disable(res->vp); > > + if (!res->is_soc_exynos5) > > + clk_disable(res->vp); > > clk_disable(res->sclk_mixer); > > > > pm_runtime_put_sync(ctx->dev); > > @@ -797,11 +807,16 @@ static void mixer_win_mode_set(void *ctx, > > static void mixer_win_commit(void *ctx, int win) > > { > > struct mixer_context *mixer_ctx = ctx; > > + struct mixer_resources *res = &mixer_ctx->mixer_res; > > > > DRM_DEBUG_KMS("[%d] %s, win: %d\n", __LINE__, __func__, win); > > > > - if (win > 1) > > - vp_video_buffer(mixer_ctx, win); > > + if (!res->is_soc_exynos5) { > > + if (win > 1) > > + vp_video_buffer(mixer_ctx, win); > > + else > > + mixer_graph_buffer(mixer_ctx, win); > > + } > > else > > mixer_graph_buffer(mixer_ctx, win); > > } > > @@ -888,6 +903,12 @@ static irqreturn_t mixer_irq_handler(int irq, void > *arg) > > > > /* handling VSYNC */ > > if (val & MXR_INT_STATUS_VSYNC) { > > + /* layer update mandatory for exynos5 soc,and not present > > + * in exynos4 */ > > + if (res->is_soc_exynos5) > > + mixer_reg_writemask(res, MXR_CFG, ~0, > > + MXR_CFG_LAYER_UPDATE); > > + > > /* interlace scan need to check shadow register */ > > if (ctx->interlace) { > > base = mixer_reg_read(res, MXR_GRAPHIC_BASE(0)); > > @@ -919,8 +940,81 @@ out: > > return IRQ_HANDLED; > > } > > > > -static int __devinit mixer_resources_init(struct > exynos_drm_hdmi_context *ctx, > > - struct platform_device *pdev) > > +static int __devinit mixer_resources_init_exynos5( > > + struct exynos_drm_hdmi_context *ctx, > > + struct platform_device *pdev) > > +{ > > + struct mixer_context *mixer_ctx = ctx->ctx; > > + struct device *dev = &pdev->dev; > > + struct mixer_resources *mixer_res = &mixer_ctx->mixer_res; > > + struct resource *res; > > + int ret; > > + > > + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__); > > + > > + mixer_res->is_soc_exynos5 = true; > > + spin_lock_init(&mixer_res->reg_slock); > > + > > + mixer_res->mixer = clk_get(dev, "mixer"); > > + if (IS_ERR_OR_NULL(mixer_res->mixer)) { > > + dev_err(dev, "failed to get clock 'mixer'\n"); > > + ret = -ENODEV; > > + goto fail; > > + } > > + > > + mixer_res->sclk_hdmi = clk_get(dev, "sclk_hdmi"); > > + if (IS_ERR_OR_NULL(mixer_res->sclk_hdmi)) { > > + dev_err(dev, "failed to get clock 'sclk_hdmi'\n"); > > + ret = -ENODEV; > > + goto fail; > > + } > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (res == NULL) { > > + dev_err(dev, "get memory resource failed.\n"); > > + ret = -ENXIO; > > + goto fail; > > + } > > + > > + mixer_res->mixer_regs = devm_ioremap(&pdev->dev, res->start, > > + resource_size(res)); > > + if (mixer_res->mixer_regs == NULL) { > > + dev_err(dev, "register mapping failed.\n"); > > + ret = -ENXIO; > > + goto fail; > > + } > > + > > + > > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > + if (res == NULL) { > > + dev_err(dev, "get interrupt resource failed.\n"); > > + ret = -ENXIO; > > + goto fail; > > + } > > + > > + ret = devm_request_irq(&pdev->dev, res->start, mixer_irq_handler, > > + 0, "drm_mixer", ctx); > > + if (ret) { > > + dev_err(dev, "request interrupt failed.\n"); > > + goto fail; > > + } > > + mixer_res->irq = res->start; > > + > > + return 0; > > + > > +fail: > > + if (!IS_ERR_OR_NULL(mixer_res->sclk_dac)) > > + clk_put(mixer_res->sclk_dac); > > + if (!IS_ERR_OR_NULL(mixer_res->sclk_hdmi)) > > + clk_put(mixer_res->sclk_hdmi); > > + if (!IS_ERR_OR_NULL(mixer_res->mixer)) > > + clk_put(mixer_res->mixer); > > + return ret; > > +} > > + > > +static int __devinit mixer_resources_init_exynos4( > > + struct exynos_drm_hdmi_context *ctx, > > + struct platform_device *pdev) > > { > > struct mixer_context *mixer_ctx = ctx->ctx; > > struct device *dev = &pdev->dev; > > @@ -928,6 +1022,10 @@ static int __devinit mixer_resources_init(struct > exynos_drm_hdmi_context *ctx, > > struct resource *res; > > int ret; > > > > + DRM_DEBUG_KMS("[%d] %s\n", __LINE__, __func__); > > + > > + mixer_res->is_soc_exynos5 = false; > > + > > spin_lock_init(&mixer_res->reg_slock); > > > > mixer_res->mixer = clk_get(dev, "mixer"); > > @@ -1028,10 +1126,17 @@ static int __devinit mixer_probe(struct > platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct exynos_drm_hdmi_context *drm_hdmi_ctx; > > struct mixer_context *ctx; > > + bool is_soc_exynos5 = false; > > int ret; > > > > dev_info(dev, "probe start\n"); > > > > + if (pdev->dev.of_node && > > + of_device_is_compatible(pdev->dev.of_node, > > + "samsung,exynos5-mixer")) { > > + is_soc_exynos5 = true; > > + } > > + > > drm_hdmi_ctx = devm_kzalloc(&pdev->dev, sizeof(*drm_hdmi_ctx), > > GFP_KERNEL); > > if (!drm_hdmi_ctx) { > > @@ -1053,7 +1158,10 @@ static int __devinit mixer_probe(struct > platform_device *pdev) > > platform_set_drvdata(pdev, drm_hdmi_ctx); > > > > /* acquire resources: regs, irqs, clocks */ > > - ret = mixer_resources_init(drm_hdmi_ctx, pdev); > > + if (is_soc_exynos5) > > + ret = mixer_resources_init_exynos5(drm_hdmi_ctx, pdev); > > + else > > + ret = mixer_resources_init_exynos4(drm_hdmi_ctx, pdev); > > I don't like this. These share many same codes. > Please, share mixer_resources_init function. I think we could reuse same codes such things related to mixer clock, sclk_hdmi, io resource and irq. > > if (ret) > > goto fail; > > > > @@ -1064,7 +1172,6 @@ static int __devinit mixer_probe(struct > platform_device *pdev) > > > > return 0; > > > > - > > fail: > > dev_info(dev, "probe failed\n"); > > return ret; > > @@ -1093,12 +1200,32 @@ static int mixer_suspend(struct device *dev) > > > > static SIMPLE_DEV_PM_OPS(mixer_pm_ops, mixer_suspend, NULL); > > > > +static struct platform_device_id mixer_driver_types[] = { > > + { > > + .name = "exynos5-mixer", > > + }, { > > + .name = "exynos4-mixer", > > + }, { > > + /* end node */ > > + } > > +}; > > + > > These names should consider devname of clock. > > > +static struct of_device_id mixer_match_types[] = { > > + { > > + .compatible = "samsung,exynos5-mixer", > > + }, { > > + /* end node */ > > + } > > +}; > > + > > struct platform_driver mixer_driver = { > > .driver = { > > - .name = "s5p-mixer", > > + .name = "exynos-mixer", > > .owner = THIS_MODULE, > > .pm = &mixer_pm_ops, > > + .of_match_table = mixer_match_types, > > }, > > .probe = mixer_probe, > > .remove = __devexit_p(mixer_remove), > > + .id_table = mixer_driver_types, > > }; > > diff --git a/drivers/gpu/drm/exynos/regs-mixer.h > b/drivers/gpu/drm/exynos/regs-mixer.h > > index fd2f4d1..0491ad8 100644 > > --- a/drivers/gpu/drm/exynos/regs-mixer.h > > +++ b/drivers/gpu/drm/exynos/regs-mixer.h > > @@ -69,6 +69,7 @@ > > (((val) << (low_bit)) & MXR_MASK(high_bit, low_bit)) > > > > /* bits for MXR_STATUS */ > > +#define MXR_STATUS_SOFT_RESET (1 << 8) > > #define MXR_STATUS_16_BURST (1 << 7) > > #define MXR_STATUS_BURST_MASK (1 << 7) > > #define MXR_STATUS_BIG_ENDIAN (1 << 3) > > @@ -77,6 +78,7 @@ > > #define MXR_STATUS_REG_RUN (1 << 0) > > > > /* bits for MXR_CFG */ > > +#define MXR_CFG_LAYER_UPDATE (1 << 31) > > #define MXR_CFG_RGB601_0_255 (0 << 9) > > #define MXR_CFG_RGB601_16_235 (1 << 9) > > #define MXR_CFG_RGB709_0_255 (2 << 9) > > Overall, i think to use is_soc_exynos5 causes too many if statements. > Let's look other good idea to solve basically. > For this, you could check mixer version reading MIXER_VER register but not check hdmi. actually hdmi has no any register we can check version. this would be the reason we used is_v13 variable to identify exynos4210 and exynos4412 SoC. I tend to agree with Rahul. I will merge it as is if there is no any better way and next we can fix it later. any opinions? > Thanks. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
