Re: [linux-sunxi] [PATCH 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen

On Wed, Nov 12, 2014 at 11:39 PM, Julian Calaby [off-list ref] wrote:

Hi Hans,

On Thu, Nov 13, 2014 at 9:08 AM, Hans de Goede [off-list ref] wrote:

quoted

Update simplefb to support the new preferred location for simplefb dt nodes
under /chosen.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/video/fbdev/simplefb.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index cd96edd..be7d288 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c

@@ -27,6 +27,7 @@
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/clk-provider.h>
+#include <linux/of_platform.h>

 static struct fb_fix_screeninfo simplefb_fix = {
        .id             = "simple",

@@ -385,7 +386,37 @@ static struct platform_driver simplefb_driver = {
        .probe = simplefb_probe,
        .remove = simplefb_remove,
 };
-module_platform_driver(simplefb_driver);
+
+static int __init simplefb_init(void)
+{
+       int i, ret;
+       char name[16];
+       struct device_node *np;
+
+       ret = platform_driver_register(&simplefb_driver);
+       if (ret)
+               return ret;
+
+       for (i = 0; ; i++) {
+               snprintf(name, sizeof(name), "framebuffer%d", i);

This smells like an infinite loop: we can be pretty sure that no
hardware will ever exist with more than 9999 (I think?) framebuffers,
however if that ever happens this'll loop until it runs out of RAM.
Maybe add a suitably high limit to the for loop?

Unlikely, but the loop is wrong anyway. The loop should be:

for_each_child_of_node(of_chosen, child)
        if (of_device_is_compatible(child, "simple-framebuffer");
                of_platform_device_create(np, NULL, NULL);

Then make the probe hook choose an appropriate FB number. It looks
like you structured the code the way you did to get the framebuffers
to register in a particular order, and therefore implicitly get the
right numbers, but that is a fragile way to go about it.

Using /aliases really is the right way to get specific framebuffer
numbers. We already use it for UARTs, so we should do the same here.
Use of_alias_get_id(, "framebuffer") to get the framebuffer number.

g.