RE: [PATCH 1/3] drm: exynos: hdmi: add exynos5 support to mixer driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux