Re: [PATCH] video: mmp: add device tree support

Sascha/Jingoo,
Thank you for your review!

On 01/09/2014 03:43 PM, Jingoo Han wrote:

On Thursday, January 09, 2014 4:32 PM, Sascha Hauer wrote:

quoted

On Thu, Jan 09, 2014 at 01:13:14PM +0800, Zhou Zhu wrote:

quoted

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

Signed-off-by: Zhou Zhu <redacted>
---
  Documentation/devicetree/bindings/fb/mmp-disp.txt |   71 ++++++++++++
  drivers/video/mmp/fb/mmpfb.c                      |   71 ++++++++----
  drivers/video/mmp/hw/mmp_ctrl.c                   |  120 ++++++++++++++++-----
  3 files changed, 217 insertions(+), 45 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/fb/mmp-disp.txt

[...]

quoted

+fb: fb {
+	compatible = "marvell,mmp-fb";

This compatible should have the specific SoC name in it, not just
'mmp'. Otherwise you can't properly distinguish between this version and
future versions of the mmp core.

We are using a same display IP for all mmp serial SoCs, and there would 
be inside register to get version. So I am planning put same compatible 
here for all SoCs using this IP.
Would it be ok if I update compatible to "marvell,mmpdcx-fb"? "mmpdcx" 
is the IP name.

 >
 >> +	marvell,fb-name = "mmp_fb";
 >> +	marvell,path-name = "mmp_pnpath";
 >
 > You're not going to use this string to reference to another node, do
 > you? We have phandles for this.
 >
I will update it in v2.

 >> +	marvell,overlay-id = <0>;
 >> +	marvell,dmafetch-id = <1>;
 >> +	marvell,default-pixfmt = <0x108>;
 >> +};
 >> +
 >> +disp: disp@d420b000 {
 >> +	compatible = "marvell,mmp-disp";
 >> +	reg = <0xd420b000 0x1fc>;
 >> +	interrupts = <0 41 0x4>;
 >> +	marvell,disp-name = "mmp_disp";
 >> +	marvell,path-num = <1>;
 >> +	marvell,clk-name = "LCDCIHCLK";
 >
 > Don't pass clk names like this. We have a documented clock binding, use
 > it.
 >
The patches to add dt support in common clk in our platforms are not 
upstreamed yet. As there's only one clock in this device, could I remove 
clock name related codes and direct use: devm_clk_get(dev, NULL)?

quoted

quoted

+#ifdef CONFIG_OF
+	struct device_node *np;
+#else
  	struct mmp_buffer_driver_mach_info *mi;
+#endif
  	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, dmafetch_id;
+	const char *path_name;

  	/* 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;
+
+#ifdef CONFIG_OF

Just because your kernel build does have CONFIG_OF enabled doesn't mean
it's actually started with a devicetree. You need to make a runtime
decision, not compile time.

Yes, right.
As Sascha Hauer said, you need to make a runtime decision,
instead of compile time. Please keep the same binary for
both cases (CONFIG_OF is 'enabled' and 'disabled').

For example,

	if (pdev->dev.of_node) {
		// DT code
	} else {
		// Non-DT code
	}

Thank you for your suggestion. I will update the code to dynamic in v2.

-- 
Thanks, -Zhou