Re: [PATCH] video: s3c-fb: Add device tree support

Hi Sylwester,

On 14 January 2012 03:00, Sylwester Nawrocki [off-list ref] wrote:

On 01/13/2012 10:03 PM, Sylwester Nawrocki wrote:

quoted

On 01/09/2012 09:01 PM, Thomas Abraham wrote:

quoted

+    for (idx = 0; idx<   nr_gpios; idx++) {
+            gpio = of_get_gpio(dev->of_node, idx);
+            if (!gpio_is_valid(gpio)) {
+                    dev_err(dev, "invalid gpio[%d]: %d\n", idx, gpio);
+                    return -EINVAL;
+            }
+
+            if (!request)
+                    continue;
+
+            ret = gpio_request(gpio, "fimd");

Is it how it normally is supposed to be done, i.e. configuring a gpio
_before_ it has been requested ? of_get_gpio() indirectly touches the
gpio controller and gpio_request() doesn't seem to serve its purpose
in this case, i.e. if there is situation like:

driver A                        driver B

of_get_gpio(nodeA, gpioA);
                                 of_get_gpio(nodeB, gpioA);
gpio_request(gpioA);
                                 gpio_request(gpioB);

                                              s/B/A

quoted

driver B will end up with configuration of gpioA from nodeA, not from
nodeB.

ugh, I put it wrong, it should instead read:

"driver A will end up with configuration of gpioA from nodeB, not from
nodeA."

quoted

As there are few drivers doing that I must be missing something,
not sure what..
I realize the GPIO number needs to be known in order for a GPIO to be
requested. Shouldn't of_get_gpio() be extended to allow locking gpio
controller's module and marking a GPIO as requested in advance ?

quoted

+            if (ret) {
+                    dev_err(dev, "gpio [%d] request failed\n", gpio);
+                    goto gpio_free;
+            }
+            sfb->gpios[idx] = gpio;
+    }
+    return 0;
+
+gpio_free:
+    while (--idx>= 0)
+            gpio_free(sfb->gpios[idx]);
+    return ret;
+}
+
+static void s3c_fb_dt_free_gpios(struct s3c_fb *sfb)
+{
+    unsigned int idx, nr_gpio;
+
+    nr_gpio = sfb->pdata->win[0]->max_bpp + 4;
+    for (idx = 0; idx<   nr_gpio; idx++)
+            gpio_free(sfb->gpios[idx]);
+}

Yes, I agree that there is a problem here. DT bindings for
pinmux/pinconfig would solve this. But the sequence of of_get_gpio()
and gpio_request() in the above code will stay even when using the dt
bindings for pinmux/pinconfig. There will be only incremental changes
required to adapt to dt bindings for pinmux/pinconfig.

Thanks,
Thomas.