[PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Ajay Kumar]

This patch enables device tree based discovery support for DP driver.
The driver is modified to handle platform data in both the cases:
with DT and non-DT.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Acked-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/exynos/exynos_dp_core.c |  161 ++++++++++++++++++++++++++++++---
 drivers/video/exynos/exynos_dp_core.h |    2 +
 2 files changed, 149 insertions(+), 14 deletions(-)

diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index d55470e..62932ba 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -18,6 +18,7 @@
 #include <linux/io.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
+#include <linux/of.h>
 
 #include <video/exynos_dp.h>
 
@@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_OF
+struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device *dev)
+{
+	struct device_node *dp_node = dev->of_node;
+	struct exynos_dp_platdata *pd;
+	struct video_info *dp_video_config;
+
+	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+	if (!pd) {
+		dev_err(dev, "memory allocation for pdata failed\n");
+		return ERR_PTR(-ENOMEM);
+	}
+	dp_video_config = devm_kzalloc(dev,
+				sizeof(*dp_video_config), GFP_KERNEL);
+
+	if (!dp_video_config) {
+		dev_err(dev, "memory allocation for video config failed\n");
+		return ERR_PTR(-ENOMEM);
+	}
+	pd->video_info = dp_video_config;
+
+	if (of_get_property(dp_node, "hsync-active-high", NULL))
+		dp_video_config->h_sync_polarity = 1;
+
+	if (of_get_property(dp_node, "vsync-active-high", NULL))
+		dp_video_config->v_sync_polarity = 1;
+
+	if (of_get_property(dp_node, "interlaced", NULL))
+		dp_video_config->interlaced = 1;
+
+	of_property_read_u32(dp_node, "samsung,color-space",
+				&dp_video_config->color_space);
+
+	of_property_read_u32(dp_node, "samsung,dynamic-range",
+				&dp_video_config->dynamic_range);
+
+	of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
+				&dp_video_config->ycbcr_coeff);
+
+	of_property_read_u32(dp_node, "samsung,color-depth",
+				&dp_video_config->color_depth);
+
+	of_property_read_u32(dp_node, "samsung,link-rate",
+				&dp_video_config->link_rate);
+
+	of_property_read_u32(dp_node, "samsung,lane-count",
+				&dp_video_config->lane_count);
+	return pd;
+}
+
+void exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
+{
+	struct device_node *dp_phy_node;
+	u32 phy_base;
+	void *virt_phy_base;
+
+	dp_phy_node = of_find_node_by_name(dp->dev->of_node, "dptx-phy");
+	if (!dp_phy_node) {
+		dp->dp_phy_addr = NULL;
+		return;
+	}
+
+	of_property_read_u32(dp_phy_node, "reg", &phy_base);
+	of_property_read_u32(dp_phy_node, "samsung,enable-mask",
+							&dp->enable_mask);
+	virt_phy_base = ioremap(phy_base, SZ_4);
+	if (!virt_phy_base) {
+		dev_err(dp->dev, "failed to ioremap dp-phy\n");
+		dp->dp_phy_addr = NULL;
+		return;
+	}
+	dp->dp_phy_addr = virt_phy_base;
+}
+
+void exynos_dp_phy_init(struct exynos_dp_device *dp)
+{
+	u32 reg;
+
+	reg = __raw_readl(dp->dp_phy_addr);
+	reg |= dp->enable_mask;
+	__raw_writel(reg, dp->dp_phy_addr);
+}
+
+void exynos_dp_phy_exit(struct exynos_dp_device *dp)
+{
+	u32 reg;
+
+	reg = __raw_readl(dp->dp_phy_addr);
+	reg &= ~(dp->enable_mask);
+	__raw_writel(reg, dp->dp_phy_addr);
+}
+#endif /* CONFIG_OF */
+
 static int __devinit exynos_dp_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -864,12 +958,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
 
 	int ret = 0;
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata) {
-		dev_err(&pdev->dev, "no platform data\n");
-		return -EINVAL;
-	}
-
 	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
 				GFP_KERNEL);
 	if (!dp) {
@@ -879,6 +967,21 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
 
 	dp->dev = &pdev->dev;
 
+	if (pdev->dev.of_node) {
+		pdata = exynos_dp_dt_parse_pdata(&pdev->dev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+
+		exynos_dp_dt_parse_phydata(dp);
+	} else {
+		pdata = pdev->dev.platform_data;
+	}
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "no platform data\n");
+		return -EINVAL;
+	}
+
 	dp->clock = devm_clk_get(&pdev->dev, "dp");
 	if (IS_ERR(dp->clock)) {
 		dev_err(&pdev->dev, "failed to get clock\n");
@@ -909,8 +1012,14 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
 	}
 
 	dp->video_info = pdata->video_info;
-	if (pdata->phy_init)
-		pdata->phy_init();
+
+	if (pdev->dev.of_node) {
+		if (dp->dp_phy_addr)
+			exynos_dp_phy_init(dp);
+	} else {
+		if (pdata->phy_init)
+			pdata->phy_init();
+	}
 
 	exynos_dp_init_dp(dp);
 
@@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
 	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
 	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
 
-	if (pdata && pdata->phy_exit)
-		pdata->phy_exit();
+	if (pdev->dev.of_node) {
+		if (dp->dp_phy_addr)
+			exynos_dp_phy_exit(dp);
+	} else {
+		if (pdata && pdata->phy_exit)
+			pdata->phy_exit();
+	}
 
 	clk_disable_unprepare(dp->clock);
 
@@ -968,8 +1082,13 @@ static int exynos_dp_suspend(struct device *dev)
 	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
 	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
 
-	if (pdata && pdata->phy_exit)
-		pdata->phy_exit();
+	if (dev->of_node) {
+		if (dp->dp_phy_addr)
+			exynos_dp_phy_exit(dp);
+	} else {
+		if (pdata && pdata->phy_exit)
+			pdata->phy_exit();
+	}
 
 	clk_disable_unprepare(dp->clock);
 
@@ -982,8 +1101,13 @@ static int exynos_dp_resume(struct device *dev)
 	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
 	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
 
-	if (pdata && pdata->phy_init)
-		pdata->phy_init();
+	if (dev->of_node) {
+		if (dp->dp_phy_addr)
+			exynos_dp_phy_init(dp);
+	} else {
+		if (pdata && pdata->phy_init)
+			pdata->phy_init();
+	}
 
 	clk_prepare_enable(dp->clock);
 
@@ -1013,6 +1137,14 @@ static const struct dev_pm_ops exynos_dp_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(exynos_dp_suspend, exynos_dp_resume)
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id exynos_dp_match[] = {
+	{ .compatible = "samsung,exynos5-dp" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, exynos_dp_match);
+#endif
+
 static struct platform_driver exynos_dp_driver = {
 	.probe		= exynos_dp_probe,
 	.remove		= __devexit_p(exynos_dp_remove),
@@ -1020,6 +1152,7 @@ static struct platform_driver exynos_dp_driver = {
 		.name	= "exynos-dp",
 		.owner	= THIS_MODULE,
 		.pm	= &exynos_dp_pm_ops,
+		.of_match_table = of_match_ptr(exynos_dp_match),
 	},
 };
 
diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
index 57b8a65..569858b 100644
--- a/drivers/video/exynos/exynos_dp_core.h
+++ b/drivers/video/exynos/exynos_dp_core.h
@@ -29,6 +29,8 @@ struct exynos_dp_device {
 	struct clk		*clock;
 	unsigned int		irq;
 	void __iomem		*reg_base;
+	void __iomem		*dp_phy_addr;
+	unsigned int		enable_mask;
 
 	struct video_info	*video_info;
 	struct link_train	link_train;
-- 
1.7.0.4

[PATCH V5 2/2] video: exynos_dp: device tree documentation

[Ajay Kumar]

Add documentation for the DT bindings in exynos display port driver.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 .../devicetree/bindings/video/exynos_dp.txt        |   80 ++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/exynos_dp.txt

diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
new file mode 100644
index 0000000..e19594b
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
@@ -0,0 +1,80 @@
+The Exynos display port interface should be configured based on the
+based on the type of panel connected to it.
+
+We use two nodes:
+	-display-port-controller node
+	-dptx-phy node(defined inside display-port-controller node)
+
+For the DP-PHY initialization, we use the dptx-phy node.
+Required properties for dptx-phy:
+	-reg:
+		Base address of DP PHY register.
+	-samsung,enable-mask:
+		The bit-mask used to enable/disable DP PHY.
+
+For the Panel initialization, we read data from display-port-controller node.
+Required properties for display-port-controller:
+	-compatible:
+		should be "samsung,exynos5-dp".
+	-reg:
+		physical base address of the controller and length
+		of memory mapped region.
+	-interrupts:
+		interrupt combiner values.
+	-interrupt-parent:
+		phandle to Interrupt combiner node.
+	-samsung,color-space:
+		input video data format.
+			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
+	-samsung,dynamic-range:
+		dynamic range for input video data.
+			VESA = 0, CEA = 1
+	-samsung,ycbcr-coeff:
+		YCbCr co-efficients for input video.
+			COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1
+	-samsung,color-depth:
+		number of bits per colour component.
+			COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
+	-samsung,link-rate:
+		link rate supported by the panel.
+			LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A
+	-samsung,lane-count:
+		number of lanes supported by the panel.
+			LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
+
+Optional properties for display-port-controller:
+	-interlaced:
+		interlace scan mode.
+			Progressive if defined, Interlaced if not defined
+	-vsync-active-high:
+		VSYNC polarity configuration.
+			High if defined, Low if not defined
+	-hsync-active-high:
+		HSYNC polarity configuration.
+			High if defined, Low if not defined
+
+Example:
+
+SOC specific portion:
+	display-port-controller {
+		compatible = "samsung,exynos5-dp";
+		reg = <0x145b0000 0x10000>;
+		interrupts = <10 3>;
+		interrupt-parent = <&combiner>;
+
+		dptx-phy {
+			reg = <0x10040720>;
+			samsung,enable-mask = <1>;
+		};
+
+        };
+
+Board Specific portion:
+	display-port-controller {
+		samsung,color-space = <0>;
+		samsung,dynamic-range = <0>;
+		samsung,ycbcr-coeff = <0>;
+		samsung,color-depth = <1>;
+		samsung,link-rate = <0x0a>;
+		samsung,lane-count = <2>;
+	};
-- 
1.7.0.4

Re: [PATCH V5 2/2] video: exynos_dp: device tree documentation

[Jingoo Han]

On Saturday, October 13, 2012 5:48 AM Ajay kumar wrote
> 
> Add documentation for the DT bindings in exynos display port driver.
> 
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  .../devicetree/bindings/video/exynos_dp.txt        |   80 ++++++++++++++++++++
>  1 files changed, 80 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/exynos_dp.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt
> b/Documentation/devicetree/bindings/video/exynos_dp.txt
> new file mode 100644
> index 0000000..e19594b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
> @@ -0,0 +1,80 @@
> +The Exynos display port interface should be configured based on the
> +based on the type of panel connected to it.

'based on' is duplicated. So, please fix it as bellows:

+The Exynos display port interface should be configured based on
+the type of panel connected to it.


> +
> +We use two nodes:
> +	-display-port-controller node
> +	-dptx-phy node(defined inside display-port-controller node)
> +
> +For the DP-PHY initialization, we use the dptx-phy node.
> +Required properties for dptx-phy:
> +	-reg:
> +		Base address of DP PHY register.
> +	-samsung,enable-mask:
> +		The bit-mask used to enable/disable DP PHY.
> +
> +For the Panel initialization, we read data from display-port-controller node.
> +Required properties for display-port-controller:
> +	-compatible:
> +		should be "samsung,exynos5-dp".
> +	-reg:
> +		physical base address of the controller and length
> +		of memory mapped region.
> +	-interrupts:
> +		interrupt combiner values.
> +	-interrupt-parent:
> +		phandle to Interrupt combiner node.
> +	-samsung,color-space:
> +		input video data format.
> +			COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2
> +	-samsung,dynamic-range:
> +		dynamic range for input video data.
> +			VESA = 0, CEA = 1
> +	-samsung,ycbcr-coeff:
> +		YCbCr co-efficients for input video.
> +			COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1
> +	-samsung,color-depth:
> +		number of bits per colour component.
> +			COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 = 3
> +	-samsung,link-rate:
> +		link rate supported by the panel.
> +			LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A
> +	-samsung,lane-count:
> +		number of lanes supported by the panel.
> +			LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
> +
> +Optional properties for display-port-controller:
> +	-interlaced:
> +		interlace scan mode.
> +			Progressive if defined, Interlaced if not defined
> +	-vsync-active-high:
> +		VSYNC polarity configuration.
> +			High if defined, Low if not defined
> +	-hsync-active-high:
> +		HSYNC polarity configuration.
> +			High if defined, Low if not defined
> +
> +Example:
> +
> +SOC specific portion:
> +	display-port-controller {
> +		compatible = "samsung,exynos5-dp";
> +		reg = <0x145b0000 0x10000>;
> +		interrupts = <10 3>;
> +		interrupt-parent = <&combiner>;
> +
> +		dptx-phy {
> +			reg = <0x10040720>;
> +			samsung,enable-mask = <1>;
> +		};
> +
> +        };
> +
> +Board Specific portion:
> +	display-port-controller {
> +		samsung,color-space = <0>;
> +		samsung,dynamic-range = <0>;
> +		samsung,ycbcr-coeff = <0>;
> +		samsung,color-depth = <1>;
> +		samsung,link-rate = <0x0a>;
> +		samsung,lane-count = <2>;
> +	};
> --
> 1.7.0.4

Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Sylwester Nawrocki]

On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> This patch enables device tree based discovery support for DP driver.
> The driver is modified to handle platform data in both the cases:
> with DT and non-DT.
> 
> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> Acked-by: Jingoo Han<jg1.han@samsung.com>
> ---
>   drivers/video/exynos/exynos_dp_core.c |  161 ++++++++++++++++++++++++++++++---
>   drivers/video/exynos/exynos_dp_core.h |    2 +
>   2 files changed, 149 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index d55470e..62932ba 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -18,6 +18,7 @@
>   #include<linux/io.h>
>   #include<linux/interrupt.h>
>   #include<linux/delay.h>
> +#include<linux/of.h>
> 
>   #include<video/exynos_dp.h>
> 
> @@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
>   	return IRQ_HANDLED;
>   }
> 
> +#ifdef CONFIG_OF
> +struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device *dev)
> +{
> +	struct device_node *dp_node = dev->of_node;
> +	struct exynos_dp_platdata *pd;
> +	struct video_info *dp_video_config;
> +
> +	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> +	if (!pd) {
> +		dev_err(dev, "memory allocation for pdata failed\n");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	dp_video_config = devm_kzalloc(dev,
> +				sizeof(*dp_video_config), GFP_KERNEL);
> +
> +	if (!dp_video_config) {
> +		dev_err(dev, "memory allocation for video config failed\n");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +	pd->video_info = dp_video_config;
> +
> +	if (of_get_property(dp_node, "hsync-active-high", NULL))
> +		dp_video_config->h_sync_polarity = 1;
> +
> +	if (of_get_property(dp_node, "vsync-active-high", NULL))
> +		dp_video_config->v_sync_polarity = 1;
> +
> +	if (of_get_property(dp_node, "interlaced", NULL))
> +		dp_video_config->interlaced = 1;

of_property_read_bool() could also be used here.

> +
> +	of_property_read_u32(dp_node, "samsung,color-space",
> +				&dp_video_config->color_space);
> +
> +	of_property_read_u32(dp_node, "samsung,dynamic-range",
> +				&dp_video_config->dynamic_range);
> +
> +	of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
> +				&dp_video_config->ycbcr_coeff);
> +
> +	of_property_read_u32(dp_node, "samsung,color-depth",
> +				&dp_video_config->color_depth);
> +
> +	of_property_read_u32(dp_node, "samsung,link-rate",
> +				&dp_video_config->link_rate);
> +
> +	of_property_read_u32(dp_node, "samsung,lane-count",
> +				&dp_video_config->lane_count);

Shouldn't it be checked whether all these required properties are present ? 
If someone forgets to specify any one the driver will silently ignore it,
not giving a clue what's wrong.

> +	return pd;
> +}
> +
> +void exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
> +{
> +	struct device_node *dp_phy_node;
> +	u32 phy_base;
> +	void *virt_phy_base;
> +
> +	dp_phy_node = of_find_node_by_name(dp->dev->of_node, "dptx-phy");
> +	if (!dp_phy_node) {
> +		dp->dp_phy_addr = NULL;

Is this required ? You've allocated dp with kzalloc().

> +		return;
> +	}
> +
> +	of_property_read_u32(dp_phy_node, "reg",&phy_base);
> +	of_property_read_u32(dp_phy_node, "samsung,enable-mask",
> +							&dp->enable_mask);

I think you should check the return values, these are required properties.
Some error logs might be useful here.

> +	virt_phy_base = ioremap(phy_base, SZ_4);

How about assigning to dp->dp_phy_addr directly and gettting rid of
virt_phy_base ?

> +	if (!virt_phy_base) {
> +		dev_err(dp->dev, "failed to ioremap dp-phy\n");
> +		dp->dp_phy_addr = NULL;
> +		return;
> +	}
> +	dp->dp_phy_addr = virt_phy_base;
> +}
> +
> +void exynos_dp_phy_init(struct exynos_dp_device *dp)
> +{
> +	u32 reg;
> +
> +	reg = __raw_readl(dp->dp_phy_addr);
> +	reg |= dp->enable_mask;
> +	__raw_writel(reg, dp->dp_phy_addr);
> +}
> +
> +void exynos_dp_phy_exit(struct exynos_dp_device *dp)
> +{
> +	u32 reg;
> +
> +	reg = __raw_readl(dp->dp_phy_addr);
> +	reg&= ~(dp->enable_mask);
> +	__raw_writel(reg, dp->dp_phy_addr);
> +}
> +#endif /* CONFIG_OF */
> +
>   static int __devinit exynos_dp_probe(struct platform_device *pdev)
>   {
>   	struct resource *res;
> @@ -864,12 +958,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> 
>   	int ret = 0;
> 
> -	pdata = pdev->dev.platform_data;
> -	if (!pdata) {
> -		dev_err(&pdev->dev, "no platform data\n");
> -		return -EINVAL;
> -	}
> -
>   	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
>   				GFP_KERNEL);
>   	if (!dp) {
> @@ -879,6 +967,21 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> 
>   	dp->dev =&pdev->dev;
> 
> +	if (pdev->dev.of_node) {
> +		pdata = exynos_dp_dt_parse_pdata(&pdev->dev);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);
> +
> +		exynos_dp_dt_parse_phydata(dp);
> +	} else {
> +		pdata = pdev->dev.platform_data;
> +	}
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "no platform data\n");
> +		return -EINVAL;
> +	}
> +

This check is only reqired for non-dt case, might be worth to move
it under else statement above.

>   	dp->clock = devm_clk_get(&pdev->dev, "dp");
>   	if (IS_ERR(dp->clock)) {
>   		dev_err(&pdev->dev, "failed to get clock\n");
> @@ -909,8 +1012,14 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>   	}
> 
>   	dp->video_info = pdata->video_info;
> -	if (pdata->phy_init)
> -		pdata->phy_init();
> +
> +	if (pdev->dev.of_node) {
> +		if (dp->dp_phy_addr)
> +			exynos_dp_phy_init(dp);
> +	} else {
> +		if (pdata->phy_init)
> +			pdata->phy_init();
> +	}
> 
>   	exynos_dp_init_dp(dp);
> 
> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>   	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>   	struct exynos_dp_device *dp = platform_get_drvdata(pdev);

This doesn't belong to your patch, but the above 2 lines could be written as:

  	struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);

Same in functions exynos_dp_suspend() and exynos_dp_resume().

> 
> -	if (pdata&&  pdata->phy_exit)
> -		pdata->phy_exit();
> +	if (pdev->dev.of_node) {
> +		if (dp->dp_phy_addr)
> +			exynos_dp_phy_exit(dp);
> +	} else {
> +		if (pdata&&  pdata->phy_exit)
> +			pdata->phy_exit();
> +	}

It is not possible to have valid dp->dp_phy_addr pointer without
valid pdev->dev.of_node, is it ?

Can't this (and all similar occurrences) be simplified to:

	if (dp->dp_phy_addr)
		exynos_dp_phy_exit(dp);
	else if (pdata->phy_exit)
		pdata->phy_exit();
?

It just requires dp->dp_phy_addr being NULL for non-dt case. 
pdata is never NULL, otherwise probe() would have already failed.

>   	clk_disable_unprepare(dp->clock);
> 
> @@ -968,8 +1082,13 @@ static int exynos_dp_suspend(struct device *dev)
>   	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>   	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> 
> -	if (pdata&&  pdata->phy_exit)
> -		pdata->phy_exit();
> +	if (dev->of_node) {
> +		if (dp->dp_phy_addr)
> +			exynos_dp_phy_exit(dp);
> +	} else {
> +		if (pdata&&  pdata->phy_exit)
> +			pdata->phy_exit();
> +	}
> 
>   	clk_disable_unprepare(dp->clock);
> 
> @@ -982,8 +1101,13 @@ static int exynos_dp_resume(struct device *dev)
>   	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>   	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> 
> -	if (pdata&&  pdata->phy_init)
> -		pdata->phy_init();
> +	if (dev->of_node) {
> +		if (dp->dp_phy_addr)
> +			exynos_dp_phy_init(dp);
> +	} else {
> +		if (pdata&&  pdata->phy_init)
> +			pdata->phy_init();
> +	}
> 
>   	clk_prepare_enable(dp->clock);
> 
> @@ -1013,6 +1137,14 @@ static const struct dev_pm_ops exynos_dp_pm_ops = {
>   	SET_SYSTEM_SLEEP_PM_OPS(exynos_dp_suspend, exynos_dp_resume)
>   };
> 
> +#ifdef CONFIG_OF
> +static const struct of_device_id exynos_dp_match[] = {
> +	{ .compatible = "samsung,exynos5-dp" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, exynos_dp_match);
> +#endif
> +

How about just dropping this #ifdef, since you're using of_match_ptr() ?

>   static struct platform_driver exynos_dp_driver = {
>   	.probe		= exynos_dp_probe,
>   	.remove		= __devexit_p(exynos_dp_remove),
> @@ -1020,6 +1152,7 @@ static struct platform_driver exynos_dp_driver = {
>   		.name	= "exynos-dp",
>   		.owner	= THIS_MODULE,
>   		.pm	=&exynos_dp_pm_ops,
> +		.of_match_table = of_match_ptr(exynos_dp_match),
>   	},
>   };
> 
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index 57b8a65..569858b 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -29,6 +29,8 @@ struct exynos_dp_device {
>   	struct clk		*clock;
>   	unsigned int		irq;
>   	void __iomem		*reg_base;
> +	void __iomem		*dp_phy_addr;

Nit: "dp" is already within the structure name. How about just
     naming it phy_addr ? dp->phy_addr might look better than 
     dp->dp_phy_addr ;)

> +	unsigned int		enable_mask;
> 
>   	struct video_info	*video_info;
>   	struct link_train	link_train;

Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Tomasz Figa]

Dnia piątek, 12 października 2012 23:44:05 Sylwester Nawrocki pisze:
> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> > This patch enables device tree based discovery support for DP driver.
> > The driver is modified to handle platform data in both the cases:
> > with DT and non-DT.
> > 
> > Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> > Acked-by: Jingoo Han<jg1.han@samsung.com>
> > ---
> > 
> >   drivers/video/exynos/exynos_dp_core.c |  161
> >   ++++++++++++++++++++++++++++++---
> >   drivers/video/exynos/exynos_dp_core.h |    2 +
> >   2 files changed, 149 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/video/exynos/exynos_dp_core.c
> > b/drivers/video/exynos/exynos_dp_core.c index d55470e..62932ba 100644
> > --- a/drivers/video/exynos/exynos_dp_core.c
> > +++ b/drivers/video/exynos/exynos_dp_core.c
> > @@ -18,6 +18,7 @@
> > 
> >   #include<linux/io.h>
> >   #include<linux/interrupt.h>
> >   #include<linux/delay.h>
> > 
> > +#include<linux/of.h>
> > 
> >   #include<video/exynos_dp.h>
> > 
> > @@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq,
> > void *arg)> 
> >   	return IRQ_HANDLED;
> >   
> >   }
> > 
> > +#ifdef CONFIG_OF
> > +struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device
> > *dev)
> > +{
> > +	struct device_node *dp_node = dev->of_node;
> > +	struct exynos_dp_platdata *pd;
> > +	struct video_info *dp_video_config;
> > +
> > +	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > +	if (!pd) {
> > +		dev_err(dev, "memory allocation for pdata failed\n");
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +	dp_video_config = devm_kzalloc(dev,
> > +				sizeof(*dp_video_config), GFP_KERNEL);
> > +
> > +	if (!dp_video_config) {
> > +		dev_err(dev, "memory allocation for video config failed\n");
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +	pd->video_info = dp_video_config;
> > +
> > +	if (of_get_property(dp_node, "hsync-active-high", NULL))
> > +		dp_video_config->h_sync_polarity = 1;
> > +
> > +	if (of_get_property(dp_node, "vsync-active-high", NULL))
> > +		dp_video_config->v_sync_polarity = 1;
> > +
> > +	if (of_get_property(dp_node, "interlaced", NULL))
> > +		dp_video_config->interlaced = 1;
> 
> of_property_read_bool() could also be used here.

Wouldn't it make the property mandatory?

I would just use of_find_property here, instead of of_get_property.

Best regards,
Tomasz Figa

Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Sylwester Nawrocki]

On 10/12/2012 11:54 PM, Tomasz Figa wrote:
>>> +	if (of_get_property(dp_node, "interlaced", NULL))
>>> +		dp_video_config->interlaced = 1;
>>
>> of_property_read_bool() could also be used here.
> 
> Wouldn't it make the property mandatory?

No, it wouldn't. of_property_read_bool() uses of_find_property() 
internally. It just returns true if property is found or false 
otherwise. Anyway, it appears of_get_property(..., NULL) pattern
is commonly used for boolean properties.

> I would just use of_find_property here, instead of of_get_property.
> 
> Best regards,
> Tomasz Figa

Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Tomasz Figa]

On Saturday 13 of October 2012 00:14:39 Sylwester Nawrocki wrote:
> On 10/12/2012 11:54 PM, Tomasz Figa wrote:
> >>> +	if (of_get_property(dp_node, "interlaced", NULL))
> >>> +		dp_video_config->interlaced = 1;
> >> 
> >> of_property_read_bool() could also be used here.
> > 
> > Wouldn't it make the property mandatory?
> 
> No, it wouldn't. of_property_read_bool() uses of_find_property()
> internally. It just returns true if property is found or false
> otherwise.

Right, sorry. I thought that all of_property_read_* return error in case 
of missing property.

> Anyway, it appears of_get_property(..., NULL) pattern
> is commonly used for boolean properties.

I guess all three of them should be fine in this case, but since there is 
a dedicated function for bool, it might be the best solution here indeed.

Best regards,
Tomasz Figa

Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Jingoo Han]

On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
> 
> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> > This patch enables device tree based discovery support for DP driver.
> > The driver is modified to handle platform data in both the cases:
> > with DT and non-DT.
> >
> > Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> > Acked-by: Jingoo Han<jg1.han@samsung.com>
> > ---
> >   drivers/video/exynos/exynos_dp_core.c |  161 ++++++++++++++++++++++++++++++---
> >   drivers/video/exynos/exynos_dp_core.h |    2 +
> >   2 files changed, 149 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> > index d55470e..62932ba 100644
> > --- a/drivers/video/exynos/exynos_dp_core.c
> > +++ b/drivers/video/exynos/exynos_dp_core.c
> > @@ -18,6 +18,7 @@
> >   #include<linux/io.h>
> >   #include<linux/interrupt.h>
> >   #include<linux/delay.h>
> > +#include<linux/of.h>
> >
> >   #include<video/exynos_dp.h>
> >
> > @@ -856,6 +857,99 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
> >   	return IRQ_HANDLED;
> >   }
> >
> > +#ifdef CONFIG_OF
> > +struct exynos_dp_platdata *exynos_dp_dt_parse_pdata(struct device *dev)
> > +{
> > +	struct device_node *dp_node = dev->of_node;
> > +	struct exynos_dp_platdata *pd;
> > +	struct video_info *dp_video_config;
> > +
> > +	pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
> > +	if (!pd) {
> > +		dev_err(dev, "memory allocation for pdata failed\n");
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +	dp_video_config = devm_kzalloc(dev,
> > +				sizeof(*dp_video_config), GFP_KERNEL);
> > +
> > +	if (!dp_video_config) {
> > +		dev_err(dev, "memory allocation for video config failed\n");
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +	pd->video_info = dp_video_config;
> > +
> > +	if (of_get_property(dp_node, "hsync-active-high", NULL))
> > +		dp_video_config->h_sync_polarity = 1;
> > +
> > +	if (of_get_property(dp_node, "vsync-active-high", NULL))
> > +		dp_video_config->v_sync_polarity = 1;
> > +
> > +	if (of_get_property(dp_node, "interlaced", NULL))
> > +		dp_video_config->interlaced = 1;
> 
> of_property_read_bool() could also be used here.


OK, right.
of_property_read_bool() would be better.

> 
> > +
> > +	of_property_read_u32(dp_node, "samsung,color-space",
> > +				&dp_video_config->color_space);
> > +
> > +	of_property_read_u32(dp_node, "samsung,dynamic-range",
> > +				&dp_video_config->dynamic_range);
> > +
> > +	of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
> > +				&dp_video_config->ycbcr_coeff);
> > +
> > +	of_property_read_u32(dp_node, "samsung,color-depth",
> > +				&dp_video_config->color_depth);
> > +
> > +	of_property_read_u32(dp_node, "samsung,link-rate",
> > +				&dp_video_config->link_rate);
> > +
> > +	of_property_read_u32(dp_node, "samsung,lane-count",
> > +				&dp_video_config->lane_count);
> 
> Shouldn't it be checked whether all these required properties are present ?
> If someone forgets to specify any one the driver will silently ignore it,
> not giving a clue what's wrong.

OK, right.

> 
> > +	return pd;
> > +}
> > +
> > +void exynos_dp_dt_parse_phydata(struct exynos_dp_device *dp)
> > +{
> > +	struct device_node *dp_phy_node;
> > +	u32 phy_base;
> > +	void *virt_phy_base;
> > +
> > +	dp_phy_node = of_find_node_by_name(dp->dev->of_node, "dptx-phy");
> > +	if (!dp_phy_node) {
> > +		dp->dp_phy_addr = NULL;
> 
> Is this required ? You've allocated dp with kzalloc().

OK, right.

> 
> > +		return;
> > +	}
> > +
> > +	of_property_read_u32(dp_phy_node, "reg",&phy_base);
> > +	of_property_read_u32(dp_phy_node, "samsung,enable-mask",
> > +							&dp->enable_mask);
> 
> I think you should check the return values, these are required properties.
> Some error logs might be useful here.

OK, right.

> 
> > +	virt_phy_base = ioremap(phy_base, SZ_4);
> 
> How about assigning to dp->dp_phy_addr directly and gettting rid of
> virt_phy_base ?

OK, right.
There is no reason, virt_phy_base should be removed.

> 
> > +	if (!virt_phy_base) {
> > +		dev_err(dp->dev, "failed to ioremap dp-phy\n");
> > +		dp->dp_phy_addr = NULL;
> > +		return;
> > +	}
> > +	dp->dp_phy_addr = virt_phy_base;
> > +}
> > +
> > +void exynos_dp_phy_init(struct exynos_dp_device *dp)
> > +{
> > +	u32 reg;
> > +
> > +	reg = __raw_readl(dp->dp_phy_addr);
> > +	reg |= dp->enable_mask;
> > +	__raw_writel(reg, dp->dp_phy_addr);
> > +}
> > +
> > +void exynos_dp_phy_exit(struct exynos_dp_device *dp)
> > +{
> > +	u32 reg;
> > +
> > +	reg = __raw_readl(dp->dp_phy_addr);
> > +	reg&= ~(dp->enable_mask);
> > +	__raw_writel(reg, dp->dp_phy_addr);
> > +}
> > +#endif /* CONFIG_OF */
> > +
> >   static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >   {
> >   	struct resource *res;
> > @@ -864,12 +958,6 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >
> >   	int ret = 0;
> >
> > -	pdata = pdev->dev.platform_data;
> > -	if (!pdata) {
> > -		dev_err(&pdev->dev, "no platform data\n");
> > -		return -EINVAL;
> > -	}
> > -
> >   	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
> >   				GFP_KERNEL);
> >   	if (!dp) {
> > @@ -879,6 +967,21 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >
> >   	dp->dev =&pdev->dev;
> >
> > +	if (pdev->dev.of_node) {
> > +		pdata = exynos_dp_dt_parse_pdata(&pdev->dev);
> > +		if (IS_ERR(pdata))
> > +			return PTR_ERR(pdata);
> > +
> > +		exynos_dp_dt_parse_phydata(dp);
> > +	} else {
> > +		pdata = pdev->dev.platform_data;
> > +	}
> > +
> > +	if (!pdata) {
> > +		dev_err(&pdev->dev, "no platform data\n");
> > +		return -EINVAL;
> > +	}
> > +
> 
> This check is only reqired for non-dt case, might be worth to move
> it under else statement above.

OK, right.

> 
> >   	dp->clock = devm_clk_get(&pdev->dev, "dp");
> >   	if (IS_ERR(dp->clock)) {
> >   		dev_err(&pdev->dev, "failed to get clock\n");
> > @@ -909,8 +1012,14 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >   	}
> >
> >   	dp->video_info = pdata->video_info;
> > -	if (pdata->phy_init)
> > -		pdata->phy_init();
> > +
> > +	if (pdev->dev.of_node) {
> > +		if (dp->dp_phy_addr)
> > +			exynos_dp_phy_init(dp);
> > +	} else {
> > +		if (pdata->phy_init)
> > +			pdata->phy_init();
> > +	}
> >
> >   	exynos_dp_init_dp(dp);
> >
> > @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >   	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> >   	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> 
> This doesn't belong to your patch, but the above 2 lines could be written as:
> 
>   	struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
> 
> Same in functions exynos_dp_suspend() and exynos_dp_resume().

No, above 2 lines cannot be reduced to 1 line, as you mentioned.
This is because it makes build error.

> 
> >
> > -	if (pdata&&  pdata->phy_exit)
> > -		pdata->phy_exit();
> > +	if (pdev->dev.of_node) {
> > +		if (dp->dp_phy_addr)
> > +			exynos_dp_phy_exit(dp);
> > +	} else {
> > +		if (pdata&&  pdata->phy_exit)
> > +			pdata->phy_exit();
> > +	}
> 
> It is not possible to have valid dp->dp_phy_addr pointer without
> valid pdev->dev.of_node, is it ?

Right, however, I prefer coding style as bellow:

if (pdev->dev.of_node)
	DT
else
	non-DT

> 
> Can't this (and all similar occurrences) be simplified to:
> 
> 	if (dp->dp_phy_addr)
> 		exynos_dp_phy_exit(dp);
> 	else if (pdata->phy_exit)
> 		pdata->phy_exit();
> ?
> 
> It just requires dp->dp_phy_addr being NULL for non-dt case.
> pdata is never NULL, otherwise probe() would have already failed.

OK, right.
pdata can be removed.

> 
> >   	clk_disable_unprepare(dp->clock);
> >
> > @@ -968,8 +1082,13 @@ static int exynos_dp_suspend(struct device *dev)
> >   	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> >   	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >
> > -	if (pdata&&  pdata->phy_exit)
> > -		pdata->phy_exit();
> > +	if (dev->of_node) {
> > +		if (dp->dp_phy_addr)
> > +			exynos_dp_phy_exit(dp);
> > +	} else {
> > +		if (pdata&&  pdata->phy_exit)
> > +			pdata->phy_exit();
> > +	}
> >
> >   	clk_disable_unprepare(dp->clock);
> >
> > @@ -982,8 +1101,13 @@ static int exynos_dp_resume(struct device *dev)
> >   	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> >   	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >
> > -	if (pdata&&  pdata->phy_init)
> > -		pdata->phy_init();
> > +	if (dev->of_node) {
> > +		if (dp->dp_phy_addr)
> > +			exynos_dp_phy_init(dp);
> > +	} else {
> > +		if (pdata&&  pdata->phy_init)
> > +			pdata->phy_init();
> > +	}
> >
> >   	clk_prepare_enable(dp->clock);
> >
> > @@ -1013,6 +1137,14 @@ static const struct dev_pm_ops exynos_dp_pm_ops = {
> >   	SET_SYSTEM_SLEEP_PM_OPS(exynos_dp_suspend, exynos_dp_resume)
> >   };
> >
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id exynos_dp_match[] = {
> > +	{ .compatible = "samsung,exynos5-dp" },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, exynos_dp_match);
> > +#endif
> > +
> 
> How about just dropping this #ifdef, since you're using of_match_ptr() ?

OK, right.

> 
> >   static struct platform_driver exynos_dp_driver = {
> >   	.probe		= exynos_dp_probe,
> >   	.remove		= __devexit_p(exynos_dp_remove),
> > @@ -1020,6 +1152,7 @@ static struct platform_driver exynos_dp_driver = {
> >   		.name	= "exynos-dp",
> >   		.owner	= THIS_MODULE,
> >   		.pm	=&exynos_dp_pm_ops,
> > +		.of_match_table = of_match_ptr(exynos_dp_match),
> >   	},
> >   };
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> > index 57b8a65..569858b 100644
> > --- a/drivers/video/exynos/exynos_dp_core.h
> > +++ b/drivers/video/exynos/exynos_dp_core.h
> > @@ -29,6 +29,8 @@ struct exynos_dp_device {
> >   	struct clk		*clock;
> >   	unsigned int		irq;
> >   	void __iomem		*reg_base;
> > +	void __iomem		*dp_phy_addr;
> 
> Nit: "dp" is already within the structure name. How about just
>      naming it phy_addr ? dp->phy_addr might look better than
>      dp->dp_phy_addr ;)

OK, right.

> 
> > +	unsigned int		enable_mask;
> >
> >   	struct video_info	*video_info;
> >   	struct link_train	link_train;

Ajay,

When CONFIG_OF is not enabled, it makes build errors.
In addition, exynos_dp_dt_parse_phydata needs return value,
instead of returning void.


Best regards,
Jingoo Han

> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Sylwester Nawrocki]

On 10/15/2012 10:14 AM, Jingoo Han wrote:
> On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
>>
>> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
>>> This patch enables device tree based discovery support for DP driver.
>>> The driver is modified to handle platform data in both the cases:
>>> with DT and non-DT.
>>>
>>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
>>> Acked-by: Jingoo Han<jg1.han@samsung.com>
>>> ---
>>>    drivers/video/exynos/exynos_dp_core.c |  161 ++++++++++++++++++++++++++++++---
>>>    drivers/video/exynos/exynos_dp_core.h |    2 +
>>>    2 files changed, 149 insertions(+), 14 deletions(-)
...
>>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>>>    	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>>>    	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>
>> This doesn't belong to your patch, but the above 2 lines could be written as:
>>
>>    	struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
>>
>> Same in functions exynos_dp_suspend() and exynos_dp_resume().
>
> No, above 2 lines cannot be reduced to 1 line, as you mentioned.
> This is because it makes build error.

Sorry, my bad. It looks fine in case of exynos_dp_remove().

But at exynos_dp_suspend/resume() there is something like:


static int exynos_dp_suspend(struct device *dev)
{
	struct platform_device *pdev = to_platform_device(dev);
	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
	struct exynos_dp_device *dp = platform_get_drvdata(pdev);


You need only pdata and dp there. I think that simpler form would
do as well:

	struct exynos_dp_device *dp = dev_get_drvdata(dev);
	struct exynos_dp_platdata *pdata = dev->platform_data;


Sorry, this is just a nitpicking.

BTW, shouldn't CONFIG_EXYNOS_VIDEO depend on ARCH_EXYNOS ?


--
Regards,
Sylwester

Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Jingoo Han]

On Tuesday, October 16, 2012 6:14 AM Sylwester Nawrocki wrote
> 
> On 10/15/2012 10:14 AM, Jingoo Han wrote:
> > On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
> >>
> >> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
> >>> This patch enables device tree based discovery support for DP driver.
> >>> The driver is modified to handle platform data in both the cases:
> >>> with DT and non-DT.
> >>>
> >>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
> >>> Acked-by: Jingoo Han<jg1.han@samsung.com>
> >>> ---
> >>>    drivers/video/exynos/exynos_dp_core.c |  161 ++++++++++++++++++++++++++++++---
> >>>    drivers/video/exynos/exynos_dp_core.h |    2 +
> >>>    2 files changed, 149 insertions(+), 14 deletions(-)
> ...
> >>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >>>    	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> >>>    	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> >>
> >> This doesn't belong to your patch, but the above 2 lines could be written as:
> >>
> >>    	struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
> >>
> >> Same in functions exynos_dp_suspend() and exynos_dp_resume().
> >
> > No, above 2 lines cannot be reduced to 1 line, as you mentioned.
> > This is because it makes build error.
> 
> Sorry, my bad. It looks fine in case of exynos_dp_remove().
> 
> But at exynos_dp_suspend/resume() there is something like:
> 
> 
> static int exynos_dp_suspend(struct device *dev)
> {
> 	struct platform_device *pdev = to_platform_device(dev);
> 	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
> 	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> 
> 
> You need only pdata and dp there. I think that simpler form would
> do as well:
> 
> 	struct exynos_dp_device *dp = dev_get_drvdata(dev);
> 	struct exynos_dp_platdata *pdata = dev->platform_data;

OK, I see.
I will accept your suggestion, because it is helpful to reduce
lines. Then, I will send v8 patch, soon.

> 
> 
> Sorry, this is just a nitpicking.
> 
> BTW, shouldn't CONFIG_EXYNOS_VIDEO depend on ARCH_EXYNOS ?
> 
> 
> --
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 1/2] video: exynos_dp: Add device tree support to DP driver

[Sylwester Nawrocki]

On 10/16/2012 04:02 AM, Jingoo Han wrote:
> On Tuesday, October 16, 2012 6:14 AM Sylwester Nawrocki wrote
>> On 10/15/2012 10:14 AM, Jingoo Han wrote:
>>> On Saturday, October 13, 2012 6:44 AM Sylwester Nawrocki wrote
>>>>
>>>> On 10/12/2012 10:47 PM, Ajay Kumar wrote:
>>>>> This patch enables device tree based discovery support for DP driver.
>>>>> The driver is modified to handle platform data in both the cases:
>>>>> with DT and non-DT.
>>>>>
>>>>> Signed-off-by: Ajay Kumar<ajaykumar.rs@samsung.com>
>>>>> Acked-by: Jingoo Han<jg1.han@samsung.com>
>>>>> ---
>>>>>    drivers/video/exynos/exynos_dp_core.c |  161 ++++++++++++++++++++++++++++++---
>>>>>    drivers/video/exynos/exynos_dp_core.h |    2 +
>>>>>    2 files changed, 149 insertions(+), 14 deletions(-)
>> ...
>>>>> @@ -953,8 +1062,13 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>>>>>    	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>>>>>    	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>>>
>>>> This doesn't belong to your patch, but the above 2 lines could be written as:
>>>>
>>>>    	struct exynos_dp_device *dp = dev_get_drvdata(&pdev->dev);
>>>>
>>>> Same in functions exynos_dp_suspend() and exynos_dp_resume().
>>>
>>> No, above 2 lines cannot be reduced to 1 line, as you mentioned.
>>> This is because it makes build error.
>>
>> Sorry, my bad. It looks fine in case of exynos_dp_remove().
>>
>> But at exynos_dp_suspend/resume() there is something like:
>>
>>
>> static int exynos_dp_suspend(struct device *dev)
>> {
>> 	struct platform_device *pdev = to_platform_device(dev);
>> 	struct exynos_dp_platdata *pdata = pdev->dev.platform_data;
>> 	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
>>
>>
>> You need only pdata and dp there. I think that simpler form would
>> do as well:
>>
>> 	struct exynos_dp_device *dp = dev_get_drvdata(dev);
>> 	struct exynos_dp_platdata *pdata = dev->platform_data;
> 
> OK, I see.
> I will accept your suggestion, because it is helpful to reduce
> lines. Then, I will send v8 patch, soon.

OK. I thought about it more as a candidate for a separate. But you seem
to have already sent new version, it's probably fine this way too.

--
Regards,
Sylwester