[PATCH v2 0/4] video: mmp: add device tree suppport

[PATCH v2 0/4] video: mmp: add device tree suppport

[Zhou Zhu]

This patch set is to add device tree support for mmp_disp.
patch 1/2 are to remove clk_name configure in mach_info before.
patch 3/4 are to add device tree support.

change of v2:
add patch to remove config of clk_name in mach_info
use phandle to get path.
runtime decision of dt usage.
clean up to use node name directly.

Zhou Zhu (4):
  arm: mmp: remove not used disp clk_name in ttc_dkb
  video: mmp: no need to get clk_name from mach_info
  video: mmp: add devm_mmp_get_path_by_phandle for DT
  video: mmp: add device tree support

 Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
 arch/arm/mach-mmp/ttc_dkb.c                       |    1 -
 drivers/video/mmp/core.c                          |   35 +++++
 drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
 drivers/video/mmp/hw/mmp_ctrl.c                   |  154 ++++++++++++++++-----
 include/video/mmp_disp.h                          |    3 +-
 6 files changed, 268 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt

-- 
1.7.9.5

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

[PATCH v2 0/4] video: mmp: add device tree suppport

[Zhou Zhu]

This patch set is to add device tree support for mmp_disp.
patch 1/2 are to remove clk_name configure in mach_info before.
patch 3/4 are to add device tree support.

change of v2:
add patch to remove config of clk_name in mach_info
use phandle to get path.
runtime decision of dt usage.
clean up to use node name directly.

Zhou Zhu (4):
  arm: mmp: remove not used disp clk_name in ttc_dkb
  video: mmp: no need to get clk_name from mach_info
  video: mmp: add devm_mmp_get_path_by_phandle for DT
  video: mmp: add device tree support

 Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
 arch/arm/mach-mmp/ttc_dkb.c                       |    1 -
 drivers/video/mmp/core.c                          |   35 +++++
 drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
 drivers/video/mmp/hw/mmp_ctrl.c                   |  154 ++++++++++++++++-----
 include/video/mmp_disp.h                          |    3 +-
 6 files changed, 268 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt

-- 
1.7.9.5

[PATCH v2 1/4] arm: mmp: remove not used disp clk_name in ttc_dkb

[Zhou Zhu]

remove clk_name of mmp_disp mach_info in ttc_dkb.
This field is not used by driver now.

Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/mach-mmp/ttc_dkb.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/mach-mmp/ttc_dkb.c b/arch/arm/mach-mmp/ttc_dkb.c
index cfadd97..9437166 100644
--- a/arch/arm/mach-mmp/ttc_dkb.c
+++ b/arch/arm/mach-mmp/ttc_dkb.c
@@ -204,7 +204,6 @@ static struct mmp_mach_path_config dkb_disp_config[] = {
 
 static struct mmp_mach_plat_info dkb_disp_info = {
 	.name = "mmp-disp",
-	.clk_name = "disp0",
 	.path_num = 1,
 	.paths = dkb_disp_config,
 };
-- 
1.7.9.5

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

[PATCH v2 1/4] arm: mmp: remove not used disp clk_name in ttc_dkb

[Zhou Zhu]

remove clk_name of mmp_disp mach_info in ttc_dkb.
This field is not used by driver now.

Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
---
 arch/arm/mach-mmp/ttc_dkb.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/mach-mmp/ttc_dkb.c b/arch/arm/mach-mmp/ttc_dkb.c
index cfadd97..9437166 100644
--- a/arch/arm/mach-mmp/ttc_dkb.c
+++ b/arch/arm/mach-mmp/ttc_dkb.c
@@ -204,7 +204,6 @@ static struct mmp_mach_path_config dkb_disp_config[] = {
 
 static struct mmp_mach_plat_info dkb_disp_info = {
 	.name = "mmp-disp",
-	.clk_name = "disp0",
 	.path_num = 1,
 	.paths = dkb_disp_config,
 };
-- 
1.7.9.5

[PATCH v2 2/4] video: mmp: no need to get clk_name from mach_info

As the display controller has only one clock, no need to
get clk_name from mach_info

Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
---
 drivers/video/mmp/hw/mmp_ctrl.c |    4 ++--
 include/video/mmp_disp.h        |    1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
index 8621a9f..b65913e 100644
--- a/drivers/video/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/mmp/hw/mmp_ctrl.c
@@ -521,9 +521,9 @@ static int mmphw_probe(struct platform_device *pdev)
 	}
 
 	/* get clock */
-	ctrl->clk = devm_clk_get(ctrl->dev, mi->clk_name);
+	ctrl->clk = devm_clk_get(ctrl->dev, NULL);
 	if (IS_ERR(ctrl->clk)) {
-		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
+		dev_err(ctrl->dev, "unable to get clk for %s\n", ctrl->name);
 		ret = -ENOENT;
 		goto failed;
 	}
diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
index 9fd9398..05a3a60 100644
--- a/include/video/mmp_disp.h
+++ b/include/video/mmp_disp.h
@@ -344,7 +344,6 @@ struct mmp_mach_path_config {
 
 struct mmp_mach_plat_info {
 	const char *name;
-	const char *clk_name;
 	int path_num;
 	struct mmp_mach_path_config *paths;
 };
-- 
1.7.9.5

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

[PATCH v2 2/4] video: mmp: no need to get clk_name from mach_info

[Zhou Zhu]

As the display controller has only one clock, no need to
get clk_name from mach_info

Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
---
 drivers/video/mmp/hw/mmp_ctrl.c |    4 ++--
 include/video/mmp_disp.h        |    1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
index 8621a9f..b65913e 100644
--- a/drivers/video/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/mmp/hw/mmp_ctrl.c
@@ -521,9 +521,9 @@ static int mmphw_probe(struct platform_device *pdev)
 	}
 
 	/* get clock */
-	ctrl->clk = devm_clk_get(ctrl->dev, mi->clk_name);
+	ctrl->clk = devm_clk_get(ctrl->dev, NULL);
 	if (IS_ERR(ctrl->clk)) {
-		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
+		dev_err(ctrl->dev, "unable to get clk for %s\n", ctrl->name);
 		ret = -ENOENT;
 		goto failed;
 	}
diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
index 9fd9398..05a3a60 100644
--- a/include/video/mmp_disp.h
+++ b/include/video/mmp_disp.h
@@ -344,7 +344,6 @@ struct mmp_mach_path_config {
 
 struct mmp_mach_plat_info {
 	const char *name;
-	const char *clk_name;
 	int path_num;
 	struct mmp_mach_path_config *paths;
 };
-- 
1.7.9.5

[PATCH v2 3/4] video: mmp: add devm_mmp_get_path_by_phandle for DT

[Zhou Zhu]

add devm_mmp_get_path_by_phandle to replace mmp_get_path
for DT usage

Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
---
 drivers/video/mmp/core.c |   35 +++++++++++++++++++++++++++++++++++
 include/video/mmp_disp.h |    2 ++
 2 files changed, 37 insertions(+)

diff --git a/drivers/video/mmp/core.c b/drivers/video/mmp/core.c
index b563b92..0538458 100644
--- a/drivers/video/mmp/core.c
+++ b/drivers/video/mmp/core.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/export.h>
+#include <linux/of.h>
 #include <video/mmp_disp.h>
 
 static struct mmp_overlay *path_get_overlay(struct mmp_path *path,
@@ -156,6 +157,40 @@ struct mmp_path *mmp_get_path(const char *name)
 EXPORT_SYMBOL_GPL(mmp_get_path);
 
 /*
+ * devm_mmp_get_path_by_phandle - get path by phandle
+ * @dev: device that want to get path
+ * @phandle: name of phandle in device node that want to get path
+ *
+ * this function gets path according to node pointed by phandle
+ * return NULL if no matching path
+ */
+struct mmp_path *devm_mmp_get_path_by_phandle(struct device *dev,
+	const char *phandle)
+{
+	struct mmp_path *path;
+	struct device_node *node;
+
+	if (!dev->of_node) {
+		dev_err(dev, "device does not have a device node entry\n");
+		return NULL;
+	}
+
+	node = of_parse_phandle(dev->of_node, phandle, 0);
+	if (!node) {
+		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
+			dev->of_node->name);
+		return NULL;
+	}
+
+	path = mmp_get_path(node->name);
+
+	of_node_put(node);
+
+	return path;
+}
+EXPORT_SYMBOL_GPL(devm_mmp_get_path_by_phandle);
+
+/*
  * mmp_register_path - init and register path by path_info
  * @p: path info provided by display controller
  *
diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
index 05a3a60..63138b8 100644
--- a/include/video/mmp_disp.h
+++ b/include/video/mmp_disp.h
@@ -248,6 +248,8 @@ struct mmp_path {
 };
 
 extern struct mmp_path *mmp_get_path(const char *name);
+struct mmp_path *devm_mmp_get_path_by_phandle(struct device *dev,
+		const char *phandle);
 static inline void mmp_path_set_mode(struct mmp_path *path,
 		struct mmp_mode *mode)
 {
-- 
1.7.9.5

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

[PATCH v2 3/4] video: mmp: add devm_mmp_get_path_by_phandle for DT

[Zhou Zhu]

add devm_mmp_get_path_by_phandle to replace mmp_get_path
for DT usage

Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
---
 drivers/video/mmp/core.c |   35 +++++++++++++++++++++++++++++++++++
 include/video/mmp_disp.h |    2 ++
 2 files changed, 37 insertions(+)

diff --git a/drivers/video/mmp/core.c b/drivers/video/mmp/core.c
index b563b92..0538458 100644
--- a/drivers/video/mmp/core.c
+++ b/drivers/video/mmp/core.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/export.h>
+#include <linux/of.h>
 #include <video/mmp_disp.h>
 
 static struct mmp_overlay *path_get_overlay(struct mmp_path *path,
@@ -156,6 +157,40 @@ struct mmp_path *mmp_get_path(const char *name)
 EXPORT_SYMBOL_GPL(mmp_get_path);
 
 /*
+ * devm_mmp_get_path_by_phandle - get path by phandle
+ * @dev: device that want to get path
+ * @phandle: name of phandle in device node that want to get path
+ *
+ * this function gets path according to node pointed by phandle
+ * return NULL if no matching path
+ */
+struct mmp_path *devm_mmp_get_path_by_phandle(struct device *dev,
+	const char *phandle)
+{
+	struct mmp_path *path;
+	struct device_node *node;
+
+	if (!dev->of_node) {
+		dev_err(dev, "device does not have a device node entry\n");
+		return NULL;
+	}
+
+	node = of_parse_phandle(dev->of_node, phandle, 0);
+	if (!node) {
+		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
+			dev->of_node->name);
+		return NULL;
+	}
+
+	path = mmp_get_path(node->name);
+
+	of_node_put(node);
+
+	return path;
+}
+EXPORT_SYMBOL_GPL(devm_mmp_get_path_by_phandle);
+
+/*
  * mmp_register_path - init and register path by path_info
  * @p: path info provided by display controller
  *
diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
index 05a3a60..63138b8 100644
--- a/include/video/mmp_disp.h
+++ b/include/video/mmp_disp.h
@@ -248,6 +248,8 @@ struct mmp_path {
 };
 
 extern struct mmp_path *mmp_get_path(const char *name);
+struct mmp_path *devm_mmp_get_path_by_phandle(struct device *dev,
+		const char *phandle);
 static inline void mmp_path_set_mode(struct mmp_path *path,
 		struct mmp_mode *mode)
 {
-- 
1.7.9.5

Re: [PATCH v2 3/4] video: mmp: add devm_mmp_get_path_by_phandle for DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]

On 14/01/14 13:16, Zhou Zhu wrote:
> add devm_mmp_get_path_by_phandle to replace mmp_get_path
> for DT usage
> 
> Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/video/mmp/core.c |   35 +++++++++++++++++++++++++++++++++++
>  include/video/mmp_disp.h |    2 ++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/video/mmp/core.c b/drivers/video/mmp/core.c
> index b563b92..0538458 100644
> --- a/drivers/video/mmp/core.c
> +++ b/drivers/video/mmp/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/slab.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/export.h>
> +#include <linux/of.h>
>  #include <video/mmp_disp.h>
>  
>  static struct mmp_overlay *path_get_overlay(struct mmp_path *path,
> @@ -156,6 +157,40 @@ struct mmp_path *mmp_get_path(const char *name)
>  EXPORT_SYMBOL_GPL(mmp_get_path);
>  
>  /*
> + * devm_mmp_get_path_by_phandle - get path by phandle
> + * @dev: device that want to get path
> + * @phandle: name of phandle in device node that want to get path
> + *
> + * this function gets path according to node pointed by phandle
> + * return NULL if no matching path
> + */
> +struct mmp_path *devm_mmp_get_path_by_phandle(struct device *dev,
> +	const char *phandle)
> +{
> +	struct mmp_path *path;
> +	struct device_node *node;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "device does not have a device node entry\n");
> +		return NULL;
> +	}
> +
> +	node = of_parse_phandle(dev->of_node, phandle, 0);
> +	if (!node) {
> +		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
> +			dev->of_node->name);
> +		return NULL;
> +	}
> +
> +	path = mmp_get_path(node->name);
> +
> +	of_node_put(node);
> +
> +	return path;
> +}
> +EXPORT_SYMBOL_GPL(devm_mmp_get_path_by_phandle);

Why is this function "devm_"? devm_ functions are supposed to
automatically free resources when the device is removed, I don't see
anything like that here.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 3/4] video: mmp: add devm_mmp_get_path_by_phandle for DT

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

On 14/01/14 13:16, Zhou Zhu wrote:
> add devm_mmp_get_path_by_phandle to replace mmp_get_path
> for DT usage
> 
> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
> ---
>  drivers/video/mmp/core.c |   35 +++++++++++++++++++++++++++++++++++
>  include/video/mmp_disp.h |    2 ++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/video/mmp/core.c b/drivers/video/mmp/core.c
> index b563b92..0538458 100644
> --- a/drivers/video/mmp/core.c
> +++ b/drivers/video/mmp/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/slab.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/export.h>
> +#include <linux/of.h>
>  #include <video/mmp_disp.h>
>  
>  static struct mmp_overlay *path_get_overlay(struct mmp_path *path,
> @@ -156,6 +157,40 @@ struct mmp_path *mmp_get_path(const char *name)
>  EXPORT_SYMBOL_GPL(mmp_get_path);
>  
>  /*
> + * devm_mmp_get_path_by_phandle - get path by phandle
> + * @dev: device that want to get path
> + * @phandle: name of phandle in device node that want to get path
> + *
> + * this function gets path according to node pointed by phandle
> + * return NULL if no matching path
> + */
> +struct mmp_path *devm_mmp_get_path_by_phandle(struct device *dev,
> +	const char *phandle)
> +{
> +	struct mmp_path *path;
> +	struct device_node *node;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "device does not have a device node entry\n");
> +		return NULL;
> +	}
> +
> +	node = of_parse_phandle(dev->of_node, phandle, 0);
> +	if (!node) {
> +		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
> +			dev->of_node->name);
> +		return NULL;
> +	}
> +
> +	path = mmp_get_path(node->name);
> +
> +	of_node_put(node);
> +
> +	return path;
> +}
> +EXPORT_SYMBOL_GPL(devm_mmp_get_path_by_phandle);

Why is this function "devm_"? devm_ functions are supposed to
automatically free resources when the device is removed, I don't see
anything like that here.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 3/4] video: mmp: add devm_mmp_get_path_by_phandle for DT

[Zhou Zhu]

On 02/10/2014 08:32 PM, Tomi Valkeinen wrote:
> On 14/01/14 13:16, Zhou Zhu wrote:
>> add devm_mmp_get_path_by_phandle to replace mmp_get_path
>> for DT usage
>>
>> Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/video/mmp/core.c |   35 +++++++++++++++++++++++++++++++++++
>>   include/video/mmp_disp.h |    2 ++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/video/mmp/core.c b/drivers/video/mmp/core.c
>> index b563b92..0538458 100644
>> --- a/drivers/video/mmp/core.c
>> +++ b/drivers/video/mmp/core.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/export.h>
>> +#include <linux/of.h>
>>   #include <video/mmp_disp.h>
>>
>>   static struct mmp_overlay *path_get_overlay(struct mmp_path *path,
>> @@ -156,6 +157,40 @@ struct mmp_path *mmp_get_path(const char *name)
>>   EXPORT_SYMBOL_GPL(mmp_get_path);
>>
>>   /*
>> + * devm_mmp_get_path_by_phandle - get path by phandle
>> + * @dev: device that want to get path
>> + * @phandle: name of phandle in device node that want to get path
>> + *
>> + * this function gets path according to node pointed by phandle
>> + * return NULL if no matching path
>> + */
>> +struct mmp_path *devm_mmp_get_path_by_phandle(struct device *dev,
>> +	const char *phandle)
>> +{
>> +	struct mmp_path *path;
>> +	struct device_node *node;
>> +
>> +	if (!dev->of_node) {
>> +		dev_err(dev, "device does not have a device node entry\n");
>> +		return NULL;
>> +	}
>> +
>> +	node = of_parse_phandle(dev->of_node, phandle, 0);
>> +	if (!node) {
>> +		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
>> +			dev->of_node->name);
>> +		return NULL;
>> +	}
>> +
>> +	path = mmp_get_path(node->name);
>> +
>> +	of_node_put(node);
>> +
>> +	return path;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mmp_get_path_by_phandle);
>
> Why is this function "devm_"? devm_ functions are supposed to
> automatically free resources when the device is removed, I don't see
> anything like that here.

Thank you for your review, I will remove "devm_" in next version.

>
>   Tomi
>
>


-- 
Thanks, -Zhou
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] video: mmp: add devm_mmp_get_path_by_phandle for DT

[Zhou Zhu]

On 02/10/2014 08:32 PM, Tomi Valkeinen wrote:
> On 14/01/14 13:16, Zhou Zhu wrote:
>> add devm_mmp_get_path_by_phandle to replace mmp_get_path
>> for DT usage
>>
>> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
>> ---
>>   drivers/video/mmp/core.c |   35 +++++++++++++++++++++++++++++++++++
>>   include/video/mmp_disp.h |    2 ++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/video/mmp/core.c b/drivers/video/mmp/core.c
>> index b563b92..0538458 100644
>> --- a/drivers/video/mmp/core.c
>> +++ b/drivers/video/mmp/core.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/dma-mapping.h>
>>   #include <linux/export.h>
>> +#include <linux/of.h>
>>   #include <video/mmp_disp.h>
>>
>>   static struct mmp_overlay *path_get_overlay(struct mmp_path *path,
>> @@ -156,6 +157,40 @@ struct mmp_path *mmp_get_path(const char *name)
>>   EXPORT_SYMBOL_GPL(mmp_get_path);
>>
>>   /*
>> + * devm_mmp_get_path_by_phandle - get path by phandle
>> + * @dev: device that want to get path
>> + * @phandle: name of phandle in device node that want to get path
>> + *
>> + * this function gets path according to node pointed by phandle
>> + * return NULL if no matching path
>> + */
>> +struct mmp_path *devm_mmp_get_path_by_phandle(struct device *dev,
>> +	const char *phandle)
>> +{
>> +	struct mmp_path *path;
>> +	struct device_node *node;
>> +
>> +	if (!dev->of_node) {
>> +		dev_err(dev, "device does not have a device node entry\n");
>> +		return NULL;
>> +	}
>> +
>> +	node = of_parse_phandle(dev->of_node, phandle, 0);
>> +	if (!node) {
>> +		dev_err(dev, "failed to get %s phandle in %s node\n", phandle,
>> +			dev->of_node->name);
>> +		return NULL;
>> +	}
>> +
>> +	path = mmp_get_path(node->name);
>> +
>> +	of_node_put(node);
>> +
>> +	return path;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mmp_get_path_by_phandle);
>
> Why is this function "devm_"? devm_ functions are supposed to
> automatically free resources when the device is removed, I don't see
> anything like that here.

Thank you for your review, I will remove "devm_" in next version.

>
>   Tomi
>
>


-- 
Thanks, -Zhou

[PATCH v2 4/4] video: mmp: add device tree support

[Zhou Zhu]

add device tree support for mmp fb/controller
the description of DT config is at
Documentation/devicetree/bindings/fb/mmp-disp.txt

Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
---
 Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
 drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
 drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
 3 files changed, 235 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt

diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
new file mode 100644
index 0000000..80702f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
@@ -0,0 +1,60 @@
+* Marvell MMP Display (MMP_DISP)
+
+To config mmp display, 3 parts are required to be set in dts:
+1. mmp fb
+Required properties:
+- compatible: Should be "marvell,<soc>-fb".
+- marvell,path: Should be the path this fb connecting to.
+- marvell,overlay-id: Should be the id of overlay this fb is on.
+- marvell,dmafetch-id: Should be the dma fetch id this fb using.
+- marvell,default-pixfmt: Should be the default pixel format when this fb is
+turned on.
+
+2. mmp controller
+Required properties:
+- compatible: Should be "marvell,<soc>-disp".
+- reg: Should be address and length of the register set for this controller.
+- interrupts: Should be interrupt of this controller.
+
+Required sub-node:
+- path:
+Required properties in this sub-node:
+-- marvell,overlay_num: Should be number of overlay this path has.
+-- marvell,output-type: Should be output-type settings
+-- marvell,path-config: Should be path-config settings
+-- marvell,link-config: Should be link-config settings
+-- marvell,rbswap: Should be rbswap settings
+
+3. panel
+Required properties:
+- marvell,path: Should be path that this panel connected to.
+- other properties each panel has.
+
+Examples:
+
+fb: mmp-fb {
+	compatible = "marvell,pxa988-fb";
+	marvell,path = <&path1>;
+	marvell,overlay-id = <0>;
+	marvell,dmafetch-id = <1>;
+	marvell,default-pixfmt = <0x108>;
+};
+
+disp: mmp-disp@d420b000 {
+	compatible = "marvell,pxa988-disp";
+	reg = <0xd420b000 0x1fc>;
+	interrupts = <0 41 0x4>;
+	path1: mmp-pnpath {
+		marvell,overlay-num = <2>;
+		marvell,output-type = <0>;
+		marvell,path-config = <0x20000000>;
+		marvell,link-config = <0x60000001>;
+		marvell,rbswap = <0>;
+	};
+};
+
+panel: <panel-name> {
+	...
+	marvell,path = <&path1>;
+	...
+};
diff --git a/drivers/video/mmp/fb/mmpfb.c b/drivers/video/mmp/fb/mmpfb.c
index 7ab31eb..f919d8e 100644
--- a/drivers/video/mmp/fb/mmpfb.c
+++ b/drivers/video/mmp/fb/mmpfb.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include "mmpfb.h"
 
 static int var_to_pixfmt(struct fb_var_screeninfo *var)
@@ -551,56 +552,79 @@ static void fb_info_clear(struct fb_info *info)
 	fb_dealloc_cmap(&info->cmap);
 }
 
+static const struct of_device_id mmp_fb_dt_match[] = {
+	{ .compatible = "marvell,mmp-fb" },
+	{ .compatible = "marvell,pxa910-fb" },
+	{ .compatible = "marvell,pxa988-fb" },
+	{},
+};
+
 static int mmpfb_probe(struct platform_device *pdev)
 {
 	struct mmp_buffer_driver_mach_info *mi;
+	struct device_node *np;
 	struct fb_info *info = 0;
 	struct mmpfb_info *fbi = 0;
-	int ret, modes_num;
-
-	mi = pdev->dev.platform_data;
-	if (mi == NULL) {
-		dev_err(&pdev->dev, "no platform data defined\n");
-		return -EINVAL;
-	}
+	int ret = -EINVAL, modes_num;
+	int overlay_id = 0, dmafetch_id = 0;
 
 	/* initialize fb */
 	info = framebuffer_alloc(sizeof(struct mmpfb_info), &pdev->dev);
 	if (info == NULL)
 		return -ENOMEM;
 	fbi = info->par;
-	if (!fbi) {
-		ret = -EINVAL;
+	if (!fbi)
 		goto failed;
+
+	np = pdev->dev.of_node;
+	if (np) {
+		fbi->path = devm_mmp_get_path_by_phandle(&pdev->dev,
+					"marvell,path");
+		if (!fbi->path || of_property_read_u32(np,
+				"marvell,default-pixfmt", &fbi->pix_fmt)) {
+			dev_err(&pdev->dev, "unable to get fb setting from dt\n");
+			goto failed;
+		}
+		/* default setting if not set */
+		of_property_read_u32(np, "marvell,overlay-id", &overlay_id);
+		of_property_read_u32(np, "marvell,dmafetch-id", &dmafetch_id);
+		fbi->name = np->name;
+	} else {
+		mi = pdev->dev.platform_data;
+		if (mi == NULL) {
+			dev_err(&pdev->dev, "no platform data defined\n");
+			goto failed;
+		}
+
+		fbi->path = mmp_get_path(mi->path_name);
+		if (!fbi->path) {
+			dev_err(&pdev->dev, "can't get the path %s\n",
+					mi->path_name);
+			goto failed;
+		}
+
+		fbi->name = mi->name;
+		overlay_id = mi->overlay_id;
+		dmafetch_id = mi->dmafetch_id;
+		fbi->pix_fmt = mi->default_pixfmt;
 	}
 
 	/* init fb */
 	fbi->fb_info = info;
 	platform_set_drvdata(pdev, fbi);
 	fbi->dev = &pdev->dev;
-	fbi->name = mi->name;
-	fbi->pix_fmt = mi->default_pixfmt;
 	pixfmt_to_var(&info->var, fbi->pix_fmt);
 	mutex_init(&fbi->access_ok);
 
-	/* get display path by name */
-	fbi->path = mmp_get_path(mi->path_name);
-	if (!fbi->path) {
-		dev_err(&pdev->dev, "can't get the path %s\n", mi->path_name);
-		ret = -EINVAL;
-		goto failed_destroy_mutex;
-	}
-
 	dev_info(fbi->dev, "path %s get\n", fbi->path->name);
 
 	/* get overlay */
-	fbi->overlay = mmp_path_get_overlay(fbi->path, mi->overlay_id);
-	if (!fbi->overlay) {
-		ret = -EINVAL;
+	fbi->overlay = mmp_path_get_overlay(fbi->path, overlay_id);
+	if (!fbi->overlay)
 		goto failed_destroy_mutex;
-	}
+
 	/* set fetch used */
-	mmp_overlay_set_fetch(fbi->overlay, mi->dmafetch_id);
+	mmp_overlay_set_fetch(fbi->overlay, dmafetch_id);
 
 	modes_num = modes_setup(fbi);
 	if (modes_num < 0) {
@@ -679,6 +703,7 @@ static struct platform_driver mmpfb_driver = {
 	.driver		= {
 		.name	= "mmp-fb",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mmp_fb_dt_match),
 	},
 	.probe		= mmpfb_probe,
 };
diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
index b65913e..08b2ee7 100644
--- a/drivers/video/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/mmp/hw/mmp_ctrl.c
@@ -37,6 +37,7 @@
 #include <linux/uaccess.h>
 #include <linux/kthread.h>
 #include <linux/io.h>
+#include <linux/of.h>
 
 #include "mmp_ctrl.h"
 
@@ -396,26 +397,43 @@ static void path_set_default(struct mmp_path *path)
 	writel_relaxed(tmp, ctrl_regs(path) + dma_ctrl(0, path->id));
 }
 
-static int path_init(struct mmphw_path_plat *path_plat,
-		struct mmp_mach_path_config *config)
+static int of_path_init(struct mmphw_path_plat *path_plat,
+		struct device_node *path_np)
 {
 	struct mmphw_ctrl *ctrl = path_plat->ctrl;
 	struct mmp_path_info *path_info;
 	struct mmp_path *path = NULL;
 
-	dev_info(ctrl->dev, "%s: %s\n", __func__, config->name);
-
 	/* init driver data */
 	path_info = kzalloc(sizeof(struct mmp_path_info), GFP_KERNEL);
 	if (!path_info) {
-		dev_err(ctrl->dev, "%s: unable to alloc path_info for %s\n",
-				__func__, config->name);
-		return 0;
+		dev_err(ctrl->dev, "%s: unable to alloc path_info\n",
+				__func__);
+		return -ENOMEM;
 	}
-	path_info->name = config->name;
+
+	if (!path_np || of_property_read_u32(path_np, "marvell,overlay-num",
+				&path_info->output_type) ||
+			of_property_read_u32(path_np, "marvell,output-type",
+				&path_info->overlay_num)) {
+		dev_err(ctrl->dev, "%s: unable to get path setting from dt\n",
+			__func__);
+		kfree(path_info);
+		return -EINVAL;
+	}
+	/* allow these settings not set */
+	of_property_read_u32(path_np, "marvell,path-config",
+		&path_plat->path_config);
+	of_property_read_u32(path_np, "marvell,link-config",
+		&path_plat->link_config);
+	of_property_read_u32(path_np, "marvell,rbswap",
+		&path_plat->dsi_rbswap);
+	path_info->name = path_np->name;
+
+	dev_info(ctrl->dev, "%s: %s\n", __func__, path_info->name);
+
 	path_info->id = path_plat->id;
 	path_info->dev = ctrl->dev;
-	path_info->overlay_num = config->overlay_num;
 	path_info->overlay_ops = &mmphw_overlay_ops;
 	path_info->set_mode = path_set_mode;
 	path_info->plat_data = path_plat;
@@ -424,16 +442,56 @@ static int path_init(struct mmphw_path_plat *path_plat,
 	path = mmp_register_path(path_info);
 	if (!path) {
 		kfree(path_info);
-		return 0;
+		return -EINVAL;
 	}
 	path_plat->path = path;
+	path_set_default(path);
+
+	kfree(path_info);
+	return 0;
+}
+
+static int path_init(struct mmphw_path_plat *path_plat,
+		struct mmp_mach_path_config *config)
+{
+	struct mmphw_ctrl *ctrl = path_plat->ctrl;
+	struct mmp_path_info *path_info;
+	struct mmp_path *path = NULL;
+
+	/* init driver data */
+	path_info = kzalloc(sizeof(struct mmp_path_info), GFP_KERNEL);
+	if (!path_info) {
+		dev_err(ctrl->dev, "%s: unable to alloc path_info\n",
+				__func__);
+		return -ENOMEM;
+	}
+
+	path_info->name = config->name;
+	path_info->overlay_num = config->overlay_num;
+	path_info->output_type = config->output_type;
 	path_plat->path_config = config->path_config;
 	path_plat->link_config = config->link_config;
 	path_plat->dsi_rbswap = config->dsi_rbswap;
+
+	dev_info(ctrl->dev, "%s: %s\n", __func__, path_info->name);
+
+	path_info->id = path_plat->id;
+	path_info->dev = ctrl->dev;
+	path_info->overlay_ops = &mmphw_overlay_ops;
+	path_info->set_mode = path_set_mode;
+	path_info->plat_data = path_plat;
+
+	/* create/register platform device */
+	path = mmp_register_path(path_info);
+	if (!path) {
+		kfree(path_info);
+		return -EINVAL;
+	}
+	path_plat->path = path;
 	path_set_default(path);
 
 	kfree(path_info);
-	return 1;
+	return 0;
 }
 
 static void path_deinit(struct mmphw_path_plat *path_plat)
@@ -445,13 +503,22 @@ static void path_deinit(struct mmphw_path_plat *path_plat)
 		mmp_unregister_path(path_plat->path);
 }
 
+static const struct of_device_id mmp_disp_dt_match[] = {
+	{ .compatible = "marvell,mmp-disp" },
+	{ .compatible = "marvell,pxa910-disp" },
+	{ .compatible = "marvell,pxa988-disp" },
+	{},
+};
+
 static int mmphw_probe(struct platform_device *pdev)
 {
-	struct mmp_mach_plat_info *mi;
 	struct resource *res;
-	int ret, i, size, irq;
+	int ret, i, size, irq, path_num;
+	const char *disp_name;
 	struct mmphw_path_plat *path_plat;
 	struct mmphw_ctrl *ctrl = NULL;
+	struct mmp_mach_plat_info *mi = pdev->dev.platform_data;
+	struct device_node *np, *path_np = NULL;
 
 	/* get resources from platform data */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -468,25 +535,38 @@ static int mmphw_probe(struct platform_device *pdev)
 		goto failed;
 	}
 
-	/* get configs from platform data */
-	mi = pdev->dev.platform_data;
-	if (mi == NULL || !mi->path_num || !mi->paths) {
-		dev_err(&pdev->dev, "%s: no platform data defined\n", __func__);
-		ret = -EINVAL;
-		goto failed;
+	np = pdev->dev.of_node;
+	if (np) {
+		path_num = of_get_child_count(np);
+		if (path_num <= 0) {
+			dev_err(&pdev->dev, "%s: failed to get settings from dt\n",
+				__func__);
+			ret = -EINVAL;
+			goto failed;
+		}
+		disp_name = np->name;
+	} else {
+		if (mi == NULL || !mi->path_num || !mi->paths) {
+			dev_err(&pdev->dev, "%s: no platform data defined\n",
+				__func__);
+			ret = -EINVAL;
+			goto failed;
+		}
+
+		disp_name = mi->name;
+		path_num = mi->path_num;
 	}
 
 	/* allocate */
 	size = sizeof(struct mmphw_ctrl) + sizeof(struct mmphw_path_plat) *
-	       mi->path_num;
+	       path_num;
 	ctrl = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
 	if (!ctrl) {
 		ret = -ENOMEM;
 		goto failed;
 	}
-
-	ctrl->name = mi->name;
-	ctrl->path_num = mi->path_num;
+	ctrl->path_num = path_num;
+	ctrl->name = disp_name;
 	ctrl->dev = &pdev->dev;
 	ctrl->irq = irq;
 	platform_set_drvdata(pdev, ctrl);
@@ -532,20 +612,31 @@ static int mmphw_probe(struct platform_device *pdev)
 	/* init global regs */
 	ctrl_set_default(ctrl);
 
-	/* init pathes from machine info and register them */
-	for (i = 0; i < ctrl->path_num; i++) {
-		/* get from config and machine info */
-		path_plat = &ctrl->path_plats[i];
-		path_plat->id = i;
-		path_plat->ctrl = ctrl;
-
-		/* path init */
-		if (!path_init(path_plat, &mi->paths[i])) {
-			ret = -EINVAL;
-			goto failed_path_init;
+	if (np) {
+		i = 0;
+		for_each_child_of_node(np, path_np) {
+			path_plat = &ctrl->path_plats[i];
+			path_plat->id = i;
+			path_plat->ctrl = ctrl;
+			i++;
+
+			ret = of_path_init(path_plat, path_np);
+			if (ret)
+				goto failed_path_init;
+		}
+	} else {
+		for (i = 0; i < ctrl->path_num; i++) {
+			path_plat = &ctrl->path_plats[i];
+			path_plat->id = i;
+			path_plat->ctrl = ctrl;
+
+			ret = path_init(path_plat, &mi->paths[i]);
+			if (ret)
+				goto failed_path_init;
 		}
 	}
 
+
 #ifdef CONFIG_MMP_DISP_SPI
 	ret = lcd_spi_register(ctrl);
 	if (ret < 0)
@@ -573,6 +664,7 @@ static struct platform_driver mmphw_driver = {
 	.driver		= {
 		.name	= "mmp-disp",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mmp_disp_dt_match),
 	},
 	.probe		= mmphw_probe,
 };
-- 
1.7.9.5

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

[PATCH v2 4/4] video: mmp: add device tree support

[Zhou Zhu]

add device tree support for mmp fb/controller
the description of DT config is at
Documentation/devicetree/bindings/fb/mmp-disp.txt

Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
---
 Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
 drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
 drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
 3 files changed, 235 insertions(+), 58 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt

diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
new file mode 100644
index 0000000..80702f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
@@ -0,0 +1,60 @@
+* Marvell MMP Display (MMP_DISP)
+
+To config mmp display, 3 parts are required to be set in dts:
+1. mmp fb
+Required properties:
+- compatible: Should be "marvell,<soc>-fb".
+- marvell,path: Should be the path this fb connecting to.
+- marvell,overlay-id: Should be the id of overlay this fb is on.
+- marvell,dmafetch-id: Should be the dma fetch id this fb using.
+- marvell,default-pixfmt: Should be the default pixel format when this fb is
+turned on.
+
+2. mmp controller
+Required properties:
+- compatible: Should be "marvell,<soc>-disp".
+- reg: Should be address and length of the register set for this controller.
+- interrupts: Should be interrupt of this controller.
+
+Required sub-node:
+- path:
+Required properties in this sub-node:
+-- marvell,overlay_num: Should be number of overlay this path has.
+-- marvell,output-type: Should be output-type settings
+-- marvell,path-config: Should be path-config settings
+-- marvell,link-config: Should be link-config settings
+-- marvell,rbswap: Should be rbswap settings
+
+3. panel
+Required properties:
+- marvell,path: Should be path that this panel connected to.
+- other properties each panel has.
+
+Examples:
+
+fb: mmp-fb {
+	compatible = "marvell,pxa988-fb";
+	marvell,path = <&path1>;
+	marvell,overlay-id = <0>;
+	marvell,dmafetch-id = <1>;
+	marvell,default-pixfmt = <0x108>;
+};
+
+disp: mmp-disp@d420b000 {
+	compatible = "marvell,pxa988-disp";
+	reg = <0xd420b000 0x1fc>;
+	interrupts = <0 41 0x4>;
+	path1: mmp-pnpath {
+		marvell,overlay-num = <2>;
+		marvell,output-type = <0>;
+		marvell,path-config = <0x20000000>;
+		marvell,link-config = <0x60000001>;
+		marvell,rbswap = <0>;
+	};
+};
+
+panel: <panel-name> {
+	...
+	marvell,path = <&path1>;
+	...
+};
diff --git a/drivers/video/mmp/fb/mmpfb.c b/drivers/video/mmp/fb/mmpfb.c
index 7ab31eb..f919d8e 100644
--- a/drivers/video/mmp/fb/mmpfb.c
+++ b/drivers/video/mmp/fb/mmpfb.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include "mmpfb.h"
 
 static int var_to_pixfmt(struct fb_var_screeninfo *var)
@@ -551,56 +552,79 @@ static void fb_info_clear(struct fb_info *info)
 	fb_dealloc_cmap(&info->cmap);
 }
 
+static const struct of_device_id mmp_fb_dt_match[] = {
+	{ .compatible = "marvell,mmp-fb" },
+	{ .compatible = "marvell,pxa910-fb" },
+	{ .compatible = "marvell,pxa988-fb" },
+	{},
+};
+
 static int mmpfb_probe(struct platform_device *pdev)
 {
 	struct mmp_buffer_driver_mach_info *mi;
+	struct device_node *np;
 	struct fb_info *info = 0;
 	struct mmpfb_info *fbi = 0;
-	int ret, modes_num;
-
-	mi = pdev->dev.platform_data;
-	if (mi = NULL) {
-		dev_err(&pdev->dev, "no platform data defined\n");
-		return -EINVAL;
-	}
+	int ret = -EINVAL, modes_num;
+	int overlay_id = 0, dmafetch_id = 0;
 
 	/* initialize fb */
 	info = framebuffer_alloc(sizeof(struct mmpfb_info), &pdev->dev);
 	if (info = NULL)
 		return -ENOMEM;
 	fbi = info->par;
-	if (!fbi) {
-		ret = -EINVAL;
+	if (!fbi)
 		goto failed;
+
+	np = pdev->dev.of_node;
+	if (np) {
+		fbi->path = devm_mmp_get_path_by_phandle(&pdev->dev,
+					"marvell,path");
+		if (!fbi->path || of_property_read_u32(np,
+				"marvell,default-pixfmt", &fbi->pix_fmt)) {
+			dev_err(&pdev->dev, "unable to get fb setting from dt\n");
+			goto failed;
+		}
+		/* default setting if not set */
+		of_property_read_u32(np, "marvell,overlay-id", &overlay_id);
+		of_property_read_u32(np, "marvell,dmafetch-id", &dmafetch_id);
+		fbi->name = np->name;
+	} else {
+		mi = pdev->dev.platform_data;
+		if (mi = NULL) {
+			dev_err(&pdev->dev, "no platform data defined\n");
+			goto failed;
+		}
+
+		fbi->path = mmp_get_path(mi->path_name);
+		if (!fbi->path) {
+			dev_err(&pdev->dev, "can't get the path %s\n",
+					mi->path_name);
+			goto failed;
+		}
+
+		fbi->name = mi->name;
+		overlay_id = mi->overlay_id;
+		dmafetch_id = mi->dmafetch_id;
+		fbi->pix_fmt = mi->default_pixfmt;
 	}
 
 	/* init fb */
 	fbi->fb_info = info;
 	platform_set_drvdata(pdev, fbi);
 	fbi->dev = &pdev->dev;
-	fbi->name = mi->name;
-	fbi->pix_fmt = mi->default_pixfmt;
 	pixfmt_to_var(&info->var, fbi->pix_fmt);
 	mutex_init(&fbi->access_ok);
 
-	/* get display path by name */
-	fbi->path = mmp_get_path(mi->path_name);
-	if (!fbi->path) {
-		dev_err(&pdev->dev, "can't get the path %s\n", mi->path_name);
-		ret = -EINVAL;
-		goto failed_destroy_mutex;
-	}
-
 	dev_info(fbi->dev, "path %s get\n", fbi->path->name);
 
 	/* get overlay */
-	fbi->overlay = mmp_path_get_overlay(fbi->path, mi->overlay_id);
-	if (!fbi->overlay) {
-		ret = -EINVAL;
+	fbi->overlay = mmp_path_get_overlay(fbi->path, overlay_id);
+	if (!fbi->overlay)
 		goto failed_destroy_mutex;
-	}
+
 	/* set fetch used */
-	mmp_overlay_set_fetch(fbi->overlay, mi->dmafetch_id);
+	mmp_overlay_set_fetch(fbi->overlay, dmafetch_id);
 
 	modes_num = modes_setup(fbi);
 	if (modes_num < 0) {
@@ -679,6 +703,7 @@ static struct platform_driver mmpfb_driver = {
 	.driver		= {
 		.name	= "mmp-fb",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mmp_fb_dt_match),
 	},
 	.probe		= mmpfb_probe,
 };
diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
index b65913e..08b2ee7 100644
--- a/drivers/video/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/mmp/hw/mmp_ctrl.c
@@ -37,6 +37,7 @@
 #include <linux/uaccess.h>
 #include <linux/kthread.h>
 #include <linux/io.h>
+#include <linux/of.h>
 
 #include "mmp_ctrl.h"
 
@@ -396,26 +397,43 @@ static void path_set_default(struct mmp_path *path)
 	writel_relaxed(tmp, ctrl_regs(path) + dma_ctrl(0, path->id));
 }
 
-static int path_init(struct mmphw_path_plat *path_plat,
-		struct mmp_mach_path_config *config)
+static int of_path_init(struct mmphw_path_plat *path_plat,
+		struct device_node *path_np)
 {
 	struct mmphw_ctrl *ctrl = path_plat->ctrl;
 	struct mmp_path_info *path_info;
 	struct mmp_path *path = NULL;
 
-	dev_info(ctrl->dev, "%s: %s\n", __func__, config->name);
-
 	/* init driver data */
 	path_info = kzalloc(sizeof(struct mmp_path_info), GFP_KERNEL);
 	if (!path_info) {
-		dev_err(ctrl->dev, "%s: unable to alloc path_info for %s\n",
-				__func__, config->name);
-		return 0;
+		dev_err(ctrl->dev, "%s: unable to alloc path_info\n",
+				__func__);
+		return -ENOMEM;
 	}
-	path_info->name = config->name;
+
+	if (!path_np || of_property_read_u32(path_np, "marvell,overlay-num",
+				&path_info->output_type) ||
+			of_property_read_u32(path_np, "marvell,output-type",
+				&path_info->overlay_num)) {
+		dev_err(ctrl->dev, "%s: unable to get path setting from dt\n",
+			__func__);
+		kfree(path_info);
+		return -EINVAL;
+	}
+	/* allow these settings not set */
+	of_property_read_u32(path_np, "marvell,path-config",
+		&path_plat->path_config);
+	of_property_read_u32(path_np, "marvell,link-config",
+		&path_plat->link_config);
+	of_property_read_u32(path_np, "marvell,rbswap",
+		&path_plat->dsi_rbswap);
+	path_info->name = path_np->name;
+
+	dev_info(ctrl->dev, "%s: %s\n", __func__, path_info->name);
+
 	path_info->id = path_plat->id;
 	path_info->dev = ctrl->dev;
-	path_info->overlay_num = config->overlay_num;
 	path_info->overlay_ops = &mmphw_overlay_ops;
 	path_info->set_mode = path_set_mode;
 	path_info->plat_data = path_plat;
@@ -424,16 +442,56 @@ static int path_init(struct mmphw_path_plat *path_plat,
 	path = mmp_register_path(path_info);
 	if (!path) {
 		kfree(path_info);
-		return 0;
+		return -EINVAL;
 	}
 	path_plat->path = path;
+	path_set_default(path);
+
+	kfree(path_info);
+	return 0;
+}
+
+static int path_init(struct mmphw_path_plat *path_plat,
+		struct mmp_mach_path_config *config)
+{
+	struct mmphw_ctrl *ctrl = path_plat->ctrl;
+	struct mmp_path_info *path_info;
+	struct mmp_path *path = NULL;
+
+	/* init driver data */
+	path_info = kzalloc(sizeof(struct mmp_path_info), GFP_KERNEL);
+	if (!path_info) {
+		dev_err(ctrl->dev, "%s: unable to alloc path_info\n",
+				__func__);
+		return -ENOMEM;
+	}
+
+	path_info->name = config->name;
+	path_info->overlay_num = config->overlay_num;
+	path_info->output_type = config->output_type;
 	path_plat->path_config = config->path_config;
 	path_plat->link_config = config->link_config;
 	path_plat->dsi_rbswap = config->dsi_rbswap;
+
+	dev_info(ctrl->dev, "%s: %s\n", __func__, path_info->name);
+
+	path_info->id = path_plat->id;
+	path_info->dev = ctrl->dev;
+	path_info->overlay_ops = &mmphw_overlay_ops;
+	path_info->set_mode = path_set_mode;
+	path_info->plat_data = path_plat;
+
+	/* create/register platform device */
+	path = mmp_register_path(path_info);
+	if (!path) {
+		kfree(path_info);
+		return -EINVAL;
+	}
+	path_plat->path = path;
 	path_set_default(path);
 
 	kfree(path_info);
-	return 1;
+	return 0;
 }
 
 static void path_deinit(struct mmphw_path_plat *path_plat)
@@ -445,13 +503,22 @@ static void path_deinit(struct mmphw_path_plat *path_plat)
 		mmp_unregister_path(path_plat->path);
 }
 
+static const struct of_device_id mmp_disp_dt_match[] = {
+	{ .compatible = "marvell,mmp-disp" },
+	{ .compatible = "marvell,pxa910-disp" },
+	{ .compatible = "marvell,pxa988-disp" },
+	{},
+};
+
 static int mmphw_probe(struct platform_device *pdev)
 {
-	struct mmp_mach_plat_info *mi;
 	struct resource *res;
-	int ret, i, size, irq;
+	int ret, i, size, irq, path_num;
+	const char *disp_name;
 	struct mmphw_path_plat *path_plat;
 	struct mmphw_ctrl *ctrl = NULL;
+	struct mmp_mach_plat_info *mi = pdev->dev.platform_data;
+	struct device_node *np, *path_np = NULL;
 
 	/* get resources from platform data */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -468,25 +535,38 @@ static int mmphw_probe(struct platform_device *pdev)
 		goto failed;
 	}
 
-	/* get configs from platform data */
-	mi = pdev->dev.platform_data;
-	if (mi = NULL || !mi->path_num || !mi->paths) {
-		dev_err(&pdev->dev, "%s: no platform data defined\n", __func__);
-		ret = -EINVAL;
-		goto failed;
+	np = pdev->dev.of_node;
+	if (np) {
+		path_num = of_get_child_count(np);
+		if (path_num <= 0) {
+			dev_err(&pdev->dev, "%s: failed to get settings from dt\n",
+				__func__);
+			ret = -EINVAL;
+			goto failed;
+		}
+		disp_name = np->name;
+	} else {
+		if (mi = NULL || !mi->path_num || !mi->paths) {
+			dev_err(&pdev->dev, "%s: no platform data defined\n",
+				__func__);
+			ret = -EINVAL;
+			goto failed;
+		}
+
+		disp_name = mi->name;
+		path_num = mi->path_num;
 	}
 
 	/* allocate */
 	size = sizeof(struct mmphw_ctrl) + sizeof(struct mmphw_path_plat) *
-	       mi->path_num;
+	       path_num;
 	ctrl = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
 	if (!ctrl) {
 		ret = -ENOMEM;
 		goto failed;
 	}
-
-	ctrl->name = mi->name;
-	ctrl->path_num = mi->path_num;
+	ctrl->path_num = path_num;
+	ctrl->name = disp_name;
 	ctrl->dev = &pdev->dev;
 	ctrl->irq = irq;
 	platform_set_drvdata(pdev, ctrl);
@@ -532,20 +612,31 @@ static int mmphw_probe(struct platform_device *pdev)
 	/* init global regs */
 	ctrl_set_default(ctrl);
 
-	/* init pathes from machine info and register them */
-	for (i = 0; i < ctrl->path_num; i++) {
-		/* get from config and machine info */
-		path_plat = &ctrl->path_plats[i];
-		path_plat->id = i;
-		path_plat->ctrl = ctrl;
-
-		/* path init */
-		if (!path_init(path_plat, &mi->paths[i])) {
-			ret = -EINVAL;
-			goto failed_path_init;
+	if (np) {
+		i = 0;
+		for_each_child_of_node(np, path_np) {
+			path_plat = &ctrl->path_plats[i];
+			path_plat->id = i;
+			path_plat->ctrl = ctrl;
+			i++;
+
+			ret = of_path_init(path_plat, path_np);
+			if (ret)
+				goto failed_path_init;
+		}
+	} else {
+		for (i = 0; i < ctrl->path_num; i++) {
+			path_plat = &ctrl->path_plats[i];
+			path_plat->id = i;
+			path_plat->ctrl = ctrl;
+
+			ret = path_init(path_plat, &mi->paths[i]);
+			if (ret)
+				goto failed_path_init;
 		}
 	}
 
+
 #ifdef CONFIG_MMP_DISP_SPI
 	ret = lcd_spi_register(ctrl);
 	if (ret < 0)
@@ -573,6 +664,7 @@ static struct platform_driver mmphw_driver = {
 	.driver		= {
 		.name	= "mmp-disp",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(mmp_disp_dt_match),
 	},
 	.probe		= mmphw_probe,
 };
-- 
1.7.9.5

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3657 bytes --]

On 14/01/14 13:16, Zhou Zhu wrote:
> add device tree support for mmp fb/controller
> the description of DT config is at
> Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>  3 files changed, 235 insertions(+), 58 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> new file mode 100644
> index 0000000..80702f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> @@ -0,0 +1,60 @@
> +* Marvell MMP Display (MMP_DISP)
> +
> +To config mmp display, 3 parts are required to be set in dts:
> +1. mmp fb
> +Required properties:
> +- compatible: Should be "marvell,<soc>-fb".
> +- marvell,path: Should be the path this fb connecting to.
> +- marvell,overlay-id: Should be the id of overlay this fb is on.
> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.
> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
> +turned on.
> +
> +2. mmp controller
> +Required properties:
> +- compatible: Should be "marvell,<soc>-disp".
> +- reg: Should be address and length of the register set for this controller.
> +- interrupts: Should be interrupt of this controller.
> +
> +Required sub-node:
> +- path:
> +Required properties in this sub-node:
> +-- marvell,overlay_num: Should be number of overlay this path has.

If that one tells how many overlays there are, maybe "num_overlays"
would be better.

> +-- marvell,output-type: Should be output-type settings
> +-- marvell,path-config: Should be path-config settings
> +-- marvell,link-config: Should be link-config settings
> +-- marvell,rbswap: Should be rbswap settings

If these terms (output-type, path-config, ...) are straight from the HW
manual, then fine. But if they are not clear, or are driver specific,
the values these can have should be documented here.

> +
> +3. panel
> +Required properties:
> +- marvell,path: Should be path that this panel connected to.
> +- other properties each panel has.
> +
> +Examples:
> +
> +fb: mmp-fb {
> +	compatible = "marvell,pxa988-fb";
> +	marvell,path = <&path1>;
> +	marvell,overlay-id = <0>;
> +	marvell,dmafetch-id = <1>;
> +	marvell,default-pixfmt = <0x108>;
> +};
> +
> +disp: mmp-disp@d420b000 {
> +	compatible = "marvell,pxa988-disp";
> +	reg = <0xd420b000 0x1fc>;
> +	interrupts = <0 41 0x4>;
> +	path1: mmp-pnpath {
> +		marvell,overlay-num = <2>;
> +		marvell,output-type = <0>;
> +		marvell,path-config = <0x20000000>;
> +		marvell,link-config = <0x60000001>;
> +		marvell,rbswap = <0>;
> +	};
> +};
> +
> +panel: <panel-name> {

How is the panel linked to all this? The nodes above do not refer to the
panel.

> +	...
> +	marvell,path = <&path1>;
> +	...
> +};

It's a bit difficult to say much about this, as I have no knowledge
about mmp.

But I don't quite understand what the pxa988-fb is. Is that some kind of
virtual device, only used to set up fbdev side? And pxa988-disp is the
actual hardware device?

If so, I don't really think pxa988-fb should exist in the DT data at
all, as it's not a hardware device.

Why isn't there just one node for pxa988-disp, which would contain all
the above information?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3629 bytes --]

On 14/01/14 13:16, Zhou Zhu wrote:
> add device tree support for mmp fb/controller
> the description of DT config is at
> Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
> ---
>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>  3 files changed, 235 insertions(+), 58 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> new file mode 100644
> index 0000000..80702f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> @@ -0,0 +1,60 @@
> +* Marvell MMP Display (MMP_DISP)
> +
> +To config mmp display, 3 parts are required to be set in dts:
> +1. mmp fb
> +Required properties:
> +- compatible: Should be "marvell,<soc>-fb".
> +- marvell,path: Should be the path this fb connecting to.
> +- marvell,overlay-id: Should be the id of overlay this fb is on.
> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.
> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
> +turned on.
> +
> +2. mmp controller
> +Required properties:
> +- compatible: Should be "marvell,<soc>-disp".
> +- reg: Should be address and length of the register set for this controller.
> +- interrupts: Should be interrupt of this controller.
> +
> +Required sub-node:
> +- path:
> +Required properties in this sub-node:
> +-- marvell,overlay_num: Should be number of overlay this path has.

If that one tells how many overlays there are, maybe "num_overlays"
would be better.

> +-- marvell,output-type: Should be output-type settings
> +-- marvell,path-config: Should be path-config settings
> +-- marvell,link-config: Should be link-config settings
> +-- marvell,rbswap: Should be rbswap settings

If these terms (output-type, path-config, ...) are straight from the HW
manual, then fine. But if they are not clear, or are driver specific,
the values these can have should be documented here.

> +
> +3. panel
> +Required properties:
> +- marvell,path: Should be path that this panel connected to.
> +- other properties each panel has.
> +
> +Examples:
> +
> +fb: mmp-fb {
> +	compatible = "marvell,pxa988-fb";
> +	marvell,path = <&path1>;
> +	marvell,overlay-id = <0>;
> +	marvell,dmafetch-id = <1>;
> +	marvell,default-pixfmt = <0x108>;
> +};
> +
> +disp: mmp-disp@d420b000 {
> +	compatible = "marvell,pxa988-disp";
> +	reg = <0xd420b000 0x1fc>;
> +	interrupts = <0 41 0x4>;
> +	path1: mmp-pnpath {
> +		marvell,overlay-num = <2>;
> +		marvell,output-type = <0>;
> +		marvell,path-config = <0x20000000>;
> +		marvell,link-config = <0x60000001>;
> +		marvell,rbswap = <0>;
> +	};
> +};
> +
> +panel: <panel-name> {

How is the panel linked to all this? The nodes above do not refer to the
panel.

> +	...
> +	marvell,path = <&path1>;
> +	...
> +};

It's a bit difficult to say much about this, as I have no knowledge
about mmp.

But I don't quite understand what the pxa988-fb is. Is that some kind of
virtual device, only used to set up fbdev side? And pxa988-disp is the
actual hardware device?

If so, I don't really think pxa988-fb should exist in the DT data at
all, as it's not a hardware device.

Why isn't there just one node for pxa988-disp, which would contain all
the above information?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Zhou Zhu]

On 02/10/2014 08:43 PM, Tomi Valkeinen wrote:
> On 14/01/14 13:16, Zhou Zhu wrote:
>> add device tree support for mmp fb/controller
>> the description of DT config is at
>> Documentation/devicetree/bindings/fb/mmp-disp.txt
>>
>> Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>>   drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>>   drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>>   3 files changed, 235 insertions(+), 58 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
>>
>> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
>> new file mode 100644
>> index 0000000..80702f5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
>> @@ -0,0 +1,60 @@
>> +* Marvell MMP Display (MMP_DISP)
>> +
>> +To config mmp display, 3 parts are required to be set in dts:
>> +1. mmp fb
>> +Required properties:
>> +- compatible: Should be "marvell,<soc>-fb".
>> +- marvell,path: Should be the path this fb connecting to.
>> +- marvell,overlay-id: Should be the id of overlay this fb is on.
>> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.
>> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
>> +turned on.
>> +
>> +2. mmp controller
>> +Required properties:
>> +- compatible: Should be "marvell,<soc>-disp".
>> +- reg: Should be address and length of the register set for this controller.
>> +- interrupts: Should be interrupt of this controller.
>> +
>> +Required sub-node:
>> +- path:
>> +Required properties in this sub-node:
>> +-- marvell,overlay_num: Should be number of overlay this path has.
>
> If that one tells how many overlays there are, maybe "num_overlays"
> would be better.
I will update it in next version.
>
>> +-- marvell,output-type: Should be output-type settings
>> +-- marvell,path-config: Should be path-config settings
>> +-- marvell,link-config: Should be link-config settings
>> +-- marvell,rbswap: Should be rbswap settings
>
> If these terms (output-type, path-config, ...) are straight from the HW
> manual, then fine. But if they are not clear, or are driver specific,
> the values these can have should be documented here.
Yes, it's straight from HW manual.
>
>> +
>> +3. panel
>> +Required properties:
>> +- marvell,path: Should be path that this panel connected to.
>> +- other properties each panel has.
>> +
>> +Examples:
>> +
>> +fb: mmp-fb {
>> +	compatible = "marvell,pxa988-fb";
>> +	marvell,path = <&path1>;
>> +	marvell,overlay-id = <0>;
>> +	marvell,dmafetch-id = <1>;
>> +	marvell,default-pixfmt = <0x108>;
>> +};
>> +
>> +disp: mmp-disp@d420b000 {
>> +	compatible = "marvell,pxa988-disp";
>> +	reg = <0xd420b000 0x1fc>;
>> +	interrupts = <0 41 0x4>;
>> +	path1: mmp-pnpath {
>> +		marvell,overlay-num = <2>;
>> +		marvell,output-type = <0>;
>> +		marvell,path-config = <0x20000000>;
>> +		marvell,link-config = <0x60000001>;
>> +		marvell,rbswap = <0>;
>> +	};
>> +};
>> +
>> +panel: <panel-name> {
>
> How is the panel linked to all this? The nodes above do not refer to the
> panel.
We are making panel refer to path, so when panel dev probe, it could 
register to related path.
The reason we not link from path to panel is our customer sometimes 
asked us to use same image pack (include dts) for different panel types 
in product. We could only add all these panels in dts and detect panel 
dynamically when boot. So moving panel out and making path not link to 
panel but panel link to path would be more straight forward.
>
>> +	...
>> +	marvell,path = <&path1>;
>> +	...
>> +};
>
> It's a bit difficult to say much about this, as I have no knowledge
> about mmp.
>
> But I don't quite understand what the pxa988-fb is. Is that some kind of
> virtual device, only used to set up fbdev side? And pxa988-disp is the
> actual hardware device?
>
> If so, I don't really think pxa988-fb should exist in the DT data at
> all, as it's not a hardware device.
Yes, fb is a virtual device for fbdev side.
In our platforms we may use more than one fb for different paths/output 
panel or multi overlays on same path for composition. So we need to 
configure fb settings like which path/overlay/dmafetch it connected to 
for one or more fbdev.
Besides, some more configures like yres_virtual size or fixed fb mem 
reserved rather than allocated which depends on platforms are also 
needed although these features are not upstreamed yet.
So we put fb as a dt node here for these configures.
>
> Why isn't there just one node for pxa988-disp, which would contain all
> the above information?
We have moved out fb/panel from path due to several reasons:
1. To simplify the node. If moved these nodes in, it might be:
disp {
	...
	path1 {
		...
		panel-xxx {
		}
		panel-yyy {
		}
		...
		fb0 {
		}
		fb1 {
		}
	}
	path2 {
		...
		panel-zzz {
		}
		fb3 {
		}
	}
}
Also due to child node type might be different, we could even not 
directly check child of node. The code would be complex.
2. the path of node would be too long and not so common.
e.g. the panel node in dts would be /soc/axi/disp/path1/panel-xxx, and 
in sysfs, node would be /sys/devices/platform/soc/axi/path1/panel-xxx. 
It would be complex and not compatible for platforms when our 
bootloader/user app doing some operations to these nodes.
If we moved them out, we could move fb/panel out of soc directory so the 
node would be /panel-xxx or /fb1 and simpler and compatible.
>
>   Tomi
>
>


-- 
Thanks, -Zhou
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Zhou Zhu]

On 02/10/2014 08:43 PM, Tomi Valkeinen wrote:
> On 14/01/14 13:16, Zhou Zhu wrote:
>> add device tree support for mmp fb/controller
>> the description of DT config is at
>> Documentation/devicetree/bindings/fb/mmp-disp.txt
>>
>> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
>> ---
>>   Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>>   drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>>   drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>>   3 files changed, 235 insertions(+), 58 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
>>
>> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
>> new file mode 100644
>> index 0000000..80702f5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
>> @@ -0,0 +1,60 @@
>> +* Marvell MMP Display (MMP_DISP)
>> +
>> +To config mmp display, 3 parts are required to be set in dts:
>> +1. mmp fb
>> +Required properties:
>> +- compatible: Should be "marvell,<soc>-fb".
>> +- marvell,path: Should be the path this fb connecting to.
>> +- marvell,overlay-id: Should be the id of overlay this fb is on.
>> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.
>> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
>> +turned on.
>> +
>> +2. mmp controller
>> +Required properties:
>> +- compatible: Should be "marvell,<soc>-disp".
>> +- reg: Should be address and length of the register set for this controller.
>> +- interrupts: Should be interrupt of this controller.
>> +
>> +Required sub-node:
>> +- path:
>> +Required properties in this sub-node:
>> +-- marvell,overlay_num: Should be number of overlay this path has.
>
> If that one tells how many overlays there are, maybe "num_overlays"
> would be better.
I will update it in next version.
>
>> +-- marvell,output-type: Should be output-type settings
>> +-- marvell,path-config: Should be path-config settings
>> +-- marvell,link-config: Should be link-config settings
>> +-- marvell,rbswap: Should be rbswap settings
>
> If these terms (output-type, path-config, ...) are straight from the HW
> manual, then fine. But if they are not clear, or are driver specific,
> the values these can have should be documented here.
Yes, it's straight from HW manual.
>
>> +
>> +3. panel
>> +Required properties:
>> +- marvell,path: Should be path that this panel connected to.
>> +- other properties each panel has.
>> +
>> +Examples:
>> +
>> +fb: mmp-fb {
>> +	compatible = "marvell,pxa988-fb";
>> +	marvell,path = <&path1>;
>> +	marvell,overlay-id = <0>;
>> +	marvell,dmafetch-id = <1>;
>> +	marvell,default-pixfmt = <0x108>;
>> +};
>> +
>> +disp: mmp-disp@d420b000 {
>> +	compatible = "marvell,pxa988-disp";
>> +	reg = <0xd420b000 0x1fc>;
>> +	interrupts = <0 41 0x4>;
>> +	path1: mmp-pnpath {
>> +		marvell,overlay-num = <2>;
>> +		marvell,output-type = <0>;
>> +		marvell,path-config = <0x20000000>;
>> +		marvell,link-config = <0x60000001>;
>> +		marvell,rbswap = <0>;
>> +	};
>> +};
>> +
>> +panel: <panel-name> {
>
> How is the panel linked to all this? The nodes above do not refer to the
> panel.
We are making panel refer to path, so when panel dev probe, it could 
register to related path.
The reason we not link from path to panel is our customer sometimes 
asked us to use same image pack (include dts) for different panel types 
in product. We could only add all these panels in dts and detect panel 
dynamically when boot. So moving panel out and making path not link to 
panel but panel link to path would be more straight forward.
>
>> +	...
>> +	marvell,path = <&path1>;
>> +	...
>> +};
>
> It's a bit difficult to say much about this, as I have no knowledge
> about mmp.
>
> But I don't quite understand what the pxa988-fb is. Is that some kind of
> virtual device, only used to set up fbdev side? And pxa988-disp is the
> actual hardware device?
>
> If so, I don't really think pxa988-fb should exist in the DT data at
> all, as it's not a hardware device.
Yes, fb is a virtual device for fbdev side.
In our platforms we may use more than one fb for different paths/output 
panel or multi overlays on same path for composition. So we need to 
configure fb settings like which path/overlay/dmafetch it connected to 
for one or more fbdev.
Besides, some more configures like yres_virtual size or fixed fb mem 
reserved rather than allocated which depends on platforms are also 
needed although these features are not upstreamed yet.
So we put fb as a dt node here for these configures.
>
> Why isn't there just one node for pxa988-disp, which would contain all
> the above information?
We have moved out fb/panel from path due to several reasons:
1. To simplify the node. If moved these nodes in, it might be:
disp {
	...
	path1 {
		...
		panel-xxx {
		}
		panel-yyy {
		}
		...
		fb0 {
		}
		fb1 {
		}
	}
	path2 {
		...
		panel-zzz {
		}
		fb3 {
		}
	}
}
Also due to child node type might be different, we could even not 
directly check child of node. The code would be complex.
2. the path of node would be too long and not so common.
e.g. the panel node in dts would be /soc/axi/disp/path1/panel-xxx, and 
in sysfs, node would be /sys/devices/platform/soc/axi/path1/panel-xxx. 
It would be complex and not compatible for platforms when our 
bootloader/user app doing some operations to these nodes.
If we moved them out, we could move fb/panel out of soc directory so the 
node would be /panel-xxx or /fb1 and simpler and compatible.
>
>   Tomi
>
>


-- 
Thanks, -Zhou

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4349 bytes --]

On 11/02/14 05:27, Zhou Zhu wrote:
> On 02/10/2014 08:43 PM, Tomi Valkeinen wrote:

>> How is the panel linked to all this? The nodes above do not refer to the
>> panel.
> We are making panel refer to path, so when panel dev probe, it could
> register to related path.
> The reason we not link from path to panel is our customer sometimes
> asked us to use same image pack (include dts) for different panel types
> in product. We could only add all these panels in dts and detect panel
> dynamically when boot. So moving panel out and making path not link to
> panel but panel link to path would be more straight forward.

Ok.

>>
>>> +    ...
>>> +    marvell,path = <&path1>;
>>> +    ...
>>> +};
>>
>> It's a bit difficult to say much about this, as I have no knowledge
>> about mmp.
>>
>> But I don't quite understand what the pxa988-fb is. Is that some kind of
>> virtual device, only used to set up fbdev side? And pxa988-disp is the
>> actual hardware device?
>>
>> If so, I don't really think pxa988-fb should exist in the DT data at
>> all, as it's not a hardware device.
> Yes, fb is a virtual device for fbdev side.
> In our platforms we may use more than one fb for different paths/output
> panel or multi overlays on same path for composition. So we need to
> configure fb settings like which path/overlay/dmafetch it connected to
> for one or more fbdev.
> Besides, some more configures like yres_virtual size or fixed fb mem
> reserved rather than allocated which depends on platforms are also
> needed although these features are not upstreamed yet.
> So we put fb as a dt node here for these configures.

Ok.

The device tree data should reflect the hardware. Not the software. So
when thinking what kind of DT data you should have, you should forget
the drivers, and just think on HW terms.

You might want to have a look at CDF (Common Display Framework)
discussions on linux-fbdev list, and the "[PATCHv3 00/41] OMAPDSS: DT
support v3" series I've posted (mostly the .dts parts).

It sounds to me that you'd benefit greatly from the CDF, and even if CDF
is not yet merged (and will probably still take time), I'd very much
recommend trying to create your DT data in such a manner that it would
be in the same direction as what is currently suggested for CDF (or in
the OMAPDSS series).

>> Why isn't there just one node for pxa988-disp, which would contain all
>> the above information?
> We have moved out fb/panel from path due to several reasons:
> 1. To simplify the node. If moved these nodes in, it might be:
> disp {
>     ...
>     path1 {
>         ...
>         panel-xxx {
>         }
>         panel-yyy {
>         }
>         ...
>         fb0 {
>         }
>         fb1 {
>         }
>     }
>     path2 {
>         ...
>         panel-zzz {
>         }
>         fb3 {
>         }
>     }
> }
> Also due to child node type might be different, we could even not
> directly check child of node. The code would be complex.
> 2. the path of node would be too long and not so common.
> e.g. the panel node in dts would be /soc/axi/disp/path1/panel-xxx, and
> in sysfs, node would be /sys/devices/platform/soc/axi/path1/panel-xxx.
> It would be complex and not compatible for platforms when our
> bootloader/user app doing some operations to these nodes.
> If we moved them out, we could move fb/panel out of soc directory so the
> node would be /panel-xxx or /fb1 and simpler and compatible.

I suggest to first think only about the DT data, and create it
correctly. After that you could think how to create compatibility code
in the driver side, so that the legacy sysfs paths are still usable.

The thing with DT data is that it's quite difficult to make big changes
to it later, without writing possibly complex compatibility
functionality. So in my opinion it's worth it to spend a good deal of
time thinking about good DT bindings from the start.

That said, if the driver and hardware in question is for some old SoC,
that's not going to be used on new boards, and the driver is not going
to be used in any future boards, it might be simpler to just make simple
bindings that work for the known set of boards and displays, and be done
with it.

I don't know if that's the case here or not.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4349 bytes --]

On 11/02/14 05:27, Zhou Zhu wrote:
> On 02/10/2014 08:43 PM, Tomi Valkeinen wrote:

>> How is the panel linked to all this? The nodes above do not refer to the
>> panel.
> We are making panel refer to path, so when panel dev probe, it could
> register to related path.
> The reason we not link from path to panel is our customer sometimes
> asked us to use same image pack (include dts) for different panel types
> in product. We could only add all these panels in dts and detect panel
> dynamically when boot. So moving panel out and making path not link to
> panel but panel link to path would be more straight forward.

Ok.

>>
>>> +    ...
>>> +    marvell,path = <&path1>;
>>> +    ...
>>> +};
>>
>> It's a bit difficult to say much about this, as I have no knowledge
>> about mmp.
>>
>> But I don't quite understand what the pxa988-fb is. Is that some kind of
>> virtual device, only used to set up fbdev side? And pxa988-disp is the
>> actual hardware device?
>>
>> If so, I don't really think pxa988-fb should exist in the DT data at
>> all, as it's not a hardware device.
> Yes, fb is a virtual device for fbdev side.
> In our platforms we may use more than one fb for different paths/output
> panel or multi overlays on same path for composition. So we need to
> configure fb settings like which path/overlay/dmafetch it connected to
> for one or more fbdev.
> Besides, some more configures like yres_virtual size or fixed fb mem
> reserved rather than allocated which depends on platforms are also
> needed although these features are not upstreamed yet.
> So we put fb as a dt node here for these configures.

Ok.

The device tree data should reflect the hardware. Not the software. So
when thinking what kind of DT data you should have, you should forget
the drivers, and just think on HW terms.

You might want to have a look at CDF (Common Display Framework)
discussions on linux-fbdev list, and the "[PATCHv3 00/41] OMAPDSS: DT
support v3" series I've posted (mostly the .dts parts).

It sounds to me that you'd benefit greatly from the CDF, and even if CDF
is not yet merged (and will probably still take time), I'd very much
recommend trying to create your DT data in such a manner that it would
be in the same direction as what is currently suggested for CDF (or in
the OMAPDSS series).

>> Why isn't there just one node for pxa988-disp, which would contain all
>> the above information?
> We have moved out fb/panel from path due to several reasons:
> 1. To simplify the node. If moved these nodes in, it might be:
> disp {
>     ...
>     path1 {
>         ...
>         panel-xxx {
>         }
>         panel-yyy {
>         }
>         ...
>         fb0 {
>         }
>         fb1 {
>         }
>     }
>     path2 {
>         ...
>         panel-zzz {
>         }
>         fb3 {
>         }
>     }
> }
> Also due to child node type might be different, we could even not
> directly check child of node. The code would be complex.
> 2. the path of node would be too long and not so common.
> e.g. the panel node in dts would be /soc/axi/disp/path1/panel-xxx, and
> in sysfs, node would be /sys/devices/platform/soc/axi/path1/panel-xxx.
> It would be complex and not compatible for platforms when our
> bootloader/user app doing some operations to these nodes.
> If we moved them out, we could move fb/panel out of soc directory so the
> node would be /panel-xxx or /fb1 and simpler and compatible.

I suggest to first think only about the DT data, and create it
correctly. After that you could think how to create compatibility code
in the driver side, so that the legacy sysfs paths are still usable.

The thing with DT data is that it's quite difficult to make big changes
to it later, without writing possibly complex compatibility
functionality. So in my opinion it's worth it to spend a good deal of
time thinking about good DT bindings from the start.

That said, if the driver and hardware in question is for some old SoC,
that's not going to be used on new boards, and the driver is not going
to be used in any future boards, it might be simpler to just make simple
bindings that work for the known set of boards and displays, and be done
with it.

I don't know if that's the case here or not.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Mark Rutland]

On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
> add device tree support for mmp fb/controller
> the description of DT config is at
> Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>  3 files changed, 235 insertions(+), 58 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> new file mode 100644
> index 0000000..80702f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> @@ -0,0 +1,60 @@
> +* Marvell MMP Display (MMP_DISP)
> +
> +To config mmp display, 3 parts are required to be set in dts:
> +1. mmp fb
> +Required properties:
> +- compatible: Should be "marvell,<soc>-fb".

Please list the precise values and when they should be used. It makes
searching for them _far_ easier.

> +- marvell,path: Should be the path this fb connecting to.

What type is this? The example implies a phandle.

What type of node does this point to?

It's not explained at this point and it's really unclear what this is.

> +- marvell,overlay-id: Should be the id of overlay this fb is on.
> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.

Are these hardware properties?

Is there any documentation which could make this clearer?

> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
> +turned on.

What format is this? The example is useless. If this is a standard
binding, please refer to the binding document.

> +
> +2. mmp controller
> +Required properties:
> +- compatible: Should be "marvell,<soc>-disp".

Please list the precise set of values.

> +- reg: Should be address and length of the register set for this controller.
> +- interrupts: Should be interrupt of this controller.

Just to check: the device only has on interrupt?

> +
> +Required sub-node:
> +- path:
> +Required properties in this sub-node:
> +-- marvell,overlay_num: Should be number of overlay this path has.

s/_/-/ in property names please.

num-overlays would be a clearer name.

> +-- marvell,output-type: Should be output-type settings
> +-- marvell,path-config: Should be path-config settings
> +-- marvell,link-config: Should be link-config settings
> +-- marvell,rbswap: Should be rbswap settings

These are completely opaque to me. Please describe what these are either
in place or in reference to standard bindings.

Is rbswap a boolean value?

[...]

> +       if (!path_np || of_property_read_u32(path_np, "marvell,overlay-num",
> +                               &path_info->output_type) ||
> +                       of_property_read_u32(path_np, "marvell,output-type",
> +                               &path_info->overlay_num)) {

These are reading into the wrong variables.

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Mark Rutland]

On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
> add device tree support for mmp fb/controller
> the description of DT config is at
> Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
> ---
>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>  3 files changed, 235 insertions(+), 58 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> diff --git a/Documentation/devicetree/bindings/fb/mmp-disp.txt b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> new file mode 100644
> index 0000000..80702f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fb/mmp-disp.txt
> @@ -0,0 +1,60 @@
> +* Marvell MMP Display (MMP_DISP)
> +
> +To config mmp display, 3 parts are required to be set in dts:
> +1. mmp fb
> +Required properties:
> +- compatible: Should be "marvell,<soc>-fb".

Please list the precise values and when they should be used. It makes
searching for them _far_ easier.

> +- marvell,path: Should be the path this fb connecting to.

What type is this? The example implies a phandle.

What type of node does this point to?

It's not explained at this point and it's really unclear what this is.

> +- marvell,overlay-id: Should be the id of overlay this fb is on.
> +- marvell,dmafetch-id: Should be the dma fetch id this fb using.

Are these hardware properties?

Is there any documentation which could make this clearer?

> +- marvell,default-pixfmt: Should be the default pixel format when this fb is
> +turned on.

What format is this? The example is useless. If this is a standard
binding, please refer to the binding document.

> +
> +2. mmp controller
> +Required properties:
> +- compatible: Should be "marvell,<soc>-disp".

Please list the precise set of values.

> +- reg: Should be address and length of the register set for this controller.
> +- interrupts: Should be interrupt of this controller.

Just to check: the device only has on interrupt?

> +
> +Required sub-node:
> +- path:
> +Required properties in this sub-node:
> +-- marvell,overlay_num: Should be number of overlay this path has.

s/_/-/ in property names please.

num-overlays would be a clearer name.

> +-- marvell,output-type: Should be output-type settings
> +-- marvell,path-config: Should be path-config settings
> +-- marvell,link-config: Should be link-config settings
> +-- marvell,rbswap: Should be rbswap settings

These are completely opaque to me. Please describe what these are either
in place or in reference to standard bindings.

Is rbswap a boolean value?

[...]

> +       if (!path_np || of_property_read_u32(path_np, "marvell,overlay-num",
> +                               &path_info->output_type) ||
> +                       of_property_read_u32(path_np, "marvell,output-type",
> +                               &path_info->overlay_num)) {

These are reading into the wrong variables.

Cheers,
Mark.

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Zhou Zhu]

Hi, Tomi and Mark,

On Mon, Feb 17, 2014 at 10:37 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>
> On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
> > add device tree support for mmp fb/controller
> > the description of DT config is at
> > Documentation/devicetree/bindings/fb/mmp-disp.txt
> >
> > Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
> >  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
> >  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
> >  3 files changed, 235 insertions(+), 58 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt


Thank you very much for your review! I am trying to update the code
according to your comments.

We have reviewed the dts and removed many software settings and
not-used settings - for example, we unpacked path-config/link-config
and removed some configures that we will never change.
Also we removed fb settings which is considered as software.

Would you please give us some feedbacks if we adjust our dts into follow style?
As there might be big changes on the code structures, I would update
the code after this dts layout is considered as "right".

mmp-disp@d420b000 {
    compatible = "marvell,mmp-disp";
    reg = <0xd420b000 0x1fc>;
    interrupts = <0 41 0x4>;

    internal-connections {
        pipe1: pn-path {
        input = "panel-graphic"; //panel-graphic is the overlay name in spec.
        output = &parallel;
        }
    }
    ports{
        parallel: parallel {
            marvell,rbswap;
            marvell,spi;
        }
    }
    status = "okay";
}

panel-xxx {
    properties;
    connection = &parallel;
}

-- 
Thanks,
-Zhou
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Zhou Zhu]

Hi, Tomi and Mark,

On Mon, Feb 17, 2014 at 10:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
> > add device tree support for mmp fb/controller
> > the description of DT config is at
> > Documentation/devicetree/bindings/fb/mmp-disp.txt
> >
> > Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
> > ---
> >  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
> >  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
> >  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
> >  3 files changed, 235 insertions(+), 58 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt


Thank you very much for your review! I am trying to update the code
according to your comments.

We have reviewed the dts and removed many software settings and
not-used settings - for example, we unpacked path-config/link-config
and removed some configures that we will never change.
Also we removed fb settings which is considered as software.

Would you please give us some feedbacks if we adjust our dts into follow style?
As there might be big changes on the code structures, I would update
the code after this dts layout is considered as "right".

mmp-disp@d420b000 {
    compatible = "marvell,mmp-disp";
    reg = <0xd420b000 0x1fc>;
    interrupts = <0 41 0x4>;

    internal-connections {
        pipe1: pn-path {
        input = "panel-graphic"; //panel-graphic is the overlay name in spec.
        output = &parallel;
        }
    }
    ports{
        parallel: parallel {
            marvell,rbswap;
            marvell,spi;
        }
    }
    status = "okay";
}

panel-xxx {
    properties;
    connection = &parallel;
}

-- 
Thanks,
-Zhou

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]

On 04/03/14 13:28, Zhou Zhu wrote:
> Hi, Tomi and Mark,
> 
> On Mon, Feb 17, 2014 at 10:37 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>>
>> On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
>>> add device tree support for mmp fb/controller
>>> the description of DT config is at
>>> Documentation/devicetree/bindings/fb/mmp-disp.txt
>>>
>>> Signed-off-by: Zhou Zhu <zzhu3-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>>> ---
>>>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>>>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>>>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>>>  3 files changed, 235 insertions(+), 58 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> 
> Thank you very much for your review! I am trying to update the code
> according to your comments.
> 
> We have reviewed the dts and removed many software settings and
> not-used settings - for example, we unpacked path-config/link-config
> and removed some configures that we will never change.
> Also we removed fb settings which is considered as software.
> 
> Would you please give us some feedbacks if we adjust our dts into follow style?
> As there might be big changes on the code structures, I would update
> the code after this dts layout is considered as "right".
> 
> mmp-disp@d420b000 {
>     compatible = "marvell,mmp-disp";
>     reg = <0xd420b000 0x1fc>;
>     interrupts = <0 41 0x4>;
> 
>     internal-connections {
>         pipe1: pn-path {
>         input = "panel-graphic"; //panel-graphic is the overlay name in spec.
>         output = &parallel;
>         }
>     }
>     ports{
>         parallel: parallel {
>             marvell,rbswap;
>             marvell,spi;
>         }
>     }
>     status = "okay";
> }
> 
> panel-xxx {
>     properties;
>     connection = &parallel;
> }

I would recommend using the same style that is used in the OMAP DSS, imx
drm and exynos patches that are introducing DT support. They use ports
and endpoints as defined in:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/media/video-interfaces.txt

with the addition of a simplified endpoint format, where the 'port' node
is not required.

To give an idea what it could look like, I've modified your example
above. It's just a rough example, you should study and think how it best
fits for mmp.

You could use the same format for the internal connections also, but I
didn't touch that. I think internal connections can as well be
configured separately, in a custom format, because there's no need for
external components to connect to the internal links.

mmp-disp@d420b000 {
    compatible = "marvell,mmp-disp";
    reg = <0xd420b000 0x1fc>;
    interrupts = <0 41 0x4>;

    internal-connections {
        pipe1: pn-path {
        input = "panel-graphic";
        output = &disp_out;
        }
    }

	disp_out: endpoint {
		remote-endpoint = <&panel_in>;
		marvell,rbswap;
		marvell,spi;
	};

    status = "okay";
}

panel-xxx {
	properties;

	panel_in: endpoint {
		remote-endpoint = <&disp_out>;
	};
};

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3258 bytes --]

On 04/03/14 13:28, Zhou Zhu wrote:
> Hi, Tomi and Mark,
> 
> On Mon, Feb 17, 2014 at 10:37 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>
>> On Tue, Jan 14, 2014 at 11:16:24AM +0000, Zhou Zhu wrote:
>>> add device tree support for mmp fb/controller
>>> the description of DT config is at
>>> Documentation/devicetree/bindings/fb/mmp-disp.txt
>>>
>>> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
>>> ---
>>>  Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>>>  drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>>>  drivers/video/mmp/hw/mmp_ctrl.c                   |  160 ++++++++++++++++-----
>>>  3 files changed, 235 insertions(+), 58 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
> 
> 
> Thank you very much for your review! I am trying to update the code
> according to your comments.
> 
> We have reviewed the dts and removed many software settings and
> not-used settings - for example, we unpacked path-config/link-config
> and removed some configures that we will never change.
> Also we removed fb settings which is considered as software.
> 
> Would you please give us some feedbacks if we adjust our dts into follow style?
> As there might be big changes on the code structures, I would update
> the code after this dts layout is considered as "right".
> 
> mmp-disp@d420b000 {
>     compatible = "marvell,mmp-disp";
>     reg = <0xd420b000 0x1fc>;
>     interrupts = <0 41 0x4>;
> 
>     internal-connections {
>         pipe1: pn-path {
>         input = "panel-graphic"; //panel-graphic is the overlay name in spec.
>         output = &parallel;
>         }
>     }
>     ports{
>         parallel: parallel {
>             marvell,rbswap;
>             marvell,spi;
>         }
>     }
>     status = "okay";
> }
> 
> panel-xxx {
>     properties;
>     connection = &parallel;
> }

I would recommend using the same style that is used in the OMAP DSS, imx
drm and exynos patches that are introducing DT support. They use ports
and endpoints as defined in:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/media/video-interfaces.txt

with the addition of a simplified endpoint format, where the 'port' node
is not required.

To give an idea what it could look like, I've modified your example
above. It's just a rough example, you should study and think how it best
fits for mmp.

You could use the same format for the internal connections also, but I
didn't touch that. I think internal connections can as well be
configured separately, in a custom format, because there's no need for
external components to connect to the internal links.

mmp-disp@d420b000 {
    compatible = "marvell,mmp-disp";
    reg = <0xd420b000 0x1fc>;
    interrupts = <0 41 0x4>;

    internal-connections {
        pipe1: pn-path {
        input = "panel-graphic";
        output = &disp_out;
        }
    }

	disp_out: endpoint {
		remote-endpoint = <&panel_in>;
		marvell,rbswap;
		marvell,spi;
	};

    status = "okay";
}

panel-xxx {
	properties;

	panel_in: endpoint {
		remote-endpoint = <&disp_out>;
	};
};

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Zhou Zhu]

Hi, Tomi,

On 03/05/2014 05:33 PM, Tomi Valkeinen wrote:
> I would recommend using the same style that is used in the OMAP DSS, imx
> drm and exynos patches that are introducing DT support. They use ports
> and endpoints as defined in:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/media/video-interfaces.txt
>
> with the addition of a simplified endpoint format, where the 'port' node
> is not required.
>
> To give an idea what it could look like, I've modified your example
> above. It's just a rough example, you should study and think how it best
> fits for mmp.
>
> You could use the same format for the internal connections also, but I
> didn't touch that. I think internal connections can as well be
> configured separately, in a custom format, because there's no need for
> external components to connect to the internal links.
>
> mmp-disp@d420b000 {
>      compatible = "marvell,mmp-disp";
>      reg = <0xd420b000 0x1fc>;
>      interrupts = <0 41 0x4>;
>
>      internal-connections {
>          pipe1: pn-path {
>          input = "panel-graphic";
>          output = &disp_out;
>          }
>      }
>
> 	disp_out: endpoint {
> 		remote-endpoint = <&panel_in>;
> 		marvell,rbswap;
> 		marvell,spi;
> 	};
>
>      status = "okay";
> }
>
> panel-xxx {
> 	properties;
>
> 	panel_in: endpoint {
> 		remote-endpoint = <&disp_out>;
> 	};
> };
>
>   Tomi
>
>
Thank you! I will update patches according to this layout later.


-- 
Thanks,
-Zhou
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/4] video: mmp: add device tree support

[Zhou Zhu]

Hi, Tomi,

On 03/05/2014 05:33 PM, Tomi Valkeinen wrote:
> I would recommend using the same style that is used in the OMAP DSS, imx
> drm and exynos patches that are introducing DT support. They use ports
> and endpoints as defined in:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/media/video-interfaces.txt
>
> with the addition of a simplified endpoint format, where the 'port' node
> is not required.
>
> To give an idea what it could look like, I've modified your example
> above. It's just a rough example, you should study and think how it best
> fits for mmp.
>
> You could use the same format for the internal connections also, but I
> didn't touch that. I think internal connections can as well be
> configured separately, in a custom format, because there's no need for
> external components to connect to the internal links.
>
> mmp-disp@d420b000 {
>      compatible = "marvell,mmp-disp";
>      reg = <0xd420b000 0x1fc>;
>      interrupts = <0 41 0x4>;
>
>      internal-connections {
>          pipe1: pn-path {
>          input = "panel-graphic";
>          output = &disp_out;
>          }
>      }
>
> 	disp_out: endpoint {
> 		remote-endpoint = <&panel_in>;
> 		marvell,rbswap;
> 		marvell,spi;
> 	};
>
>      status = "okay";
> }
>
> panel-xxx {
> 	properties;
>
> 	panel_in: endpoint {
> 		remote-endpoint = <&disp_out>;
> 	};
> };
>
>   Tomi
>
>
Thank you! I will update patches according to this layout later.


-- 
Thanks,
-Zhou

Re: [PATCH v2 0/4] video: mmp: add device tree suppport

[Zhou Zhu]

Hi, Tomi,
Looks there's no more comment now.
Would you please help to review this patch set and apply it also for 
3.14 if possible?
Thank you very much!

On 01/14/2014 07:16 PM, Zhou Zhu wrote:
> This patch set is to add device tree support for mmp_disp.
> patch 1/2 are to remove clk_name configure in mach_info before.
> patch 3/4 are to add device tree support.
>
> change of v2:
> add patch to remove config of clk_name in mach_info
> use phandle to get path.
> runtime decision of dt usage.
> clean up to use node name directly.
>
> Zhou Zhu (4):
>    arm: mmp: remove not used disp clk_name in ttc_dkb
>    video: mmp: no need to get clk_name from mach_info
>    video: mmp: add devm_mmp_get_path_by_phandle for DT
>    video: mmp: add device tree support
>
>   Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>   arch/arm/mach-mmp/ttc_dkb.c                       |    1 -
>   drivers/video/mmp/core.c                          |   35 +++++
>   drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>   drivers/video/mmp/hw/mmp_ctrl.c                   |  154 ++++++++++++++++-----
>   include/video/mmp_disp.h                          |    3 +-
>   6 files changed, 268 insertions(+), 58 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
>


-- 
Thanks, -Zhou
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] video: mmp: add device tree suppport

[Zhou Zhu]

Hi, Tomi,
Looks there's no more comment now.
Would you please help to review this patch set and apply it also for 
3.14 if possible?
Thank you very much!

On 01/14/2014 07:16 PM, Zhou Zhu wrote:
> This patch set is to add device tree support for mmp_disp.
> patch 1/2 are to remove clk_name configure in mach_info before.
> patch 3/4 are to add device tree support.
>
> change of v2:
> add patch to remove config of clk_name in mach_info
> use phandle to get path.
> runtime decision of dt usage.
> clean up to use node name directly.
>
> Zhou Zhu (4):
>    arm: mmp: remove not used disp clk_name in ttc_dkb
>    video: mmp: no need to get clk_name from mach_info
>    video: mmp: add devm_mmp_get_path_by_phandle for DT
>    video: mmp: add device tree support
>
>   Documentation/devicetree/bindings/fb/mmp-disp.txt |   60 ++++++++
>   arch/arm/mach-mmp/ttc_dkb.c                       |    1 -
>   drivers/video/mmp/core.c                          |   35 +++++
>   drivers/video/mmp/fb/mmpfb.c                      |   73 ++++++----
>   drivers/video/mmp/hw/mmp_ctrl.c                   |  154 ++++++++++++++++-----
>   include/video/mmp_disp.h                          |    3 +-
>   6 files changed, 268 insertions(+), 58 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt
>


-- 
Thanks, -Zhou