[PATCHv2 0/5] device tree support for exynos rotator

[PATCHv2 0/5] device tree support for exynos rotator

[Chanho Park]

This patchset includes device tree support for rotator of exynos4210/4x12/5250.
Unfortunately, each of them has slightly different limitations of image size.
The rotator can support several image formats(RGB888/555, YCbCr422/420_2/3p).
For convinience, however, exynos drm rotator driver only support RGB888 and
YCbCr420 2-Plane formats. For example, the exynos4210 has 16k x 16k maximum size
. But rest of them has 8k x 8k. In addition, RGB888 X/Y pixel size of exynos5250
should be multiple of 2. But others are multiple of 4.
Thus, we should define different compatibles and limit tables for each chipsets.

Changes from v1:
 - Added exynos5250 binding.
 - Move limit table into driver code from DT-nodes

Chanho Park (5):
  drm/exynos: add device tree support for rotator
  ARM: dts: Add rotator node for exynos4210
  ARM: dts: Add rotator node for exynos4x12
  ARM: dts: Add rotator node for exynos5250
  ARM: dts: Add dt binding documentation for exynos rotator

 .../devicetree/bindings/gpu/samsung-rotator.txt    |   26 +++++
 arch/arm/boot/dts/exynos4.dtsi                     |    9 ++
 arch/arm/boot/dts/exynos4x12.dtsi                  |    4 +
 arch/arm/boot/dts/exynos5250.dtsi                  |    9 ++
 drivers/gpu/drm/exynos/exynos_drm_rotator.c        |  109 +++++++++++++++-----
 5 files changed, 129 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt

-- 
1.7.9.5

[PATCHv2 0/5] device tree support for exynos rotator

[Chanho Park]

This patchset includes device tree support for rotator of exynos4210/4x12/5250.
Unfortunately, each of them has slightly different limitations of image size.
The rotator can support several image formats(RGB888/555, YCbCr422/420_2/3p).
For convinience, however, exynos drm rotator driver only support RGB888 and
YCbCr420 2-Plane formats. For example, the exynos4210 has 16k x 16k maximum size
. But rest of them has 8k x 8k. In addition, RGB888 X/Y pixel size of exynos5250
should be multiple of 2. But others are multiple of 4.
Thus, we should define different compatibles and limit tables for each chipsets.

Changes from v1:
 - Added exynos5250 binding.
 - Move limit table into driver code from DT-nodes

Chanho Park (5):
  drm/exynos: add device tree support for rotator
  ARM: dts: Add rotator node for exynos4210
  ARM: dts: Add rotator node for exynos4x12
  ARM: dts: Add rotator node for exynos5250
  ARM: dts: Add dt binding documentation for exynos rotator

 .../devicetree/bindings/gpu/samsung-rotator.txt    |   26 +++++
 arch/arm/boot/dts/exynos4.dtsi                     |    9 ++
 arch/arm/boot/dts/exynos4x12.dtsi                  |    4 +
 arch/arm/boot/dts/exynos5250.dtsi                  |    9 ++
 drivers/gpu/drm/exynos/exynos_drm_rotator.c        |  109 +++++++++++++++-----
 5 files changed, 129 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt

-- 
1.7.9.5

[PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Chanho Park]

The exynos4 platform is only dt-based since 3.10, we should convert driver data
and ids to dt-based parsing methods. The rotator driver has a limit table to get
size limit of input picture. Each SoCs has slightly different limit value
compared with any others.
For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have
8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size
for its X/Y axis. Thus, we should keep different tables for each of them.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109 ++++++++++++++++++++-------
 1 file changed, 81 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
index 427640a..39b09e0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
@@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device *dev, enum drm_exynos_ipp_cmd cmd)
 	return 0;
 }
 
+static struct rot_limit_table rot_limit_tbl_4210 = {
+	.ycbcr420_2p = {
+		.min_w = 32,
+		.min_h = 32,
+		.max_w = SZ_64K,
+		.max_h = SZ_64K,
+		.align = 3,
+	},
+	.rgb888 = {
+		.min_w = 8,
+		.min_h = 8,
+		.max_w = SZ_16K,
+		.max_h = SZ_16K,
+		.align = 2,
+	},
+};
+
+static struct rot_limit_table rot_limit_tbl_4x12 = {
+	.ycbcr420_2p = {
+		.min_w = 32,
+		.min_h = 32,
+		.max_w = SZ_32K,
+		.max_h = SZ_32K,
+		.align = 3,
+	},
+	.rgb888 = {
+		.min_w = 8,
+		.min_h = 8,
+		.max_w = SZ_8K,
+		.max_h = SZ_8K,
+		.align = 2,
+	},
+};
+
+static struct rot_limit_table rot_limit_tbl_5250 = {
+	.ycbcr420_2p = {
+		.min_w = 32,
+		.min_h = 32,
+		.max_w = SZ_32K,
+		.max_h = SZ_32K,
+		.align = 3,
+	},
+	.rgb888 = {
+		.min_w = 8,
+		.min_h = 8,
+		.max_w = SZ_8K,
+		.max_h = SZ_8K,
+		.align = 1,
+	},
+};
+
+static const struct of_device_id exynos_rotator_match[] = {
+	{
+		.compatible = "samsung,exynos4210-rotator",
+		.data = &rot_limit_tbl_4210,
+	},
+	{
+		.compatible = "samsung,exynos4212-rotator",
+		.data = &rot_limit_tbl_4x12,
+	},
+	{
+		.compatible = "samsung,exynos5250-rotator",
+		.data = &rot_limit_tbl_5250,
+	},
+	{},
+};
+
 static int rotator_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	rot->limit_tbl = (struct rot_limit_table *)
-				platform_get_device_id(pdev)->driver_data;
+	if (dev->of_node) {
+		const struct of_device_id *match;
+		match = of_match_node(of_match_ptr(exynos_rotator_match),
+					dev->of_node);
+		if (match == NULL) {
+			dev_err(dev, "failed to match node\n");
+			return -ENODEV;
+		}
+		rot->limit_tbl = (struct rot_limit_table *)match->data;
+	} else {
+		dev_err(dev, "cannot find binding\n");
+		return -ENODEV;
+	}
 
 	rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	rot->regs = devm_ioremap_resource(dev, rot->regs_res);
@@ -718,31 +796,6 @@ static int rotator_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct rot_limit_table rot_limit_tbl = {
-	.ycbcr420_2p = {
-		.min_w = 32,
-		.min_h = 32,
-		.max_w = SZ_32K,
-		.max_h = SZ_32K,
-		.align = 3,
-	},
-	.rgb888 = {
-		.min_w = 8,
-		.min_h = 8,
-		.max_w = SZ_8K,
-		.max_h = SZ_8K,
-		.align = 2,
-	},
-};
-
-static struct platform_device_id rotator_driver_ids[] = {
-	{
-		.name		= "exynos-rot",
-		.driver_data	= (unsigned long)&rot_limit_tbl,
-	},
-	{},
-};
-
 static int rotator_clk_crtl(struct rot_context *rot, bool enable)
 {
 	if (enable) {
@@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = {
 struct platform_driver rotator_driver = {
 	.probe		= rotator_probe,
 	.remove		= rotator_remove,
-	.id_table	= rotator_driver_ids,
 	.driver		= {
 		.name	= "exynos-rot",
 		.owner	= THIS_MODULE,
 		.pm	= &rotator_pm_ops,
+		.of_match_table = of_match_ptr(exynos_rotator_match),
 	},
 };
-- 
1.7.9.5

[PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Chanho Park]

The exynos4 platform is only dt-based since 3.10, we should convert driver data
and ids to dt-based parsing methods. The rotator driver has a limit table to get
size limit of input picture. Each SoCs has slightly different limit value
compared with any others.
For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have
8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size
for its X/Y axis. Thus, we should keep different tables for each of them.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109 ++++++++++++++++++++-------
 1 file changed, 81 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
index 427640a..39b09e0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
@@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device *dev, enum drm_exynos_ipp_cmd cmd)
 	return 0;
 }
 
+static struct rot_limit_table rot_limit_tbl_4210 = {
+	.ycbcr420_2p = {
+		.min_w = 32,
+		.min_h = 32,
+		.max_w = SZ_64K,
+		.max_h = SZ_64K,
+		.align = 3,
+	},
+	.rgb888 = {
+		.min_w = 8,
+		.min_h = 8,
+		.max_w = SZ_16K,
+		.max_h = SZ_16K,
+		.align = 2,
+	},
+};
+
+static struct rot_limit_table rot_limit_tbl_4x12 = {
+	.ycbcr420_2p = {
+		.min_w = 32,
+		.min_h = 32,
+		.max_w = SZ_32K,
+		.max_h = SZ_32K,
+		.align = 3,
+	},
+	.rgb888 = {
+		.min_w = 8,
+		.min_h = 8,
+		.max_w = SZ_8K,
+		.max_h = SZ_8K,
+		.align = 2,
+	},
+};
+
+static struct rot_limit_table rot_limit_tbl_5250 = {
+	.ycbcr420_2p = {
+		.min_w = 32,
+		.min_h = 32,
+		.max_w = SZ_32K,
+		.max_h = SZ_32K,
+		.align = 3,
+	},
+	.rgb888 = {
+		.min_w = 8,
+		.min_h = 8,
+		.max_w = SZ_8K,
+		.max_h = SZ_8K,
+		.align = 1,
+	},
+};
+
+static const struct of_device_id exynos_rotator_match[] = {
+	{
+		.compatible = "samsung,exynos4210-rotator",
+		.data = &rot_limit_tbl_4210,
+	},
+	{
+		.compatible = "samsung,exynos4212-rotator",
+		.data = &rot_limit_tbl_4x12,
+	},
+	{
+		.compatible = "samsung,exynos5250-rotator",
+		.data = &rot_limit_tbl_5250,
+	},
+	{},
+};
+
 static int rotator_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	rot->limit_tbl = (struct rot_limit_table *)
-				platform_get_device_id(pdev)->driver_data;
+	if (dev->of_node) {
+		const struct of_device_id *match;
+		match = of_match_node(of_match_ptr(exynos_rotator_match),
+					dev->of_node);
+		if (match == NULL) {
+			dev_err(dev, "failed to match node\n");
+			return -ENODEV;
+		}
+		rot->limit_tbl = (struct rot_limit_table *)match->data;
+	} else {
+		dev_err(dev, "cannot find binding\n");
+		return -ENODEV;
+	}
 
 	rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	rot->regs = devm_ioremap_resource(dev, rot->regs_res);
@@ -718,31 +796,6 @@ static int rotator_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct rot_limit_table rot_limit_tbl = {
-	.ycbcr420_2p = {
-		.min_w = 32,
-		.min_h = 32,
-		.max_w = SZ_32K,
-		.max_h = SZ_32K,
-		.align = 3,
-	},
-	.rgb888 = {
-		.min_w = 8,
-		.min_h = 8,
-		.max_w = SZ_8K,
-		.max_h = SZ_8K,
-		.align = 2,
-	},
-};
-
-static struct platform_device_id rotator_driver_ids[] = {
-	{
-		.name		= "exynos-rot",
-		.driver_data	= (unsigned long)&rot_limit_tbl,
-	},
-	{},
-};
-
 static int rotator_clk_crtl(struct rot_context *rot, bool enable)
 {
 	if (enable) {
@@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = {
 struct platform_driver rotator_driver = {
 	.probe		= rotator_probe,
 	.remove		= rotator_remove,
-	.id_table	= rotator_driver_ids,
 	.driver		= {
 		.name	= "exynos-rot",
 		.owner	= THIS_MODULE,
 		.pm	= &rotator_pm_ops,
+		.of_match_table = of_match_ptr(exynos_rotator_match),
 	},
 };
-- 
1.7.9.5

[PATCHv2 2/5] ARM: dts: Add rotator node for exynos4210

[Chanho Park]

This patch adds a rotator node for exynos4210. The exynos4210 has different
limitation of image size compared with later chips.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4.dtsi |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index 597cfcf..002b2b8 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -243,6 +243,15 @@
 		status = "disabled";
 	};
 
+	rotator: rotator@12810000 {
+		compatible = "samsung,exynos4210-rotator";
+		reg = <0x12810000 0x1000>;
+		interrupts = <0 83 0>;
+		clocks = <&clock 278>;
+		clock-names = "rotator";
+		status = "disabled";
+	};
+
 	mfc: codec@13400000 {
 		compatible = "samsung,mfc-v5";
 		reg = <0x13400000 0x10000>;
-- 
1.7.9.5

[PATCHv2 2/5] ARM: dts: Add rotator node for exynos4210

[Chanho Park]

This patch adds a rotator node for exynos4210. The exynos4210 has different
limitation of image size compared with later chips.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4.dtsi |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index 597cfcf..002b2b8 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -243,6 +243,15 @@
 		status = "disabled";
 	};
 
+	rotator: rotator at 12810000 {
+		compatible = "samsung,exynos4210-rotator";
+		reg = <0x12810000 0x1000>;
+		interrupts = <0 83 0>;
+		clocks = <&clock 278>;
+		clock-names = "rotator";
+		status = "disabled";
+	};
+
 	mfc: codec at 13400000 {
 		compatible = "samsung,mfc-v5";
 		reg = <0x13400000 0x10000>;
-- 
1.7.9.5

[PATCHv2 3/5] ARM: dts: Add rotator node for exynos4x12

[Chanho Park]

This patch adds a rotator node for exynos4212 and 4412. These have different
limitation of image size compared with the exynos4210. So, we should define
new compatible to distinguish it from the exynos4210.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4x12.dtsi |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 954628c..7bae33f 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -176,4 +176,8 @@
 			};
 		};
 	};
+
+	rotator: rotator@12810000 {
+		compatible = "samsung,exynos4212-rotator";
+	};
 };
-- 
1.7.9.5

[PATCHv2 3/5] ARM: dts: Add rotator node for exynos4x12

[Chanho Park]

This patch adds a rotator node for exynos4212 and 4412. These have different
limitation of image size compared with the exynos4210. So, we should define
new compatible to distinguish it from the exynos4210.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4x12.dtsi |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi
index 954628c..7bae33f 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -176,4 +176,8 @@
 			};
 		};
 	};
+
+	rotator: rotator at 12810000 {
+		compatible = "samsung,exynos4212-rotator";
+	};
 };
-- 
1.7.9.5

[PATCHv2 4/5] ARM: dts: Add rotator node for exynos5250

[Chanho Park]

This patch adds a rotator node for exynos5250. It has different align value of
image size compared with any other chips. So, we should define new compatible
for the exynos5250.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos5250.dtsi |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 6f356ce..7ee2da8 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -570,6 +570,15 @@
 		};
 	};
 
+	rotator: rotator@11C00000 {
+		compatible = "samsung,exynos5250-rotator";
+		reg = <0x11C00000 0x1000>;
+		interrupts = <0 84 0>;
+		clocks = <&clock 269>;
+		clock-names = "rotator";
+		status = "disabled";
+	};
+
 	gsc_0:  gsc@0x13e00000 {
 		compatible = "samsung,exynos5-gsc";
 		reg = <0x13e00000 0x1000>;
-- 
1.7.9.5

[PATCHv2 4/5] ARM: dts: Add rotator node for exynos5250

[Chanho Park]

This patch adds a rotator node for exynos5250. It has different align value of
image size compared with any other chips. So, we should define new compatible
for the exynos5250.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos5250.dtsi |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 6f356ce..7ee2da8 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -570,6 +570,15 @@
 		};
 	};
 
+	rotator: rotator at 11C00000 {
+		compatible = "samsung,exynos5250-rotator";
+		reg = <0x11C00000 0x1000>;
+		interrupts = <0 84 0>;
+		clocks = <&clock 269>;
+		clock-names = "rotator";
+		status = "disabled";
+	};
+
 	gsc_0:  gsc at 0x13e00000 {
 		compatible = "samsung,exynos5-gsc";
 		reg = <0x13e00000 0x1000>;
-- 
1.7.9.5

[PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Chanho Park]

This patch describes each nodes of rotator and specifies a example how to bind
it.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/gpu/samsung-rotator.txt    |   26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt

diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
new file mode 100644
index 0000000..31ee7b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
@@ -0,0 +1,26 @@
+* Samsung Image Rotator
+
+Required properties:
+  - compatible : value should be following:
+	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
+	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
+	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
+
+  - reg : Physical base address of the IP registers and length of memory
+	  mapped region.
+
+  - interrupts : Interrupt number of Rotator
+
+  - clocks : Clock number described in Documentations/devicetree/binding/clock.
+
+  - clock-names : Clock name.
+
+Example:
+	rotator: rotator@12800000 {
+		compatible = "samsung,exynos4210-rotator";
+		reg = <0x12810000 0x1000>;
+		interrupts = <0 83 0>;
+		clocks = <&clock 278>;
+		clock-names = "rotator";
+		status = "disabled";
+	};
-- 
1.7.9.5

[PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Chanho Park]

This patch describes each nodes of rotator and specifies a example how to bind
it.

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/gpu/samsung-rotator.txt    |   26 ++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt

diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
new file mode 100644
index 0000000..31ee7b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
@@ -0,0 +1,26 @@
+* Samsung Image Rotator
+
+Required properties:
+  - compatible : value should be following:
+	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
+	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
+	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
+
+  - reg : Physical base address of the IP registers and length of memory
+	  mapped region.
+
+  - interrupts : Interrupt number of Rotator
+
+  - clocks : Clock number described in Documentations/devicetree/binding/clock.
+
+  - clock-names : Clock name.
+
+Example:
+	rotator: rotator at 12800000 {
+		compatible = "samsung,exynos4210-rotator";
+		reg = <0x12810000 0x1000>;
+		interrupts = <0 83 0>;
+		clocks = <&clock 278>;
+		clock-names = "rotator";
+		status = "disabled";
+	};
-- 
1.7.9.5

Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Sachin Kamat]

Hi Chanho,

On 9 August 2013 13:10, Chanho Park <chanho61.park@samsung.com> wrote:
> This patch describes each nodes of rotator and specifies a example how to bind
> it.
>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Cc: Thomas Abraham <thomas.abraham@linaro.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/gpu/samsung-rotator.txt    |   26 ++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
>
> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> new file mode 100644
> index 0000000..31ee7b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> @@ -0,0 +1,26 @@
> +* Samsung Image Rotator
> +
> +Required properties:
> +  - compatible : value should be following:

This should be "should be one of the following:"

> +       (a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
> +       (b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
> +       (c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
> +
> +  - reg : Physical base address of the IP registers and length of memory
> +         mapped region.
> +
> +  - interrupts : Interrupt number of Rotator
> +
> +  - clocks : Clock number described in Documentations/devicetree/binding/clock.

The path has several typos. Instead you can simply mention
clocks : from common clock binding: handle to rotator clock.

-- 
With warm regards,
Sachin

[PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Sachin Kamat]

Hi Chanho,

On 9 August 2013 13:10, Chanho Park <chanho61.park@samsung.com> wrote:
> This patch describes each nodes of rotator and specifies a example how to bind
> it.
>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Cc: Thomas Abraham <thomas.abraham@linaro.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/gpu/samsung-rotator.txt    |   26 ++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
>
> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> new file mode 100644
> index 0000000..31ee7b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> @@ -0,0 +1,26 @@
> +* Samsung Image Rotator
> +
> +Required properties:
> +  - compatible : value should be following:

This should be "should be one of the following:"

> +       (a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
> +       (b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
> +       (c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
> +
> +  - reg : Physical base address of the IP registers and length of memory
> +         mapped region.
> +
> +  - interrupts : Interrupt number of Rotator
> +
> +  - clocks : Clock number described in Documentations/devicetree/binding/clock.

The path has several typos. Instead you can simply mention
clocks : from common clock binding: handle to rotator clock.

-- 
With warm regards,
Sachin

Re: [PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Sachin Kamat]

Hi Chanho,

On 9 August 2013 13:10, Chanho Park <chanho61.park@samsung.com> wrote:
> The exynos4 platform is only dt-based since 3.10, we should convert driver data
> and ids to dt-based parsing methods. The rotator driver has a limit table to get
> size limit of input picture. Each SoCs has slightly different limit value
> compared with any others.
> For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have
> 8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size
> for its X/Y axis. Thus, we should keep different tables for each of them.
>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109 ++++++++++++++++++++-------
>  1 file changed, 81 insertions(+), 28 deletions(-)
[snip]
> +
> +static const struct of_device_id exynos_rotator_match[] = {
> +       {
> +               .compatible = "samsung,exynos4210-rotator",
> +               .data = &rot_limit_tbl_4210,
> +       },
> +       {
> +               .compatible = "samsung,exynos4212-rotator",
> +               .data = &rot_limit_tbl_4x12,
> +       },
> +       {
> +               .compatible = "samsung,exynos5250-rotator",
> +               .data = &rot_limit_tbl_5250,
> +       },
> +       {},
> +};
> +
>  static int rotator_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>         }
>
> -       rot->limit_tbl = (struct rot_limit_table *)
> -                               platform_get_device_id(pdev)->driver_data;
> +       if (dev->of_node) {
> +               const struct of_device_id *match;
> +               match = of_match_node(of_match_ptr(exynos_rotator_match),

of_match_ptr is not needed since exynos_rotator_match is always compiled in.

[snip]
> -static struct platform_device_id rotator_driver_ids[] = {
> -       {
> -               .name           = "exynos-rot",
> -               .driver_data    = (unsigned long)&rot_limit_tbl,
> -       },
> -       {},
> -};
> -
>  static int rotator_clk_crtl(struct rot_context *rot, bool enable)
>  {
>         if (enable) {
> @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = {
>  struct platform_driver rotator_driver = {
>         .probe          = rotator_probe,
>         .remove         = rotator_remove,
> -       .id_table       = rotator_driver_ids,
>         .driver         = {
>                 .name   = "exynos-rot",
>                 .owner  = THIS_MODULE,
>                 .pm     = &rotator_pm_ops,
> +               .of_match_table = of_match_ptr(exynos_rotator_match),

ditto

-- 
With warm regards,
Sachin

[PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Sachin Kamat]

Hi Chanho,

On 9 August 2013 13:10, Chanho Park <chanho61.park@samsung.com> wrote:
> The exynos4 platform is only dt-based since 3.10, we should convert driver data
> and ids to dt-based parsing methods. The rotator driver has a limit table to get
> size limit of input picture. Each SoCs has slightly different limit value
> compared with any others.
> For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have
> 8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size
> for its X/Y axis. Thus, we should keep different tables for each of them.
>
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109 ++++++++++++++++++++-------
>  1 file changed, 81 insertions(+), 28 deletions(-)
[snip]
> +
> +static const struct of_device_id exynos_rotator_match[] = {
> +       {
> +               .compatible = "samsung,exynos4210-rotator",
> +               .data = &rot_limit_tbl_4210,
> +       },
> +       {
> +               .compatible = "samsung,exynos4212-rotator",
> +               .data = &rot_limit_tbl_4x12,
> +       },
> +       {
> +               .compatible = "samsung,exynos5250-rotator",
> +               .data = &rot_limit_tbl_5250,
> +       },
> +       {},
> +};
> +
>  static int rotator_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev)
>                 return -ENOMEM;
>         }
>
> -       rot->limit_tbl = (struct rot_limit_table *)
> -                               platform_get_device_id(pdev)->driver_data;
> +       if (dev->of_node) {
> +               const struct of_device_id *match;
> +               match = of_match_node(of_match_ptr(exynos_rotator_match),

of_match_ptr is not needed since exynos_rotator_match is always compiled in.

[snip]
> -static struct platform_device_id rotator_driver_ids[] = {
> -       {
> -               .name           = "exynos-rot",
> -               .driver_data    = (unsigned long)&rot_limit_tbl,
> -       },
> -       {},
> -};
> -
>  static int rotator_clk_crtl(struct rot_context *rot, bool enable)
>  {
>         if (enable) {
> @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = {
>  struct platform_driver rotator_driver = {
>         .probe          = rotator_probe,
>         .remove         = rotator_remove,
> -       .id_table       = rotator_driver_ids,
>         .driver         = {
>                 .name   = "exynos-rot",
>                 .owner  = THIS_MODULE,
>                 .pm     = &rotator_pm_ops,
> +               .of_match_table = of_match_ptr(exynos_rotator_match),

ditto

-- 
With warm regards,
Sachin

RE: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Chanho Park]

Hi Sachin,

> -----Original Message-----
> From: Sachin Kamat [mailto:sachin.kamat@linaro.org]
> Sent: Friday, August 09, 2013 6:40 PM
> To: Chanho Park
> Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri-
> devel@lists.freedesktop.org; kyungmin.park@samsung.com;
> mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com;
> tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; Thomas Abraham
> Subject: Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for
> exynos rotator
> 
> Hi Chanho,
> 
> On 9 August 2013 13:10, Chanho Park <chanho61.park@samsung.com> wrote:
> > This patch describes each nodes of rotator and specifies a example how
> > to bind it.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > Cc: Thomas Abraham <thomas.abraham@linaro.org>
> > Cc: Kukjin Kim <kgene.kim@samsung.com>
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  .../devicetree/bindings/gpu/samsung-rotator.txt    |   26
> ++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> > b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> > new file mode 100644
> > index 0000000..31ee7b5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> > @@ -0,0 +1,26 @@
> > +* Samsung Image Rotator
> > +
> > +Required properties:
> > +  - compatible : value should be following:
> 
> This should be "should be one of the following:"

I left out while I changed it to select multiple choices.
Thanks.

> 
> > +       (a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
> > +       (b) "samsung,exynos4212-rotator" for Rotator IP in
> Exynos4212/4412
> > +       (c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
> > +
> > +  - reg : Physical base address of the IP registers and length of
> memory
> > +         mapped region.
> > +
> > +  - interrupts : Interrupt number of Rotator
> > +
> > +  - clocks : Clock number described in
> Documentations/devicetree/binding/clock.
> 
> The path has several typos. Instead you can simply mention clocks : from
> common clock binding: handle to rotator clock.

Oh, it has many typos. I'll change it.
Thanks for your review.

Best Regards,
Chanho Park

RE: [PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Chanho Park]

Hi Sachin,

> -----Original Message-----
> From: Sachin Kamat [mailto:sachin.kamat@linaro.org]
> Sent: Friday, August 09, 2013 6:49 PM
> To: Chanho Park
> Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri-
> devel@lists.freedesktop.org; kyungmin.park@samsung.com;
> mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com;
> tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org
> Subject: Re: [PATCHv2 1/5] drm/exynos: add device tree support for
> rotator
> 
> Hi Chanho,
> 
> On 9 August 2013 13:10, Chanho Park <chanho61.park@samsung.com> wrote:
> > The exynos4 platform is only dt-based since 3.10, we should convert
> > driver data and ids to dt-based parsing methods. The rotator driver
> > has a limit table to get size limit of input picture. Each SoCs has
> > slightly different limit value compared with any others.
> > For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others
> > have 8k x 8k. Another example the exynos5250 should have multiple of 2
> > pixel size for its X/Y axis. Thus, we should keep different tables for
> each of them.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109
> > ++++++++++++++++++++-------
> >  1 file changed, 81 insertions(+), 28 deletions(-)
> [snip]
> > +
> > +static const struct of_device_id exynos_rotator_match[] = {
> > +       {
> > +               .compatible = "samsung,exynos4210-rotator",
> > +               .data = &rot_limit_tbl_4210,
> > +       },
> > +       {
> > +               .compatible = "samsung,exynos4212-rotator",
> > +               .data = &rot_limit_tbl_4x12,
> > +       },
> > +       {
> > +               .compatible = "samsung,exynos5250-rotator",
> > +               .data = &rot_limit_tbl_5250,
> > +       },
> > +       {},
> > +};
> > +
> >  static int rotator_probe(struct platform_device *pdev)  {
> >         struct device *dev = &pdev->dev; @@ -645,8 +712,19 @@ static
> > int rotator_probe(struct platform_device *pdev)
> >                 return -ENOMEM;
> >         }
> >
> > -       rot->limit_tbl = (struct rot_limit_table *)
> > -
platform_get_device_id(pdev)->driver_data;
> > +       if (dev->of_node) {
> > +               const struct of_device_id *match;
> > +               match =
> > + of_match_node(of_match_ptr(exynos_rotator_match),
> 
> of_match_ptr is not needed since exynos_rotator_match is always compiled
> in.

OK. I'll remove it.
Thanks.

> 
> [snip]
> > -static struct platform_device_id rotator_driver_ids[] = {
> > -       {
> > -               .name           = "exynos-rot",
> > -               .driver_data    = (unsigned long)&rot_limit_tbl,
> > -       },
> > -       {},
> > -};
> > -
> >  static int rotator_clk_crtl(struct rot_context *rot, bool enable)  {
> >         if (enable) {
> > @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops =
> > {  struct platform_driver rotator_driver = {
> >         .probe          = rotator_probe,
> >         .remove         = rotator_remove,
> > -       .id_table       = rotator_driver_ids,
> >         .driver         = {
> >                 .name   = "exynos-rot",
> >                 .owner  = THIS_MODULE,
> >                 .pm     = &rotator_pm_ops,
> > +               .of_match_table = of_match_ptr(exynos_rotator_match),
> 
> ditto
> 
> --
> With warm regards,
> Sachin

Re: [PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Tomasz Figa]

Hi Chanho,

On Friday 09 of August 2013 16:40:49 Chanho Park wrote:
> The exynos4 platform is only dt-based since 3.10, we should convert
> driver data and ids to dt-based parsing methods. The rotator driver has
> a limit table to get size limit of input picture. Each SoCs has slightly
> different limit value compared with any others.
> For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others
> have 8k x 8k. Another example the exynos5250 should have multiple of 2
> pixel size for its X/Y axis. Thus, we should keep different tables for
> each of them.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109
> ++++++++++++++++++++------- 1 file changed, 81 insertions(+), 28
> deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 427640a..39b09e0
> 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> @@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device *dev,
> enum drm_exynos_ipp_cmd cmd) return 0;
>  }
> 
> +static struct rot_limit_table rot_limit_tbl_4210 = {
> +	.ycbcr420_2p = {
> +		.min_w = 32,
> +		.min_h = 32,
> +		.max_w = SZ_64K,
> +		.max_h = SZ_64K,
> +		.align = 3,
> +	},
> +	.rgb888 = {
> +		.min_w = 8,
> +		.min_h = 8,
> +		.max_w = SZ_16K,
> +		.max_h = SZ_16K,
> +		.align = 2,
> +	},
> +};
> +
> +static struct rot_limit_table rot_limit_tbl_4x12 = {
> +	.ycbcr420_2p = {
> +		.min_w = 32,
> +		.min_h = 32,
> +		.max_w = SZ_32K,
> +		.max_h = SZ_32K,
> +		.align = 3,
> +	},
> +	.rgb888 = {
> +		.min_w = 8,
> +		.min_h = 8,
> +		.max_w = SZ_8K,
> +		.max_h = SZ_8K,
> +		.align = 2,
> +	},
> +};
> +
> +static struct rot_limit_table rot_limit_tbl_5250 = {
> +	.ycbcr420_2p = {
> +		.min_w = 32,
> +		.min_h = 32,
> +		.max_w = SZ_32K,
> +		.max_h = SZ_32K,
> +		.align = 3,
> +	},
> +	.rgb888 = {
> +		.min_w = 8,
> +		.min_h = 8,
> +		.max_w = SZ_8K,
> +		.max_h = SZ_8K,
> +		.align = 1,
> +	},
> +};
> +
> +static const struct of_device_id exynos_rotator_match[] = {
> +	{
> +		.compatible = "samsung,exynos4210-rotator",
> +		.data = &rot_limit_tbl_4210,
> +	},
> +	{
> +		.compatible = "samsung,exynos4212-rotator",
> +		.data = &rot_limit_tbl_4x12,
> +	},
> +	{
> +		.compatible = "samsung,exynos5250-rotator",
> +		.data = &rot_limit_tbl_5250,
> +	},
> +	{},
> +};
> +
>  static int rotator_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device
> *pdev) return -ENOMEM;
>  	}
> 
> -	rot->limit_tbl = (struct rot_limit_table *)
> -				platform_get_device_id(pdev)->driver_data;
> +	if (dev->of_node) {
> +		const struct of_device_id *match;
> +		match = of_match_node(of_match_ptr(exynos_rotator_match),
> +					dev->of_node);
> +		if (match == NULL) {
> +			dev_err(dev, "failed to match node\n");
> +			return -ENODEV;
> +		}
> +		rot->limit_tbl = (struct rot_limit_table *)match->data;
> +	} else {
> +		dev_err(dev, "cannot find binding\n");

What about having a check for !dev->of_node at the beginning of probe, to 
not complicate further code?

Also the error message is confusing. It should be something closer to 
"device does not have of_node".

> +		return -ENODEV;
> +	}
> 
>  	rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	rot->regs = devm_ioremap_resource(dev, rot->regs_res);
> @@ -718,31 +796,6 @@ static int rotator_remove(struct platform_device
> *pdev) return 0;
>  }
> 
> -static struct rot_limit_table rot_limit_tbl = {
> -	.ycbcr420_2p = {
> -		.min_w = 32,
> -		.min_h = 32,
> -		.max_w = SZ_32K,
> -		.max_h = SZ_32K,
> -		.align = 3,
> -	},
> -	.rgb888 = {
> -		.min_w = 8,
> -		.min_h = 8,
> -		.max_w = SZ_8K,
> -		.max_h = SZ_8K,
> -		.align = 2,
> -	},
> -};
> -
> -static struct platform_device_id rotator_driver_ids[] = {
> -	{
> -		.name		= "exynos-rot",
> -		.driver_data	= (unsigned long)&rot_limit_tbl,
> -	},
> -	{},
> -};
> -
>  static int rotator_clk_crtl(struct rot_context *rot, bool enable)
>  {
>  	if (enable) {
> @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = {
>  struct platform_driver rotator_driver = {
>  	.probe		= rotator_probe,
>  	.remove		= rotator_remove,
> -	.id_table	= rotator_driver_ids,
>  	.driver		= {
>  		.name	= "exynos-rot",
>  		.owner	= THIS_MODULE,
>  		.pm	= &rotator_pm_ops,
> +		.of_match_table = of_match_ptr(exynos_rotator_match),
>  	},
>  };

Otherwise looks fine.

One more thing is that IMHO patch 5/5 could be squashed with this one, so 
documentation for the binding would be available at the same it is 
introduced.

Best regards,
Tomasz

[PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Tomasz Figa]

Hi Chanho,

On Friday 09 of August 2013 16:40:49 Chanho Park wrote:
> The exynos4 platform is only dt-based since 3.10, we should convert
> driver data and ids to dt-based parsing methods. The rotator driver has
> a limit table to get size limit of input picture. Each SoCs has slightly
> different limit value compared with any others.
> For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others
> have 8k x 8k. Another example the exynos5250 should have multiple of 2
> pixel size for its X/Y axis. Thus, we should keep different tables for
> each of them.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109
> ++++++++++++++++++++------- 1 file changed, 81 insertions(+), 28
> deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 427640a..39b09e0
> 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> @@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device *dev,
> enum drm_exynos_ipp_cmd cmd) return 0;
>  }
> 
> +static struct rot_limit_table rot_limit_tbl_4210 = {
> +	.ycbcr420_2p = {
> +		.min_w = 32,
> +		.min_h = 32,
> +		.max_w = SZ_64K,
> +		.max_h = SZ_64K,
> +		.align = 3,
> +	},
> +	.rgb888 = {
> +		.min_w = 8,
> +		.min_h = 8,
> +		.max_w = SZ_16K,
> +		.max_h = SZ_16K,
> +		.align = 2,
> +	},
> +};
> +
> +static struct rot_limit_table rot_limit_tbl_4x12 = {
> +	.ycbcr420_2p = {
> +		.min_w = 32,
> +		.min_h = 32,
> +		.max_w = SZ_32K,
> +		.max_h = SZ_32K,
> +		.align = 3,
> +	},
> +	.rgb888 = {
> +		.min_w = 8,
> +		.min_h = 8,
> +		.max_w = SZ_8K,
> +		.max_h = SZ_8K,
> +		.align = 2,
> +	},
> +};
> +
> +static struct rot_limit_table rot_limit_tbl_5250 = {
> +	.ycbcr420_2p = {
> +		.min_w = 32,
> +		.min_h = 32,
> +		.max_w = SZ_32K,
> +		.max_h = SZ_32K,
> +		.align = 3,
> +	},
> +	.rgb888 = {
> +		.min_w = 8,
> +		.min_h = 8,
> +		.max_w = SZ_8K,
> +		.max_h = SZ_8K,
> +		.align = 1,
> +	},
> +};
> +
> +static const struct of_device_id exynos_rotator_match[] = {
> +	{
> +		.compatible = "samsung,exynos4210-rotator",
> +		.data = &rot_limit_tbl_4210,
> +	},
> +	{
> +		.compatible = "samsung,exynos4212-rotator",
> +		.data = &rot_limit_tbl_4x12,
> +	},
> +	{
> +		.compatible = "samsung,exynos5250-rotator",
> +		.data = &rot_limit_tbl_5250,
> +	},
> +	{},
> +};
> +
>  static int rotator_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device
> *pdev) return -ENOMEM;
>  	}
> 
> -	rot->limit_tbl = (struct rot_limit_table *)
> -				platform_get_device_id(pdev)->driver_data;
> +	if (dev->of_node) {
> +		const struct of_device_id *match;
> +		match = of_match_node(of_match_ptr(exynos_rotator_match),
> +					dev->of_node);
> +		if (match == NULL) {
> +			dev_err(dev, "failed to match node\n");
> +			return -ENODEV;
> +		}
> +		rot->limit_tbl = (struct rot_limit_table *)match->data;
> +	} else {
> +		dev_err(dev, "cannot find binding\n");

What about having a check for !dev->of_node at the beginning of probe, to 
not complicate further code?

Also the error message is confusing. It should be something closer to 
"device does not have of_node".

> +		return -ENODEV;
> +	}
> 
>  	rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	rot->regs = devm_ioremap_resource(dev, rot->regs_res);
> @@ -718,31 +796,6 @@ static int rotator_remove(struct platform_device
> *pdev) return 0;
>  }
> 
> -static struct rot_limit_table rot_limit_tbl = {
> -	.ycbcr420_2p = {
> -		.min_w = 32,
> -		.min_h = 32,
> -		.max_w = SZ_32K,
> -		.max_h = SZ_32K,
> -		.align = 3,
> -	},
> -	.rgb888 = {
> -		.min_w = 8,
> -		.min_h = 8,
> -		.max_w = SZ_8K,
> -		.max_h = SZ_8K,
> -		.align = 2,
> -	},
> -};
> -
> -static struct platform_device_id rotator_driver_ids[] = {
> -	{
> -		.name		= "exynos-rot",
> -		.driver_data	= (unsigned long)&rot_limit_tbl,
> -	},
> -	{},
> -};
> -
>  static int rotator_clk_crtl(struct rot_context *rot, bool enable)
>  {
>  	if (enable) {
> @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = {
>  struct platform_driver rotator_driver = {
>  	.probe		= rotator_probe,
>  	.remove		= rotator_remove,
> -	.id_table	= rotator_driver_ids,
>  	.driver		= {
>  		.name	= "exynos-rot",
>  		.owner	= THIS_MODULE,
>  		.pm	= &rotator_pm_ops,
> +		.of_match_table = of_match_ptr(exynos_rotator_match),
>  	},
>  };

Otherwise looks fine.

One more thing is that IMHO patch 5/5 could be squashed with this one, so 
documentation for the binding would be available at the same it is 
introduced.

Best regards,
Tomasz

Re: [PATCHv2 2/5] ARM: dts: Add rotator node for exynos4210

[Tomasz Figa]

On Friday 09 of August 2013 16:40:50 Chanho Park wrote:
> This patch adds a rotator node for exynos4210. The exynos4210 has
> different limitation of image size compared with later chips.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Cc: Thomas Abraham <thomas.abraham@linaro.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4.dtsi |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4.dtsi
> b/arch/arm/boot/dts/exynos4.dtsi index 597cfcf..002b2b8 100644
> --- a/arch/arm/boot/dts/exynos4.dtsi
> +++ b/arch/arm/boot/dts/exynos4.dtsi
> @@ -243,6 +243,15 @@
>  		status = "disabled";
>  	};
> 
> +	rotator: rotator@12810000 {
> +		compatible = "samsung,exynos4210-rotator";
> +		reg = <0x12810000 0x1000>;
> +		interrupts = <0 83 0>;
> +		clocks = <&clock 278>;
> +		clock-names = "rotator";
> +		status = "disabled";

Does the rotator need any board-specific information to work? If no, it 
should not be disabled by default. Same comment goes to patch 4/5.

Best regards,
Tomasz

[PATCHv2 2/5] ARM: dts: Add rotator node for exynos4210

[Tomasz Figa]

On Friday 09 of August 2013 16:40:50 Chanho Park wrote:
> This patch adds a rotator node for exynos4210. The exynos4210 has
> different limitation of image size compared with later chips.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Cc: Thomas Abraham <thomas.abraham@linaro.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/boot/dts/exynos4.dtsi |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4.dtsi
> b/arch/arm/boot/dts/exynos4.dtsi index 597cfcf..002b2b8 100644
> --- a/arch/arm/boot/dts/exynos4.dtsi
> +++ b/arch/arm/boot/dts/exynos4.dtsi
> @@ -243,6 +243,15 @@
>  		status = "disabled";
>  	};
> 
> +	rotator: rotator at 12810000 {
> +		compatible = "samsung,exynos4210-rotator";
> +		reg = <0x12810000 0x1000>;
> +		interrupts = <0 83 0>;
> +		clocks = <&clock 278>;
> +		clock-names = "rotator";
> +		status = "disabled";

Does the rotator need any board-specific information to work? If no, it 
should not be disabled by default. Same comment goes to patch 4/5.

Best regards,
Tomasz

Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Tomasz Figa]

Hi Chanho,

On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
> This patch describes each nodes of rotator and specifies a example how to
> bind it.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Cc: Thomas Abraham <thomas.abraham@linaro.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/gpu/samsung-rotator.txt    |   26
> ++++++++++++++++++++ 1 file changed, 26 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file
> mode 100644
> index 0000000..31ee7b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> @@ -0,0 +1,26 @@
> +* Samsung Image Rotator
> +
> +Required properties:
> +  - compatible : value should be following:
> +	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
> +	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
> +	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
> +
> +  - reg : Physical base address of the IP registers and length of memory
> +	  mapped region.
> +
> +  - interrupts : Interrupt number of Rotator

What about: interrupt specifier for rotator interrupt, according to format 
specific to interrupt parent.

> +
> +  - clocks : Clock number described in
> Documentations/devicetree/binding/clock. +

Perhaps: clock specifier for rotator clock, according to generic clock 
bindings.

> +  - clock-names : Clock name.

Names of clocks specified in "clocks" property. For exynos rotator this 
property should contain only one name: "rotator".

> +Example:
> +	rotator: rotator@12800000 {

I wonder if we shouldn't use a more generic name here, according to ePAPR 
node naming recommendation.

> +		compatible = "samsung,exynos4210-rotator";
> +		reg = <0x12810000 0x1000>;

Typo. Node has 12800000 in unit-address suffix, but this property has 
12810000 instead.

> +		interrupts = <0 83 0>;
> +		clocks = <&clock 278>;
> +		clock-names = "rotator";
> +		status = "disabled";

Status property should be omitted from this example, as it's not a part of 
this binding.

Best regards,
Tomasz

[PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Tomasz Figa]

Hi Chanho,

On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
> This patch describes each nodes of rotator and specifies a example how to
> bind it.
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> Cc: Thomas Abraham <thomas.abraham@linaro.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/gpu/samsung-rotator.txt    |   26
> ++++++++++++++++++++ 1 file changed, 26 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file
> mode 100644
> index 0000000..31ee7b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> @@ -0,0 +1,26 @@
> +* Samsung Image Rotator
> +
> +Required properties:
> +  - compatible : value should be following:
> +	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
> +	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
> +	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
> +
> +  - reg : Physical base address of the IP registers and length of memory
> +	  mapped region.
> +
> +  - interrupts : Interrupt number of Rotator

What about: interrupt specifier for rotator interrupt, according to format 
specific to interrupt parent.

> +
> +  - clocks : Clock number described in
> Documentations/devicetree/binding/clock. +

Perhaps: clock specifier for rotator clock, according to generic clock 
bindings.

> +  - clock-names : Clock name.

Names of clocks specified in "clocks" property. For exynos rotator this 
property should contain only one name: "rotator".

> +Example:
> +	rotator: rotator at 12800000 {

I wonder if we shouldn't use a more generic name here, according to ePAPR 
node naming recommendation.

> +		compatible = "samsung,exynos4210-rotator";
> +		reg = <0x12810000 0x1000>;

Typo. Node has 12800000 in unit-address suffix, but this property has 
12810000 instead.

> +		interrupts = <0 83 0>;
> +		clocks = <&clock 278>;
> +		clock-names = "rotator";
> +		status = "disabled";

Status property should be omitted from this example, as it's not a part of 
this binding.

Best regards,
Tomasz

Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Stephen Warren]

On 08/09/2013 07:15 AM, Tomasz Figa wrote:
> Hi Chanho,
> 
> On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
>> This patch describes each nodes of rotator and specifies a example how to
>> bind it.

>> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt

>> +* Samsung Image Rotator
>> +
>> +Required properties:
>> +  - compatible : value should be following:
>> +	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
>> +	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
>> +	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250

Is "rotator" the name that the HW documentation uses to refer to this
block? If so, those compatible values seem fine. If not, they seem
slightly too generic for me; perhaps better to use the raw HW block name?

[PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Stephen Warren]

On 08/09/2013 07:15 AM, Tomasz Figa wrote:
> Hi Chanho,
> 
> On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
>> This patch describes each nodes of rotator and specifies a example how to
>> bind it.

>> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt

>> +* Samsung Image Rotator
>> +
>> +Required properties:
>> +  - compatible : value should be following:
>> +	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
>> +	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
>> +	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250

Is "rotator" the name that the HW documentation uses to refer to this
block? If so, those compatible values seem fine. If not, they seem
slightly too generic for me; perhaps better to use the raw HW block name?

Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Tomasz Figa]

On Friday 09 of August 2013 10:38:30 Stephen Warren wrote:
> On 08/09/2013 07:15 AM, Tomasz Figa wrote:
> > Hi Chanho,
> > 
> > On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
> >> This patch describes each nodes of rotator and specifies a example how
> >> to bind it.
> >> 
> >> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> >> 
> >> +* Samsung Image Rotator
> >> +
> >> +Required properties:
> >> +  - compatible : value should be following:
> >> +	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
> >> +	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
> >> +	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
> 
> Is "rotator" the name that the HW documentation uses to refer to this
> block?

Yes, it is. More specifically it is referred to as "Image Rotator".

Best regards,
Tomasz

> If so, those compatible values seem fine. If not, they seem
> slightly too generic for me; perhaps better to use the raw HW block name?

[PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Tomasz Figa]

On Friday 09 of August 2013 10:38:30 Stephen Warren wrote:
> On 08/09/2013 07:15 AM, Tomasz Figa wrote:
> > Hi Chanho,
> > 
> > On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
> >> This patch describes each nodes of rotator and specifies a example how
> >> to bind it.
> >> 
> >> diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> >> 
> >> +* Samsung Image Rotator
> >> +
> >> +Required properties:
> >> +  - compatible : value should be following:
> >> +	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
> >> +	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
> >> +	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
> 
> Is "rotator" the name that the HW documentation uses to refer to this
> block?

Yes, it is. More specifically it is referred to as "Image Rotator".

Best regards,
Tomasz

> If so, those compatible values seem fine. If not, they seem
> slightly too generic for me; perhaps better to use the raw HW block name?

RE: [PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Chanho Park]

Hi Tomasz,

> -----Original Message-----
> From: Tomasz Figa [mailto:t.figa@samsung.com]
> Sent: Friday, August 09, 2013 9:51 PM
> To: Chanho Park
> Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri-
> devel@lists.freedesktop.org; kyungmin.park@samsung.com;
> mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com;
> tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org
> Subject: Re: [PATCHv2 1/5] drm/exynos: add device tree support for
> rotator
> 
> Hi Chanho,
> 
> On Friday 09 of August 2013 16:40:49 Chanho Park wrote:
> > The exynos4 platform is only dt-based since 3.10, we should convert
> > driver data and ids to dt-based parsing methods. The rotator driver
> > has a limit table to get size limit of input picture. Each SoCs has
> > slightly different limit value compared with any others.
> > For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others
> > have 8k x 8k. Another example the exynos5250 should have multiple of 2
> > pixel size for its X/Y axis. Thus, we should keep different tables for
> > each of them.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109
> > ++++++++++++++++++++------- 1 file changed, 81 insertions(+), 28
> > deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> > b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 427640a..39b09e0
> > 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
> > @@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device
> > *dev, enum drm_exynos_ipp_cmd cmd) return 0;  }
> >
> > +static struct rot_limit_table rot_limit_tbl_4210 = {
> > +	.ycbcr420_2p = {
> > +		.min_w = 32,
> > +		.min_h = 32,
> > +		.max_w = SZ_64K,
> > +		.max_h = SZ_64K,
> > +		.align = 3,
> > +	},
> > +	.rgb888 = {
> > +		.min_w = 8,
> > +		.min_h = 8,
> > +		.max_w = SZ_16K,
> > +		.max_h = SZ_16K,
> > +		.align = 2,
> > +	},
> > +};
> > +
> > +static struct rot_limit_table rot_limit_tbl_4x12 = {
> > +	.ycbcr420_2p = {
> > +		.min_w = 32,
> > +		.min_h = 32,
> > +		.max_w = SZ_32K,
> > +		.max_h = SZ_32K,
> > +		.align = 3,
> > +	},
> > +	.rgb888 = {
> > +		.min_w = 8,
> > +		.min_h = 8,
> > +		.max_w = SZ_8K,
> > +		.max_h = SZ_8K,
> > +		.align = 2,
> > +	},
> > +};
> > +
> > +static struct rot_limit_table rot_limit_tbl_5250 = {
> > +	.ycbcr420_2p = {
> > +		.min_w = 32,
> > +		.min_h = 32,
> > +		.max_w = SZ_32K,
> > +		.max_h = SZ_32K,
> > +		.align = 3,
> > +	},
> > +	.rgb888 = {
> > +		.min_w = 8,
> > +		.min_h = 8,
> > +		.max_w = SZ_8K,
> > +		.max_h = SZ_8K,
> > +		.align = 1,
> > +	},
> > +};
> > +
> > +static const struct of_device_id exynos_rotator_match[] = {
> > +	{
> > +		.compatible = "samsung,exynos4210-rotator",
> > +		.data = &rot_limit_tbl_4210,
> > +	},
> > +	{
> > +		.compatible = "samsung,exynos4212-rotator",
> > +		.data = &rot_limit_tbl_4x12,
> > +	},
> > +	{
> > +		.compatible = "samsung,exynos5250-rotator",
> > +		.data = &rot_limit_tbl_5250,
> > +	},
> > +	{},
> > +};
> > +
> >  static int rotator_probe(struct platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> > @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device
> > *pdev) return -ENOMEM;
> >  	}
> >
> > -	rot->limit_tbl = (struct rot_limit_table *)
> > -				platform_get_device_id(pdev)->driver_data;
> > +	if (dev->of_node) {
> > +		const struct of_device_id *match;
> > +		match = of_match_node(of_match_ptr(exynos_rotator_match),
> > +					dev->of_node);
> > +		if (match == NULL) {
> > +			dev_err(dev, "failed to match node\n");
> > +			return -ENODEV;
> > +		}
> > +		rot->limit_tbl = (struct rot_limit_table *)match->data;
> > +	} else {
> > +		dev_err(dev, "cannot find binding\n");
> 
> What about having a check for !dev->of_node at the beginning of probe,
> to not complicate further code?

I agree with your comment. I'll move it at the beginning.

> 
> Also the error message is confusing. It should be something closer to
> "device does not have of_node".

I'll change it to avoid confusing. Thanks.

> 
> > +		return -ENODEV;
> > +	}
> >
> >  	rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >  	rot->regs = devm_ioremap_resource(dev, rot->regs_res); @@ -718,31
> > +796,6 @@ static int rotator_remove(struct platform_device
> > *pdev) return 0;
> >  }
> >
> > -static struct rot_limit_table rot_limit_tbl = {
> > -	.ycbcr420_2p = {
> > -		.min_w = 32,
> > -		.min_h = 32,
> > -		.max_w = SZ_32K,
> > -		.max_h = SZ_32K,
> > -		.align = 3,
> > -	},
> > -	.rgb888 = {
> > -		.min_w = 8,
> > -		.min_h = 8,
> > -		.max_w = SZ_8K,
> > -		.max_h = SZ_8K,
> > -		.align = 2,
> > -	},
> > -};
> > -
> > -static struct platform_device_id rotator_driver_ids[] = {
> > -	{
> > -		.name		= "exynos-rot",
> > -		.driver_data	= (unsigned long)&rot_limit_tbl,
> > -	},
> > -	{},
> > -};
> > -
> >  static int rotator_clk_crtl(struct rot_context *rot, bool enable)  {
> >  	if (enable) {
> > @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops =
> > {  struct platform_driver rotator_driver = {
> >  	.probe		= rotator_probe,
> >  	.remove		= rotator_remove,
> > -	.id_table	= rotator_driver_ids,
> >  	.driver		= {
> >  		.name	= "exynos-rot",
> >  		.owner	= THIS_MODULE,
> >  		.pm	= &rotator_pm_ops,
> > +		.of_match_table = of_match_ptr(exynos_rotator_match),
> >  	},
> >  };
> 
> Otherwise looks fine.
> 
> One more thing is that IMHO patch 5/5 could be squashed with this one,
> so documentation for the binding would be available at the same it is
> introduced.

Yes. it seems to be better consolidating patch1 and 5.
I'll apply it to next patch.
Thanks.

> 
> Best regards,
> Tomasz

RE: [PATCHv2 2/5] ARM: dts: Add rotator node for exynos4210

[Chanho Park]

Hi Tomasz,

> -----Original Message-----
> From: Tomasz Figa [mailto:t.figa@samsung.com]
> Sent: Friday, August 09, 2013 9:57 PM
> To: Chanho Park
> Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri-
> devel@lists.freedesktop.org; kyungmin.park@samsung.com;
> mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com;
> tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; Thomas Abraham
> Subject: Re: [PATCHv2 2/5] ARM: dts: Add rotator node for exynos4210
> 
> On Friday 09 of August 2013 16:40:50 Chanho Park wrote:
> > This patch adds a rotator node for exynos4210. The exynos4210 has
> > different limitation of image size compared with later chips.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > Cc: Thomas Abraham <thomas.abraham@linaro.org>
> > Cc: Kukjin Kim <kgene.kim@samsung.com>
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  arch/arm/boot/dts/exynos4.dtsi |    9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/exynos4.dtsi
> > b/arch/arm/boot/dts/exynos4.dtsi index 597cfcf..002b2b8 100644
> > --- a/arch/arm/boot/dts/exynos4.dtsi
> > +++ b/arch/arm/boot/dts/exynos4.dtsi
> > @@ -243,6 +243,15 @@
> >  		status = "disabled";
> >  	};
> >
> > +	rotator: rotator@12810000 {
> > +		compatible = "samsung,exynos4210-rotator";
> > +		reg = <0x12810000 0x1000>;
> > +		interrupts = <0 83 0>;
> > +		clocks = <&clock 278>;
> > +		clock-names = "rotator";
> > +		status = "disabled";
> 
> Does the rotator need any board-specific information to work? If no, it
> should not be disabled by default. Same comment goes to patch 4/5.

It does not have any board-specific information. I'll remove the status
node.
Thanks.

Best Regards,
Chanho Park

RE: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Chanho Park]

Hi Tomasz,

> -----Original Message-----
> From: Tomasz Figa [mailto:t.figa@samsung.com]
> Sent: Friday, August 09, 2013 10:16 PM
> To: Chanho Park
> Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri-
> devel@lists.freedesktop.org; kyungmin.park@samsung.com;
> mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com;
> tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; Thomas Abraham;
> swarren@wwwdotorg.org; ian.campbell@citrix.com; rob.herring@calxeda.com;
> pawel.moll@arm.com
> Subject: Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for
> exynos rotator
> 
> Hi Chanho,
> 
> On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
> > This patch describes each nodes of rotator and specifies a example how
> > to bind it.
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> > Cc: Thomas Abraham <thomas.abraham@linaro.org>
> > Cc: Kukjin Kim <kgene.kim@samsung.com>
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  .../devicetree/bindings/gpu/samsung-rotator.txt    |   26
> > ++++++++++++++++++++ 1 file changed, 26 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> > b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file
> > mode 100644 index 0000000..31ee7b5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
> > @@ -0,0 +1,26 @@
> > +* Samsung Image Rotator
> > +
> > +Required properties:
> > +  - compatible : value should be following:
> > +	(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
> > +	(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
> > +	(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
> > +
> > +  - reg : Physical base address of the IP registers and length of
> memory
> > +	  mapped region.
> > +
> > +  - interrupts : Interrupt number of Rotator
> 
> What about: interrupt specifier for rotator interrupt, according to
> format specific to interrupt parent.
> 
> > +
> > +  - clocks : Clock number described in
> > Documentations/devicetree/binding/clock. +
> 
> Perhaps: clock specifier for rotator clock, according to generic clock
> bindings.

Become more clearly. Thanks.

> 
> > +  - clock-names : Clock name.
> 
> Names of clocks specified in "clocks" property. For exynos rotator this
> property should contain only one name: "rotator".
> 
> > +Example:
> > +	rotator: rotator@12800000 {
> 
> I wonder if we shouldn't use a more generic name here, according to
> ePAPR node naming recommendation.

Hmm. I'm not sure what name is better than rotator.
How about

image-rotator@12810000

> 
> > +		compatible = "samsung,exynos4210-rotator";
> > +		reg = <0x12810000 0x1000>;
> 
> Typo. Node has 12800000 in unit-address suffix, but this property has
> 12810000 instead.

Yes. It's typo. I'll correct it now.

> 
> > +		interrupts = <0 83 0>;
> > +		clocks = <&clock 278>;
> > +		clock-names = "rotator";
> > +		status = "disabled";
> 
> Status property should be omitted from this example, as it's not a part
> of this binding.

I'll remove the property.
Thanks.

Best Regards,
Chanho Park