video: s3c-fb: Add device tree support

+Required properties:

+- gpios: The gpios used to interface with the external LCD panel.

Quoted 3 lines · Thomas Abraham

+Required properties:

+- gpios: The gpios used to interface with the external LCD panel.

This isn't terribly informative for the user - which GPIOs and in what order are they specified? ?Looking at the code it looks like pin mux configuration so just some wording saying it's a list of pins to be used for the framebuffer function.

On 10 January 2012 10:22, Mark Brown

Quoted 4 lines

This isn't terribly informative for the user - which GPIOs and in what order are they specified? ?Looking at the code it looks like pin mux configuration so just some wording saying it's a list of pins to be used for the framebuffer function.

Ok. I will modify the sentence to mean that it is list of gpios used by framebuffer controller for data/timing interface with the lcd panel. Thanks for your review.

-----Original Message-----
From: Thomas Abraham [mailto:thomas.abraham at linaro.org]
Sent: Tuesday, January 10, 2012 5:02 AM
To: linux-fbdev at vger.kernel.org; devicetree-discuss at lists.ozlabs.org
Cc: FlorianSchandinat at gmx.de; grant.likely at secretlab.ca; rob.herring at calxeda.com; linux-arm- kernel at lists.infradead.org; linux-samsung-soc at vger.kernel.org; kgene.kim at samsung.com; jg1.han at samsung.com; patches at linaro.org
Subject: [PATCH] video: s3c-fb: Add device tree support

Add device tree based discovery support for Samsung's display controller.

Cc Grant Likely <[email protected]>

Cc Rob Herring <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
 .../devicetree/bindings/fb/samsung-fb.txt          |  103 ++++++++++
 drivers/video/s3c-fb.c                             |  209 +++++++++++++++++++-
 2 files changed, 304 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fb/samsung-fb.txt

[...]

+- samsung,fimd-bpp: Specifies the bits per pixel. Two values should be

Documentation/devicetree/bindings/fb/samsung-fb.txt new file +5 -0

+  specified in the following order.
+    - max-bpp: maximum required bpp for the overlay.
+    - default-bpp: bpp supported by the overlay.
[...]
+		fimd-overlay0 {
+			samsung,fimd-htiming = <64 16 48 1024>;

Hi, Thomas.

Quoted 31 lines

-----Original Message-----
From: Thomas Abraham [mailto:thomas.abraham at linaro.org]
Sent: Tuesday, January 10, 2012 5:02 AM
To: linux-fbdev at vger.kernel.org; devicetree-discuss at lists.ozlabs.org
Cc: FlorianSchandinat at gmx.de; grant.likely at secretlab.ca; rob.herring at calxeda.com; linux-arm- kernel at lists.infradead.org; linux-samsung-soc at vger.kernel.org; kgene.kim at samsung.com; jg1.han at samsung.com; patches at linaro.org
Subject: [PATCH] video: s3c-fb: Add device tree support

Add device tree based discovery support for Samsung's display controller.

Cc Grant Likely <[email protected]>

Cc Rob Herring <[email protected]>

Signed-off-by Thomas Abraham <[email protected]>

---
?.../devicetree/bindings/fb/samsung-fb.txt ? ? ? ? ?| ?103 ++++++++++ ?drivers/video/s3c-fb.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ?209 +++++++++++++++++++- ?2 files changed, 304 insertions(+), 8 deletions(-)
?create mode 100644 Documentation/devicetree/bindings/fb/samsung-fb.txt

[...]

+- samsung,fimd-bpp: Specifies the bits per pixel. Two values should be
+ ?specified in the following order.
+ ? ?- max-bpp: maximum required bpp for the overlay.
+ ? ?- default-bpp: bpp supported by the overlay.
[...]
+ ? ? ? ? ? ? fimd-overlay0 {
+ ? ? ? ? ? ? ? ? ? ? samsung,fimd-htiming = <64 16 48 1024>;
+ ? ? ? ? ? ? ? ? ? ? samsung,fimd-vtiming = <64 16 3 600>;
+ ? ? ? ? ? ? ? ? ? ? samsung,fimd-bpp = <24 32>;

Quoted 8 lines

+ ? ? ? ? ? ? };
+
+ ? ? ? ? ? ? fimd-overlay1 {
+ ? ? ? ? ? ? ? ? ? ? samsung,fimd-htiming = <64 16 48 200>;
+ ? ? ? ? ? ? ? ? ? ? samsung,fimd-vtiming = <64 16 3 100>;
+ ? ? ? ? ? ? ? ? ? ? samsung,fimd-bpp = <16 32>;
+ ? ? ? ? ? ? };
+ ? ? };

In the above 'samsung-fb.txt', the order is defined as max-bpp and default-bpp. However, <24 32> and <16 32> seems to be wrong.
It should be <32 24> and <32 16>, respectively.
Max bpp would be 32bpp in s3c-fb.

Best regards,
Jingoo Han

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at ?http://vger.kernel.org/majordomo-info.html

drivers/video/s3c-fb.c +22 -0

+		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]);
+}

drivers/video/s3c-fb.c static void s3c_fb_clear_win(struct s3c_fb *sfb, int win) · line 1344

Quoted 12 lines · Thomas Abraham

drivers/video/s3c-fb.c +11 -0

+	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);

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

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 22 lines

drivers/video/s3c-fb.c +22 -0

+		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]);
+}

Quoted 19 lines · Sylwester Nawrocki

Review

Quoted 12 lines · Thomas Abraham

+ ? ?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;
+ ? ? ? ? ? ?}

(quoted diff) +5 -0

+
+ ? ? ? ? ? ?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:

Quoted 6 lines

driver A ? ? ? ? ? ? ? ? ? ? ? ?driver B

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

? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? s/B/A

Quoted 3 lines

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 20 lines

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 14 lines

+ ? ? ? ? ? ?if (ret) {
+ ? ? ? ? ? ? ? ? ? ?dev_err(dev, "gpio [%d] request failed\n", gpio);
+ ? ? ? ? ? ? ? ? ? ?goto gpio_free;
+ ? ? ? ? ? ?}
+ ? ? ? ? ? ?sfb->gpios[idx] = gpio;
+ ? ?}
+ ? ?return 0;

drivers/video/s3c-fb.c +7 -0

+
+gpio_free:
+ ? ?while (--idx>= 0)
+ ? ? ? ? ? ?gpio_free(sfb->gpios[idx]);
+ ? ?return ret;
+}
+

Quoted 8 lines

drivers/video/s3c-fb.c static void s3c_fb_clear_win(struct s3c_fb *sfb, int win) · line 1366

Quoted 8 lines

drivers/video/s3c-fb.c +8 -0

+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]);
+}