[PATCH 0/3] drm: exynos: hdmi: add exynos5 support for drm hdmi

[PATCH 0/3] drm: exynos: hdmi: add exynos5 support for drm hdmi

[Rahul Sharma]

This patch set adds the support for Samsung's Exynos5250 in DRM-HDMI. It
includes modifcations in hdmi and mixer paltform drivers. Also, removes
the dependency on plf data for hdmi, mixer context pointers needed by
exynos-drm-hdmi. 

This patchset is based on branch exynos-drm-next at 
git://git.infradead.org/users/kmpark/linux-samsung (linux v3.6-rc4)

Rahul Sharma (3):
  drm: exynos: hdmi: add exynos5 support to mixer driver
  drm: exynos: hdmi: add exynos5 support to hdmi driver
  drm: exynos: hdmi: clean dependency on plf data for mixer, hdmi
    context

 drivers/gpu/drm/exynos/exynos_drm_hdmi.c |   47 +++---
 drivers/gpu/drm/exynos/exynos_drm_hdmi.h |    2 +
 drivers/gpu/drm/exynos/exynos_hdmi.c     |  303 +++++++++++++++++++++++++++---
 drivers/gpu/drm/exynos/exynos_mixer.c    |  156 ++++++++++++++--
 drivers/gpu/drm/exynos/regs-mixer.h      |    2 +
 5 files changed, 443 insertions(+), 67 deletions(-)

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

[Rahul Sharma]

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@samsung.com>
Signed-off-by: Shirish S <s.shirish@samsung.com>
Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
---
 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);
 	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 */
+	}
+};
+
+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)
-- 
1.7.0.4

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

[Joonyoung Shim]

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@samsung.com>
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
> ---
>   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.

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

Thanks.

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

[Inki Dae]

> -----Original Message-----
> From: Joonyoung Shim [mailto:jy0922.shim@samsung.com]
> Sent: Thursday, September 13, 2012 10:44 AM
> To: Rahul Sharma
> Cc: dri-devel@lists.freedesktop.org; sw0312.kim@samsung.com;
> inki.dae@samsung.com; kyungmin.park@samsung.com; prashanth.g@samsung.com;
> joshi@samsung.com; s.shirish@samsung.com; fahad.k@samsung.com;
> l.krishna@samsung.com; r.sh.open@gmail.com
> 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@samsung.com>
> > Signed-off-by: Shirish S <s.shirish@samsung.com>
> > Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
> > ---
> >   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.

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

[Joonyoung Shim]

On 09/13/2012 11:53 AM, Inki Dae wrote:
>
>> -----Original Message-----
>> From: Joonyoung Shim [mailto:jy0922.shim@samsung.com]
>> Sent: Thursday, September 13, 2012 10:44 AM
>> To: Rahul Sharma
>> Cc: dri-devel@lists.freedesktop.org; sw0312.kim@samsung.com;
>> inki.dae@samsung.com; kyungmin.park@samsung.com; prashanth.g@samsung.com;
>> joshi@samsung.com; s.shirish@samsung.com; fahad.k@samsung.com;
>> l.krishna@samsung.com; r.sh.open@gmail.com
>> 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@samsung.com>
>>> Signed-off-by: Shirish S <s.shirish@samsung.com>
>>> Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
>>> ---
>>>    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?
>

Let's think to disassociate hdmi and mixer. I have plan to unify to one
many things of exynos hdmi. Above problem occurs because exynos5 doesn't
have video processor ip. Even if we use a field such is_soc_exynos5, the
is_soc_exynos5 is unsuitable naming if other exynos SoC also doesn't
have video processor ip.

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

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 4604 bytes --]

2012년 9월 13일 목요일에 Joonyoung Shim<jy0922.shim@samsung.com>님이 작성:
> On 09/13/2012 11:53 AM, Inki Dae wrote:
>
> -----Original Message-----
> From: Joonyoung Shim [mailto:jy0922.shim@samsung.com]
> Sent: Thursday, September 13, 2012 10:44 AM
> To: Rahul Sharma
> Cc: dri-devel@lists.freedesktop.org; sw0312.kim@samsung.com;
> inki.dae@samsung.com; kyungmin.park@samsung.com; prashanth.g@samsung.com;
> joshi@samsung.com; s.shirish@samsung.com; fahad.k@samsung.com;
> l.krishna@samsung.com; r.sh.open@gmail.com
> 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@samsung.com>
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
> ---
>    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
>
> Let's think to disassociate hdmi and mixer. I have plan to unify to one
> many things of exynos hdmi. Above problem occurs because exynos5 doesn't
> have video processor ip. Even if we use a field such is_soc_exynos5, the
> is_soc_exynos5 is unsuitable naming if other exynos SoC also doesn't
> have video processor ip.
>

one more thing, exynos5 uses GScaler instead of Video processor. the
GScaler can be also used as post processor but exynos5 spec has no any
descriptions to this. so we should check that first and next let's update
things related to hdmi.
_______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 7240 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm: exynos: hdmi: add exynos5 support to hdmi driver

[Rahul Sharma]

Added support for exynos5 to hdmi driver. Resource init
is splitted for exynos5 and exynos4. Exynos5 hdmi driver
is dt based while exynos4 hdmi driver is not.

Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
Signed-off-by: Shirish S <s.shirish@samsung.com>
Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c |  300 ++++++++++++++++++++++++++++++----
 1 files changed, 269 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index a6aea6f..5236256 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -32,6 +32,9 @@
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <linux/regulator/consumer.h>
+#include <linux/io.h>
+#include <linux/of_gpio.h>
+#include <plat/gpio-cfg.h>
 
 #include <drm/exynos_drm.h>
 
@@ -61,11 +64,13 @@ struct hdmi_context {
 	bool				powered;
 	bool				is_v13;
 	bool				dvi_mode;
+	bool				is_soc_exynos5;
 	struct mutex			hdmi_mutex;
 
 	void __iomem			*regs;
-	unsigned int			external_irq;
-	unsigned int			internal_irq;
+	int						external_irq;
+	int						internal_irq;
+	int						hpd_gpio;
 
 	struct i2c_client		*ddc_port;
 	struct i2c_client		*hdmiphy_port;
@@ -953,6 +958,23 @@ static inline void hdmi_reg_writemask(struct hdmi_context *hdata,
 	writel(value, hdata->regs + reg_id);
 }
 
+static void hdmi_cfg_hpd(struct hdmi_context *hdata, bool internal)
+{
+	if (!internal) {
+		s3c_gpio_cfgpin(hdata->hpd_gpio, S3C_GPIO_SFN(0xf));
+		s3c_gpio_setpull(hdata->hpd_gpio, S3C_GPIO_PULL_DOWN);
+	} else {
+		s3c_gpio_cfgpin(hdata->hpd_gpio, S3C_GPIO_SFN(3));
+		s3c_gpio_setpull(hdata->hpd_gpio, S3C_GPIO_PULL_NONE);
+	}
+}
+
+static int hdmi_get_hpd(struct hdmi_context *hdata)
+{
+	int gpio_stat = gpio_get_value(hdata->hpd_gpio);
+	return gpio_stat;
+}
+
 static void hdmi_v13_regs_dump(struct hdmi_context *hdata, char *prefix)
 {
 #define DUMPREG(reg_id) \
@@ -2026,6 +2048,9 @@ static void hdmi_poweron(struct hdmi_context *hdata)
 
 	if (hdata->cfg_hpd)
 		hdata->cfg_hpd(true);
+	else
+		hdmi_cfg_hpd(hdata, true);
+
 	mutex_unlock(&hdata->hdmi_mutex);
 
 	pm_runtime_get_sync(hdata->dev);
@@ -2063,6 +2088,8 @@ static void hdmi_poweroff(struct hdmi_context *hdata)
 	mutex_lock(&hdata->hdmi_mutex);
 	if (hdata->cfg_hpd)
 		hdata->cfg_hpd(false);
+	else
+		hdmi_cfg_hpd(hdata, false);
 
 	hdata->powered = false;
 
@@ -2110,17 +2137,16 @@ static irqreturn_t hdmi_external_irq_thread(int irq, void *arg)
 	struct exynos_drm_hdmi_context *ctx = arg;
 	struct hdmi_context *hdata = ctx->ctx;
 
-	if (!hdata->get_hpd)
-		goto out;
-
 	mutex_lock(&hdata->hdmi_mutex);
-	hdata->hpd = hdata->get_hpd();
+	if (hdata->get_hpd)
+		hdata->hpd = hdata->get_hpd();
+	else
+		hdata->hpd = hdmi_get_hpd(hdata);
 	mutex_unlock(&hdata->hdmi_mutex);
 
 	if (ctx->drm_dev)
 		drm_helper_hpd_irq_event(ctx->drm_dev);
 
-out:
 	return IRQ_HANDLED;
 }
 
@@ -2203,23 +2229,25 @@ static int __devinit hdmi_resources_init(struct hdmi_context *hdata)
 
 	clk_set_parent(res->sclk_hdmi, res->sclk_pixel);
 
-	res->regul_bulk = kzalloc(ARRAY_SIZE(supply) *
-		sizeof(res->regul_bulk[0]), GFP_KERNEL);
-	if (!res->regul_bulk) {
-		DRM_ERROR("failed to get memory for regulators\n");
-		goto fail;
-	}
-	for (i = 0; i < ARRAY_SIZE(supply); ++i) {
-		res->regul_bulk[i].supply = supply[i];
-		res->regul_bulk[i].consumer = NULL;
-	}
-	ret = regulator_bulk_get(dev, ARRAY_SIZE(supply), res->regul_bulk);
-	if (ret) {
-		DRM_ERROR("failed to get regulators\n");
-		goto fail;
+	if (!hdata->is_soc_exynos5) {
+		res->regul_bulk = kzalloc(ARRAY_SIZE(supply) *
+			sizeof(res->regul_bulk[0]), GFP_KERNEL);
+		if (!res->regul_bulk) {
+			DRM_ERROR("failed to get memory for regulators\n");
+			goto fail;
+		}
+		for (i = 0; i < ARRAY_SIZE(supply); ++i) {
+			res->regul_bulk[i].supply = supply[i];
+			res->regul_bulk[i].consumer = NULL;
+		}
+		ret = regulator_bulk_get(dev, ARRAY_SIZE(supply),
+			res->regul_bulk);
+		if (ret) {
+			DRM_ERROR("failed to get regulators\n");
+			goto fail;
+		}
+		res->regul_count = ARRAY_SIZE(supply);
 	}
-	res->regul_count = ARRAY_SIZE(supply);
-
 	return 0;
 fail:
 	DRM_ERROR("HDMI resource init - failed\n");
@@ -2262,7 +2290,8 @@ void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
 		hdmi_hdmiphy = hdmiphy;
 }
 
-static int __devinit hdmi_probe(struct platform_device *pdev)
+static int __devinit hdmi_resources_init_exynos4(
+	struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
@@ -2271,7 +2300,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
 	struct resource *res;
 	int ret;
 
-	DRM_DEBUG_KMS("[%d]\n", __LINE__);
+	DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
 
 	pdata = pdev->dev.platform_data;
 	if (!pdata) {
@@ -2304,14 +2333,21 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
 	hdata->cfg_hpd = pdata->cfg_hpd;
 	hdata->get_hpd = pdata->get_hpd;
 	hdata->dev = dev;
+	hdata->is_soc_exynos5 = false;
 
 	ret = hdmi_resources_init(hdata);
 	if (ret) {
 		ret = -EINVAL;
+		DRM_ERROR("hdmi_resources_init failed\n");
 		goto err_data;
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		DRM_ERROR("failed to find registers\n");
+		ret = -ENOENT;
+		goto err_resource;
+	}
 
 	hdata->regs = devm_request_and_ioremap(&pdev->dev, res);
 	if (!hdata->regs) {
@@ -2340,7 +2376,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
 
 	hdata->external_irq = platform_get_irq_byname(pdev, "external_irq");
 	if (hdata->external_irq < 0) {
-		DRM_ERROR("failed to get platform irq\n");
+		DRM_ERROR("failed to get platform external irq\n");
 		ret = hdata->external_irq;
 		goto err_hdmiphy;
 	}
@@ -2357,7 +2393,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
 			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			"hdmi_external", drm_hdmi_ctx);
 	if (ret) {
-		DRM_ERROR("failed to register hdmi internal interrupt\n");
+		DRM_ERROR("failed to register hdmi external interrupt\n");
 		goto err_hdmiphy;
 	}
 
@@ -2372,10 +2408,157 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
 		goto err_free_irq;
 	}
 
-	/* register specific callbacks to common hdmi. */
-	exynos_hdmi_ops_register(&hdmi_ops);
+	return 0;
 
-	pm_runtime_enable(dev);
+err_free_irq:
+	free_irq(hdata->external_irq, drm_hdmi_ctx);
+err_hdmiphy:
+	i2c_del_driver(&hdmiphy_driver);
+err_ddc:
+	i2c_del_driver(&ddc_driver);
+err_resource:
+	hdmi_resources_cleanup(hdata);
+err_data:
+	return ret;
+}
+
+static int __devinit hdmi_resources_init_exynos5(
+			struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
+	struct hdmi_context *hdata;
+	struct resource *res;
+	unsigned int value;
+	int ret;
+	enum of_gpio_flags flags;
+
+	DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
+
+	drm_hdmi_ctx = devm_kzalloc(&pdev->dev, sizeof(*drm_hdmi_ctx),
+								GFP_KERNEL);
+	if (!drm_hdmi_ctx) {
+		DRM_ERROR("failed to allocate common hdmi context.\n");
+		return -ENOMEM;
+	}
+
+	hdata = devm_kzalloc(&pdev->dev, sizeof(struct hdmi_context),
+								GFP_KERNEL);
+	if (!hdata) {
+		DRM_ERROR("out of memory\n");
+		return -ENOMEM;
+	}
+
+	mutex_init(&hdata->hdmi_mutex);
+
+	drm_hdmi_ctx->ctx = (void *)hdata;
+	hdata->parent_ctx = (void *)drm_hdmi_ctx;
+
+	platform_set_drvdata(pdev, drm_hdmi_ctx);
+
+	if (!of_property_read_u32(pdev->dev.of_node,
+				"v13_support", &value)) {
+		hdata->is_v13 = (value == 0) ? false : true;
+	} else {
+		DRM_ERROR("no hdmi version property found\n");
+		ret = -EINVAL;
+		goto err_data;
+	}
+
+	if (!of_find_property(pdev->dev.of_node,
+				"hpd-gpio", &value)){
+		DRM_ERROR("no hpd gpio property found\n");
+		ret = -EINVAL;
+		goto err_data;
+	}
+
+	hdata->hpd_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
+				"hpd-gpio", 0, &flags);
+
+	if (!gpio_is_valid(hdata->hpd_gpio)) {
+		DRM_ERROR("failed to get hpd gpio.");
+		ret = -EINVAL;
+		goto err_data;
+	}
+
+	hdata->cfg_hpd = NULL;
+	hdata->get_hpd = NULL;
+	hdata->dev = dev;
+	hdata->is_soc_exynos5 = true;
+
+	ret = hdmi_resources_init(hdata);
+	if (ret) {
+		ret = -EINVAL;
+		DRM_ERROR("failed hdmi_resources_init.");
+		goto err_data;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		DRM_ERROR("failed to find registers\n");
+		ret = -ENOENT;
+		goto err_resource;
+	}
+
+	hdata->regs = devm_request_and_ioremap(&pdev->dev, res);
+	if (!hdata->regs) {
+		DRM_ERROR("failed to map registers\n");
+		ret = -ENXIO;
+		goto err_resource;
+	}
+
+	/* DDC i2c driver */
+	if (i2c_add_driver(&ddc_driver)) {
+		DRM_ERROR("failed to register ddc i2c driver\n");
+		ret = -ENOENT;
+		goto err_resource;
+	}
+
+	hdata->ddc_port = hdmi_ddc;
+
+	/* hdmiphy i2c driver */
+	if (i2c_add_driver(&hdmiphy_driver)) {
+		DRM_ERROR("failed to register hdmiphy i2c driver\n");
+		ret = -ENOENT;
+		goto err_ddc;
+	}
+
+	hdata->hdmiphy_port = hdmi_hdmiphy;
+
+	hdata->external_irq = gpio_to_irq(hdata->hpd_gpio);
+
+	if (hdata->external_irq < 0) {
+		DRM_ERROR("failed to get platform external irq\n");
+		ret = hdata->external_irq;
+		goto err_hdmiphy;
+	}
+
+	hdata->internal_irq = platform_get_irq(pdev, 0);
+
+	if (hdata->internal_irq < 0) {
+		DRM_ERROR("failed to get platform internal irq\n");
+		ret = hdata->internal_irq;
+		goto err_hdmiphy;
+	}
+
+	ret = request_threaded_irq(hdata->external_irq, NULL,
+			hdmi_external_irq_thread, IRQF_TRIGGER_RISING |
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			"hdmi_external", drm_hdmi_ctx);
+	if (ret) {
+		DRM_ERROR("failed to register hdmi external interrupt\n");
+		goto err_hdmiphy;
+	}
+
+	hdmi_cfg_hpd(hdata, false);
+
+	ret = request_threaded_irq(hdata->internal_irq, NULL,
+			hdmi_internal_irq_thread, IRQF_ONESHOT,
+			"hdmi_internal", drm_hdmi_ctx);
+	if (ret) {
+		DRM_ERROR("failed to register hdmi internal interrupt\n");
+		goto err_free_irq;
+	}
 
 	return 0;
 
@@ -2391,6 +2574,41 @@ err_data:
 	return ret;
 }
 
+static int __devinit hdmi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
+	bool is_soc_exynos5 = false;
+	int ret;
+
+	DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
+
+	if (pdev->dev.of_node &&
+			of_device_is_compatible(pdev->dev.of_node,
+			"samsung,exynos5-hdmi")) {
+			is_soc_exynos5 = true;
+	}
+
+	/* acquire resources: regs, irqs, clocks */
+	if (is_soc_exynos5)
+		ret = hdmi_resources_init_exynos5(pdev);
+	else
+		ret = hdmi_resources_init_exynos4(pdev);
+	if (ret)
+		goto err_data;
+
+	drm_hdmi_ctx = platform_get_drvdata(pdev);
+
+	/* register specific callbacks to common hdmi. */
+	exynos_hdmi_ops_register(&hdmi_ops);
+
+	pm_runtime_enable(dev);
+
+	return 0;
+
+err_data:	return ret;
+}
+
 static int __devexit hdmi_remove(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -2444,12 +2662,32 @@ static int hdmi_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(hdmi_pm_ops, hdmi_suspend, hdmi_resume);
 
+static struct platform_device_id hdmi_driver_types[] = {
+	{
+		.name		= "exynos5-hdmi",
+	}, {
+		.name		= "exynos4-hdmi",
+	}, {
+		/* end node */
+	}
+};
+
+static struct of_device_id hdmi_match_types[] = {
+	{
+		.compatible = "samsung,exynos5-hdmi",
+	}, {
+		/* end node */
+	}
+};
+
 struct platform_driver hdmi_driver = {
 	.probe		= hdmi_probe,
 	.remove		= __devexit_p(hdmi_remove),
+	.id_table	= hdmi_driver_types,
 	.driver		= {
-		.name	= "exynos4-hdmi",
+		.name	= "exynos-hdmi",
 		.owner	= THIS_MODULE,
 		.pm	= &hdmi_pm_ops,
+		.of_match_table = hdmi_match_types,
 	},
 };
-- 
1.7.0.4

Re: [PATCH 2/3] drm: exynos: hdmi: add exynos5 support to hdmi driver

[Joonyoung Shim]

Hi, Rahul.

Overall, i think this patch causes messy codes.

On 09/12/2012 09:08 PM, Rahul Sharma wrote:
> Added support for exynos5 to hdmi driver. Resource init
> is splitted for exynos5 and exynos4. Exynos5 hdmi driver
> is dt based while exynos4 hdmi driver is not.
>
> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
> ---
>   drivers/gpu/drm/exynos/exynos_hdmi.c |  300 ++++++++++++++++++++++++++++++----
>   1 files changed, 269 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index a6aea6f..5236256 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -32,6 +32,9 @@
>   #include <linux/pm_runtime.h>
>   #include <linux/clk.h>
>   #include <linux/regulator/consumer.h>
> +#include <linux/io.h>
> +#include <linux/of_gpio.h>
> +#include <plat/gpio-cfg.h>
>   
>   #include <drm/exynos_drm.h>
>   
> @@ -61,11 +64,13 @@ struct hdmi_context {
>   	bool				powered;
>   	bool				is_v13;
>   	bool				dvi_mode;
> +	bool				is_soc_exynos5;
>   	struct mutex			hdmi_mutex;
>   
>   	void __iomem			*regs;
> -	unsigned int			external_irq;
> -	unsigned int			internal_irq;
> +	int						external_irq;
> +	int						internal_irq;
> +	int						hpd_gpio;
>   
>   	struct i2c_client		*ddc_port;
>   	struct i2c_client		*hdmiphy_port;
> @@ -953,6 +958,23 @@ static inline void hdmi_reg_writemask(struct hdmi_context *hdata,
>   	writel(value, hdata->regs + reg_id);
>   }
>   
> +static void hdmi_cfg_hpd(struct hdmi_context *hdata, bool internal)
> +{
> +	if (!internal) {
> +		s3c_gpio_cfgpin(hdata->hpd_gpio, S3C_GPIO_SFN(0xf));
> +		s3c_gpio_setpull(hdata->hpd_gpio, S3C_GPIO_PULL_DOWN);
> +	} else {
> +		s3c_gpio_cfgpin(hdata->hpd_gpio, S3C_GPIO_SFN(3));
> +		s3c_gpio_setpull(hdata->hpd_gpio, S3C_GPIO_PULL_NONE);
> +	}
> +}
> +

Don't use SoC specific functions in the driver.

> +static int hdmi_get_hpd(struct hdmi_context *hdata)
> +{
> +	int gpio_stat = gpio_get_value(hdata->hpd_gpio);
> +	return gpio_stat;
> +}
> +

Actually, above two functions should come from platform data, but these
will remove soon.

>   static void hdmi_v13_regs_dump(struct hdmi_context *hdata, char *prefix)
>   {
>   #define DUMPREG(reg_id) \
> @@ -2026,6 +2048,9 @@ static void hdmi_poweron(struct hdmi_context *hdata)
>   
>   	if (hdata->cfg_hpd)
>   		hdata->cfg_hpd(true);
> +	else
> +		hdmi_cfg_hpd(hdata, true);
> +
>   	mutex_unlock(&hdata->hdmi_mutex);
>   
>   	pm_runtime_get_sync(hdata->dev);
> @@ -2063,6 +2088,8 @@ static void hdmi_poweroff(struct hdmi_context *hdata)
>   	mutex_lock(&hdata->hdmi_mutex);
>   	if (hdata->cfg_hpd)
>   		hdata->cfg_hpd(false);
> +	else
> +		hdmi_cfg_hpd(hdata, false);
>   
>   	hdata->powered = false;
>   
> @@ -2110,17 +2137,16 @@ static irqreturn_t hdmi_external_irq_thread(int irq, void *arg)
>   	struct exynos_drm_hdmi_context *ctx = arg;
>   	struct hdmi_context *hdata = ctx->ctx;
>   
> -	if (!hdata->get_hpd)
> -		goto out;
> -
>   	mutex_lock(&hdata->hdmi_mutex);
> -	hdata->hpd = hdata->get_hpd();
> +	if (hdata->get_hpd)
> +		hdata->hpd = hdata->get_hpd();
> +	else
> +		hdata->hpd = hdmi_get_hpd(hdata);
>   	mutex_unlock(&hdata->hdmi_mutex);
>   
>   	if (ctx->drm_dev)
>   		drm_helper_hpd_irq_event(ctx->drm_dev);
>   
> -out:
>   	return IRQ_HANDLED;
>   }
>   
> @@ -2203,23 +2229,25 @@ static int __devinit hdmi_resources_init(struct hdmi_context *hdata)
>   
>   	clk_set_parent(res->sclk_hdmi, res->sclk_pixel);
>   
> -	res->regul_bulk = kzalloc(ARRAY_SIZE(supply) *
> -		sizeof(res->regul_bulk[0]), GFP_KERNEL);
> -	if (!res->regul_bulk) {
> -		DRM_ERROR("failed to get memory for regulators\n");
> -		goto fail;
> -	}
> -	for (i = 0; i < ARRAY_SIZE(supply); ++i) {
> -		res->regul_bulk[i].supply = supply[i];
> -		res->regul_bulk[i].consumer = NULL;
> -	}
> -	ret = regulator_bulk_get(dev, ARRAY_SIZE(supply), res->regul_bulk);
> -	if (ret) {
> -		DRM_ERROR("failed to get regulators\n");
> -		goto fail;
> +	if (!hdata->is_soc_exynos5) {
> +		res->regul_bulk = kzalloc(ARRAY_SIZE(supply) *
> +			sizeof(res->regul_bulk[0]), GFP_KERNEL);
> +		if (!res->regul_bulk) {
> +			DRM_ERROR("failed to get memory for regulators\n");
> +			goto fail;
> +		}
> +		for (i = 0; i < ARRAY_SIZE(supply); ++i) {
> +			res->regul_bulk[i].supply = supply[i];
> +			res->regul_bulk[i].consumer = NULL;
> +		}
> +		ret = regulator_bulk_get(dev, ARRAY_SIZE(supply),
> +			res->regul_bulk);
> +		if (ret) {
> +			DRM_ERROR("failed to get regulators\n");
> +			goto fail;
> +		}
> +		res->regul_count = ARRAY_SIZE(supply);
>   	}
> -	res->regul_count = ARRAY_SIZE(supply);
> -
>   	return 0;
>   fail:
>   	DRM_ERROR("HDMI resource init - failed\n");
> @@ -2262,7 +2290,8 @@ void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
>   		hdmi_hdmiphy = hdmiphy;
>   }
>   
> -static int __devinit hdmi_probe(struct platform_device *pdev)
> +static int __devinit hdmi_resources_init_exynos4(
> +	struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
> @@ -2271,7 +2300,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
>   	struct resource *res;
>   	int ret;
>   
> -	DRM_DEBUG_KMS("[%d]\n", __LINE__);
> +	DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
>   
>   	pdata = pdev->dev.platform_data;
>   	if (!pdata) {
> @@ -2304,14 +2333,21 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
>   	hdata->cfg_hpd = pdata->cfg_hpd;
>   	hdata->get_hpd = pdata->get_hpd;
>   	hdata->dev = dev;
> +	hdata->is_soc_exynos5 = false;
>   
>   	ret = hdmi_resources_init(hdata);
>   	if (ret) {
>   		ret = -EINVAL;
> +		DRM_ERROR("hdmi_resources_init failed\n");
>   		goto err_data;
>   	}
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		DRM_ERROR("failed to find registers\n");
> +		ret = -ENOENT;
> +		goto err_resource;
> +	}
>   
>   	hdata->regs = devm_request_and_ioremap(&pdev->dev, res);
>   	if (!hdata->regs) {
> @@ -2340,7 +2376,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
>   
>   	hdata->external_irq = platform_get_irq_byname(pdev, "external_irq");
>   	if (hdata->external_irq < 0) {
> -		DRM_ERROR("failed to get platform irq\n");
> +		DRM_ERROR("failed to get platform external irq\n");
>   		ret = hdata->external_irq;
>   		goto err_hdmiphy;
>   	}
> @@ -2357,7 +2393,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
>   			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>   			"hdmi_external", drm_hdmi_ctx);
>   	if (ret) {
> -		DRM_ERROR("failed to register hdmi internal interrupt\n");
> +		DRM_ERROR("failed to register hdmi external interrupt\n");
>   		goto err_hdmiphy;
>   	}
>   
> @@ -2372,10 +2408,157 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
>   		goto err_free_irq;
>   	}
>   
> -	/* register specific callbacks to common hdmi. */
> -	exynos_hdmi_ops_register(&hdmi_ops);
> +	return 0;
>   
> -	pm_runtime_enable(dev);
> +err_free_irq:
> +	free_irq(hdata->external_irq, drm_hdmi_ctx);
> +err_hdmiphy:
> +	i2c_del_driver(&hdmiphy_driver);
> +err_ddc:
> +	i2c_del_driver(&ddc_driver);
> +err_resource:
> +	hdmi_resources_cleanup(hdata);
> +err_data:
> +	return ret;
> +}
> +
> +static int __devinit hdmi_resources_init_exynos5(
> +			struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
> +	struct hdmi_context *hdata;
> +	struct resource *res;
> +	unsigned int value;
> +	int ret;
> +	enum of_gpio_flags flags;
> +
> +	DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
> +
> +	drm_hdmi_ctx = devm_kzalloc(&pdev->dev, sizeof(*drm_hdmi_ctx),
> +								GFP_KERNEL);
> +	if (!drm_hdmi_ctx) {
> +		DRM_ERROR("failed to allocate common hdmi context.\n");
> +		return -ENOMEM;
> +	}
> +
> +	hdata = devm_kzalloc(&pdev->dev, sizeof(struct hdmi_context),
> +								GFP_KERNEL);
> +	if (!hdata) {
> +		DRM_ERROR("out of memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	mutex_init(&hdata->hdmi_mutex);
> +
> +	drm_hdmi_ctx->ctx = (void *)hdata;
> +	hdata->parent_ctx = (void *)drm_hdmi_ctx;
> +
> +	platform_set_drvdata(pdev, drm_hdmi_ctx);
> +
> +	if (!of_property_read_u32(pdev->dev.of_node,
> +				"v13_support", &value)) {
> +		hdata->is_v13 = (value == 0) ? false : true;
> +	} else {
> +		DRM_ERROR("no hdmi version property found\n");
> +		ret = -EINVAL;
> +		goto err_data;
> +	}
> +
> +	if (!of_find_property(pdev->dev.of_node,
> +				"hpd-gpio", &value)){
> +		DRM_ERROR("no hpd gpio property found\n");
> +		ret = -EINVAL;
> +		goto err_data;
> +	}
> +
> +	hdata->hpd_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
> +				"hpd-gpio", 0, &flags);
> +
> +	if (!gpio_is_valid(hdata->hpd_gpio)) {
> +		DRM_ERROR("failed to get hpd gpio.");
> +		ret = -EINVAL;
> +		goto err_data;
> +	}
> +
> +	hdata->cfg_hpd = NULL;
> +	hdata->get_hpd = NULL;
> +	hdata->dev = dev;
> +	hdata->is_soc_exynos5 = true;
> +
> +	ret = hdmi_resources_init(hdata);
> +	if (ret) {
> +		ret = -EINVAL;
> +		DRM_ERROR("failed hdmi_resources_init.");
> +		goto err_data;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		DRM_ERROR("failed to find registers\n");
> +		ret = -ENOENT;
> +		goto err_resource;
> +	}
> +
> +	hdata->regs = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!hdata->regs) {
> +		DRM_ERROR("failed to map registers\n");
> +		ret = -ENXIO;
> +		goto err_resource;
> +	}
> +
> +	/* DDC i2c driver */
> +	if (i2c_add_driver(&ddc_driver)) {
> +		DRM_ERROR("failed to register ddc i2c driver\n");
> +		ret = -ENOENT;
> +		goto err_resource;
> +	}
> +
> +	hdata->ddc_port = hdmi_ddc;
> +
> +	/* hdmiphy i2c driver */
> +	if (i2c_add_driver(&hdmiphy_driver)) {
> +		DRM_ERROR("failed to register hdmiphy i2c driver\n");
> +		ret = -ENOENT;
> +		goto err_ddc;
> +	}
> +
> +	hdata->hdmiphy_port = hdmi_hdmiphy;
> +
> +	hdata->external_irq = gpio_to_irq(hdata->hpd_gpio);
> +
> +	if (hdata->external_irq < 0) {
> +		DRM_ERROR("failed to get platform external irq\n");
> +		ret = hdata->external_irq;
> +		goto err_hdmiphy;
> +	}
> +
> +	hdata->internal_irq = platform_get_irq(pdev, 0);
> +
> +	if (hdata->internal_irq < 0) {
> +		DRM_ERROR("failed to get platform internal irq\n");
> +		ret = hdata->internal_irq;
> +		goto err_hdmiphy;
> +	}
> +
> +	ret = request_threaded_irq(hdata->external_irq, NULL,
> +			hdmi_external_irq_thread, IRQF_TRIGGER_RISING |
> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			"hdmi_external", drm_hdmi_ctx);
> +	if (ret) {
> +		DRM_ERROR("failed to register hdmi external interrupt\n");
> +		goto err_hdmiphy;
> +	}
> +
> +	hdmi_cfg_hpd(hdata, false);
> +
> +	ret = request_threaded_irq(hdata->internal_irq, NULL,
> +			hdmi_internal_irq_thread, IRQF_ONESHOT,
> +			"hdmi_internal", drm_hdmi_ctx);
> +	if (ret) {
> +		DRM_ERROR("failed to register hdmi internal interrupt\n");
> +		goto err_free_irq;
> +	}
>   
>   	return 0;
>   
> @@ -2391,6 +2574,41 @@ err_data:
>   	return ret;
>   }
>   
> +static int __devinit hdmi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
> +	bool is_soc_exynos5 = false;
> +	int ret;
> +
> +	DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
> +
> +	if (pdev->dev.of_node &&
> +			of_device_is_compatible(pdev->dev.of_node,
> +			"samsung,exynos5-hdmi")) {
> +			is_soc_exynos5 = true;
> +	}
> +
> +	/* acquire resources: regs, irqs, clocks */
> +	if (is_soc_exynos5)
> +		ret = hdmi_resources_init_exynos5(pdev);
> +	else
> +		ret = hdmi_resources_init_exynos4(pdev);

I think it isn't good to split using is_soc_exynos5 because the exynos5
hdmi is almost same that of exynos4x12.

> +	if (ret)
> +		goto err_data;
> +
> +	drm_hdmi_ctx = platform_get_drvdata(pdev);
> +
> +	/* register specific callbacks to common hdmi. */
> +	exynos_hdmi_ops_register(&hdmi_ops);
> +
> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +
> +err_data:	return ret;
> +}
> +
>   static int __devexit hdmi_remove(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> @@ -2444,12 +2662,32 @@ static int hdmi_resume(struct device *dev)
>   
>   static SIMPLE_DEV_PM_OPS(hdmi_pm_ops, hdmi_suspend, hdmi_resume);
>   
> +static struct platform_device_id hdmi_driver_types[] = {
> +	{
> +		.name		= "exynos5-hdmi",
> +	}, {
> +		.name		= "exynos4-hdmi",
> +	}, {
> +		/* end node */
> +	}
> +};
> +
> +static struct of_device_id hdmi_match_types[] = {
> +	{
> +		.compatible = "samsung,exynos5-hdmi",
> +	}, {
> +		/* end node */
> +	}
> +};
> +
>   struct platform_driver hdmi_driver = {
>   	.probe		= hdmi_probe,
>   	.remove		= __devexit_p(hdmi_remove),
> +	.id_table	= hdmi_driver_types,
>   	.driver		= {
> -		.name	= "exynos4-hdmi",
> +		.name	= "exynos-hdmi",
>   		.owner	= THIS_MODULE,
>   		.pm	= &hdmi_pm_ops,
> +		.of_match_table = hdmi_match_types,
>   	},
>   };

Thanks.

[PATCH 3/3] drm: exynos: hdmi: clean dependency on plf data for mixer, hdmi context

[Rahul Sharma]

exynos-drm-hdmi need context pointers from hdmi and mixer. These
pointers were expected from the plf data. Cleaned this dependency
by exporting i/f which are called by hdmi, mixer driver probes
for setting their context.

Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_hdmi.c |   47 +++++++++++++++--------------
 drivers/gpu/drm/exynos/exynos_drm_hdmi.h |    2 +
 drivers/gpu/drm/exynos/exynos_hdmi.c     |    3 ++
 drivers/gpu/drm/exynos/exynos_mixer.c    |    3 ++
 4 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
index 0584132..4c8d933 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
@@ -29,6 +29,11 @@
 #define get_ctx_from_subdrv(subdrv)	container_of(subdrv,\
 					struct drm_hdmi_context, subdrv);
 
+/* Common hdmi subdrv needs to access the hdmi and mixer though context.
+* These should be initialied by the repective drivers */
+static struct exynos_drm_hdmi_context *hdmi_ctx;
+static struct exynos_drm_hdmi_context *mixer_ctx;
+
 /* these callback points shoud be set by specific drivers. */
 static struct exynos_hdmi_ops *hdmi_ops;
 static struct exynos_mixer_ops *mixer_ops;
@@ -41,6 +46,18 @@ struct drm_hdmi_context {
 	bool	enabled[MIXER_WIN_NR];
 };
 
+void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx)
+{
+	if (ctx)
+		hdmi_ctx = ctx;
+}
+
+void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx)
+{
+	if (ctx)
+		mixer_ctx = ctx;
+}
+
 void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops)
 {
 	DRM_DEBUG_KMS("%s\n", __FILE__);
@@ -303,46 +320,30 @@ static int hdmi_subdrv_probe(struct drm_device *drm_dev,
 {
 	struct exynos_drm_subdrv *subdrv = to_subdrv(dev);
 	struct drm_hdmi_context *ctx;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct exynos_drm_common_hdmi_pd *pd;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
-	pd = pdev->dev.platform_data;
-
-	if (!pd) {
-		DRM_DEBUG_KMS("platform data is null.\n");
-		return -EFAULT;
-	}
-
-	if (!pd->hdmi_dev) {
+	if (!hdmi_ctx) {
 		DRM_DEBUG_KMS("hdmi device is null.\n");
 		return -EFAULT;
 	}
 
-	if (!pd->mixer_dev) {
+	if (!mixer_ctx) {
 		DRM_DEBUG_KMS("mixer device is null.\n");
 		return -EFAULT;
 	}
 
 	ctx = get_ctx_from_subdrv(subdrv);
 
-	ctx->hdmi_ctx = (struct exynos_drm_hdmi_context *)
-				to_context(pd->hdmi_dev);
-	if (!ctx->hdmi_ctx) {
-		DRM_DEBUG_KMS("hdmi context is null.\n");
+	if (!ctx) {
+		DRM_DEBUG_KMS("context is null.\n");
 		return -EFAULT;
 	}
 
-	ctx->hdmi_ctx->drm_dev = drm_dev;
-
-	ctx->mixer_ctx = (struct exynos_drm_hdmi_context *)
-				to_context(pd->mixer_dev);
-	if (!ctx->mixer_ctx) {
-		DRM_DEBUG_KMS("mixer context is null.\n");
-		return -EFAULT;
-	}
+	ctx->hdmi_ctx = hdmi_ctx;
+	ctx->mixer_ctx = mixer_ctx;
 
+	ctx->hdmi_ctx->drm_dev = drm_dev;
 	ctx->mixer_ctx->drm_dev = drm_dev;
 
 	return 0;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
index d9f9e9f..2da5ffd 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
@@ -73,6 +73,8 @@ struct exynos_mixer_ops {
 	void (*win_disable)(void *ctx, int zpos);
 };
 
+void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx);
+void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx);
 void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops);
 void exynos_mixer_ops_register(struct exynos_mixer_ops *ops);
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 5236256..82ee810 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -2599,6 +2599,9 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
 
 	drm_hdmi_ctx = platform_get_drvdata(pdev);
 
+	/* Attach HDMI Driver to common hdmi. */
+	exynos_hdmi_drv_attach(drm_hdmi_ctx);
+
 	/* register specific callbacks to common hdmi. */
 	exynos_hdmi_ops_register(&hdmi_ops);
 
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 7d04a40..f9e26f2 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -1165,6 +1165,9 @@ static int __devinit mixer_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail;
 
+	/* attach mixer driver to common hdmi. */
+	exynos_mixer_drv_attach(drm_hdmi_ctx);
+
 	/* register specific callback point to common hdmi. */
 	exynos_mixer_ops_register(&mixer_ops);
 
-- 
1.7.0.4

Re: [PATCH 3/3] drm: exynos: hdmi: clean dependency on plf data for mixer, hdmi context

[Joonyoung Shim]

Hi, Rahul.

On 09/12/2012 09:08 PM, Rahul Sharma wrote:
> exynos-drm-hdmi need context pointers from hdmi and mixer. These
> pointers were expected from the plf data. Cleaned this dependency

What does plf data mean?

> by exporting i/f which are called by hdmi, mixer driver probes
> for setting their context.

It is reasonable to me. This can remove struct exynos_drm_common_hdmi_pd.

Thanks.

> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> ---
>   drivers/gpu/drm/exynos/exynos_drm_hdmi.c |   47 +++++++++++++++--------------
>   drivers/gpu/drm/exynos/exynos_drm_hdmi.h |    2 +
>   drivers/gpu/drm/exynos/exynos_hdmi.c     |    3 ++
>   drivers/gpu/drm/exynos/exynos_mixer.c    |    3 ++
>   4 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> index 0584132..4c8d933 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> @@ -29,6 +29,11 @@
>   #define get_ctx_from_subdrv(subdrv)	container_of(subdrv,\
>   					struct drm_hdmi_context, subdrv);
>   
> +/* Common hdmi subdrv needs to access the hdmi and mixer though context.
> +* These should be initialied by the repective drivers */
> +static struct exynos_drm_hdmi_context *hdmi_ctx;
> +static struct exynos_drm_hdmi_context *mixer_ctx;
> +
>   /* these callback points shoud be set by specific drivers. */
>   static struct exynos_hdmi_ops *hdmi_ops;
>   static struct exynos_mixer_ops *mixer_ops;
> @@ -41,6 +46,18 @@ struct drm_hdmi_context {
>   	bool	enabled[MIXER_WIN_NR];
>   };
>   
> +void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx)
> +{
> +	if (ctx)
> +		hdmi_ctx = ctx;
> +}
> +
> +void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx)
> +{
> +	if (ctx)
> +		mixer_ctx = ctx;
> +}
> +
>   void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops)
>   {
>   	DRM_DEBUG_KMS("%s\n", __FILE__);
> @@ -303,46 +320,30 @@ static int hdmi_subdrv_probe(struct drm_device *drm_dev,
>   {
>   	struct exynos_drm_subdrv *subdrv = to_subdrv(dev);
>   	struct drm_hdmi_context *ctx;
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct exynos_drm_common_hdmi_pd *pd;
>   
>   	DRM_DEBUG_KMS("%s\n", __FILE__);
>   
> -	pd = pdev->dev.platform_data;
> -
> -	if (!pd) {
> -		DRM_DEBUG_KMS("platform data is null.\n");
> -		return -EFAULT;
> -	}
> -
> -	if (!pd->hdmi_dev) {
> +	if (!hdmi_ctx) {
>   		DRM_DEBUG_KMS("hdmi device is null.\n");
>   		return -EFAULT;
>   	}
>   
> -	if (!pd->mixer_dev) {
> +	if (!mixer_ctx) {
>   		DRM_DEBUG_KMS("mixer device is null.\n");
>   		return -EFAULT;
>   	}
>   
>   	ctx = get_ctx_from_subdrv(subdrv);
>   
> -	ctx->hdmi_ctx = (struct exynos_drm_hdmi_context *)
> -				to_context(pd->hdmi_dev);
> -	if (!ctx->hdmi_ctx) {
> -		DRM_DEBUG_KMS("hdmi context is null.\n");
> +	if (!ctx) {
> +		DRM_DEBUG_KMS("context is null.\n");
>   		return -EFAULT;
>   	}
>   
> -	ctx->hdmi_ctx->drm_dev = drm_dev;
> -
> -	ctx->mixer_ctx = (struct exynos_drm_hdmi_context *)
> -				to_context(pd->mixer_dev);
> -	if (!ctx->mixer_ctx) {
> -		DRM_DEBUG_KMS("mixer context is null.\n");
> -		return -EFAULT;
> -	}
> +	ctx->hdmi_ctx = hdmi_ctx;
> +	ctx->mixer_ctx = mixer_ctx;
>   
> +	ctx->hdmi_ctx->drm_dev = drm_dev;
>   	ctx->mixer_ctx->drm_dev = drm_dev;
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> index d9f9e9f..2da5ffd 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> @@ -73,6 +73,8 @@ struct exynos_mixer_ops {
>   	void (*win_disable)(void *ctx, int zpos);
>   };
>   
> +void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx);
> +void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx);
>   void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops);
>   void exynos_mixer_ops_register(struct exynos_mixer_ops *ops);
>   #endif
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 5236256..82ee810 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -2599,6 +2599,9 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
>   
>   	drm_hdmi_ctx = platform_get_drvdata(pdev);
>   
> +	/* Attach HDMI Driver to common hdmi. */
> +	exynos_hdmi_drv_attach(drm_hdmi_ctx);
> +
>   	/* register specific callbacks to common hdmi. */
>   	exynos_hdmi_ops_register(&hdmi_ops);
>   
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 7d04a40..f9e26f2 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -1165,6 +1165,9 @@ static int __devinit mixer_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto fail;
>   
> +	/* attach mixer driver to common hdmi. */
> +	exynos_mixer_drv_attach(drm_hdmi_ctx);
> +
>   	/* register specific callback point to common hdmi. */
>   	exynos_mixer_ops_register(&mixer_ops);
>   

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

[RAHUL SHARMA]

------- Original Message -------
Sender : In-Ki Dae<inki.dae@samsung.com> S5/Senior Engineer/System S/W Lab./Samsung Electronics
Date : Sep 13, 2012 10:07 (GMT+05:30)
Title : Re: [PATCH 1/3] drm: exynos: hdmi: add exynos5 support to mixer driver



2012? 9? 13? ???? Joonyoung Shim<jy0922.shim@samsung.com>?? ??:
> On 09/13/2012 11:53 AM, Inki Dae wrote:
>
> -----Original Message-----
> From: Joonyoung Shim [mailto:jy0922.shim@samsung.com]
> Sent: Thursday, September 13, 2012 10:44 AM
> To: Rahul Sharma
> Cc: dri-devel@lists.freedesktop.org; sw0312.kim@samsung.com;
> inki.dae@samsung.com; kyungmin.park@samsung.com; prashanth.g@samsung.com;
> joshi@samsung.com; s.shirish@samsung.com; fahad.k@samsung.com;
> l.krishna@samsung.com; r.sh.open@gmail.com
> 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@samsung.com>
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
> ---
>    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
>
> Let's think to disassociate hdmi and mixer. I have plan to unify to one
> many things of exynos hdmi. Above problem occurs because exynos5 doesn't
> have video processor ip. Even if we use a field such is_soc_exynos5, the
> is_soc_exynos5 is unsuitable naming if other exynos SoC also doesn't
> have video processor ip.
> 

one more thing, exynos5 uses GScaler instead of Video processor. the GScaler can be also used as post processor but exynos5 spec has no any descriptions to this. so we should check that first and next let's update things related to hdmi.

Thanks Joonyoung, Inki,

I agree with you. mixer_resources_init_exynnos4,5 has code repetition here but Since Exy4
is non-DT and and Exy5 is DT enbled, method for getting resource are different. I need to
replace the platform_get_resource_byname to platform_get_resource by index.
Then I will be able to group the two functions into one. It will also help us when we add DT support
for Exynos4 hdmi driver. How does it look to you?

is_soc_exynos5 variable is used for 2 scenarios
1) Resource initialization. I will eliminate its usage by unifying the resource init function.
2) Video Processor. I can add a property in mixer node in device tree stating that VP support is
required or not. Based on that I can Disable VP support. By default it will be enabled.
With this method Exy4, Exy5, or any other SoC in future can define the property accordingly.

For Exynos-4 I will revert name to "s5p-mixer".

I have access to exy5 GScaler specs. It is providing very basic VPP fucntionality, can only see
CSC which again looks optional to me for hdmi. We may need to add GSC for scaling but can be
optioanlly accessed as v4l driver from application as doing in the current project. If you foresee
any impact on gscalar addition due to my changes please tell me.

regards.
Rahul Sharma
_______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>