[PATCH v2 0/5] simplefb: Add clock support to simplefb

[PATCH v2 0/5] simplefb: Add clock support to simplefb

[Hans de Goede]

Hi Tomi,

Here is v2 of the patch-set to add clocks support to simplefb.

Changes in v2:
-Added "simplefb: Add simplefb MAINTAINERS entry" patch

If you've followed the discussion sofar, then you know that not everyone
agrees 100% with this. The primary concern being that this will turn simplefb
into not-so-simplefb.

However no-one has been able to come up with a workable alternative solution.

We've asked input from Mike Turquette, the clk maintainer, and he agrees
that the solution in this patch-set is the right way to deal with clocks for
simplefb, and he has given his Reviewed-by for the dt-bindings patch.

As for the simplefb code becoming to complex for this, I noticed that
simplefb currently does not have an active maintainer. I've discussed this
with Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>, the original author of simplefb,
and with his permisson I'm picking up maintainership of simplefb.

And as the new maintainer of simplefb I agree that the approach in this
patch-set is the best way forward, and I've added my Reviewed-by to Luc's
patches adding the clocks property support. I fully commit myself to
actively maintain simplefb for the foreseeable future.

I believe that having the acks of both the clk and simplefb maintainer
is more then good enough to get this set merged despite not everyone
agreeing 100% with it (note some people have expressed concerns, but no one
has actually nacked it).

Therefor I would like to ask you to merge this set for 3.19, or if there is
still time for 3.18.

Thanks & Regards,

Hans

[PATCH v2 1/5] simplefb: Add simplefb MAINTAINERS entry

[Hans de Goede]

During the discussion about adding clock handling code to simplefb, it became
clear that simplefb currently does not have an active maintainer.

I've discussed this with Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>, the original
author of simplefb, and with his permisson I'm picking up maintainership of
simplefb.

Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3705430..01a5fd8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8289,6 +8289,13 @@ F:	drivers/media/usb/siano/
 F:	drivers/media/usb/siano/
 F:	drivers/media/mmc/siano/
 
+SIMPLEFB FB DRIVER
+M:	Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
+L:	linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/video/simple-framebuffer.txt
+F:	drivers/video/fbdev/simplefb.c
+
 SH_VEU V4L2 MEM2MEM DRIVER
 L:	linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
 S:	Orphan
-- 
2.1.0

[PATCH v2 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Hans de Goede]

A simple-framebuffer node represents a framebuffer setup by the firmware /
bootloader. Such a framebuffer may have a number of clocks in use, add a
property to communicate this to the OS.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 70c26f3..e75478e 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -14,6 +14,9 @@ Required properties:
   - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
   - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
 
+Optional properties:
+- clocks : List of clocks used by the framebuffer
+
 Example:
 
 	framebuffer {
-- 
2.1.0

Re: [PATCH v2 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Rob Herring]

On Fri, Oct 3, 2014 at 6:52 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> A simple-framebuffer node represents a framebuffer setup by the firmware /
> bootloader. Such a framebuffer may have a number of clocks in use, add a
> property to communicate this to the OS.
>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 70c26f3..e75478e 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -14,6 +14,9 @@ Required properties:
>    - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>    - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>
> +Optional properties:
> +- clocks : List of clocks used by the framebuffer

A simple framebuffer represents a memory region. So now you are saying
this memory region has a clock. That does not make sense and the
description of simple framebuffer is no longer correct.

I assume you are trying to work around the clock framework turning off
clocks on you. If you bothered to describe the clock such that it can
be turned off by the kernel, you should then describe it's actual
connection to the hardware.

Furthermore, when you do add actual driver(s) for the hardware, that
driver will not be able to turn off the clocks because the simplefb
has forced them on. I can see the cases where you want to use
simple-framebuffer early for splash screen or something before the
real driver is up and use it to provide the default video mode for the
real driver.

Rob

Re: [PATCH v2 2/5] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Hans de Goede]

Hi,

On 10/03/2014 03:20 PM, Rob Herring wrote:
> On Fri, Oct 3, 2014 at 6:52 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> A simple-framebuffer node represents a framebuffer setup by the firmware /
>> bootloader. Such a framebuffer may have a number of clocks in use, add a
>> property to communicate this to the OS.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Reviewed-by: Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/video/simple-framebuffer.txt | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 70c26f3..e75478e 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -14,6 +14,9 @@ Required properties:
>>    - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
>>    - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>>
>> +Optional properties:
>> +- clocks : List of clocks used by the framebuffer
> 
> A simple framebuffer represents a memory region. So now you are saying
> this memory region has a clock. That does not make sense and the
> description of simple framebuffer is no longer correct.

Good point, I'll do a v3 updating the description changing it to:

"A simple frame-buffer describes a frame-buffer setup by firmware or
the bootloader, with the assumption that the display hardware has already
been set up to scan out from the memory pointed to by the ref property."

> I assume you are trying to work around the clock framework turning off
> clocks on you. If you bothered to describe the clock such that it can
> be turned off by the kernel, you should then describe it's actual
> connection to the hardware.

In the parts of devicetree where we actually describe the hardware yes,
for the simplefb virtual device it suffices to simple list the used
clocks. Note that a display pipeline often consists of multiple hw-blocks,
so there is not 1:1 mapping between the involved hardware blocks and
the simplefb. The blocks (and clocks) used my even differ between boots
depending on which video output the firmware / bootloader has setup.

> Furthermore, when you do add actual driver(s) for the hardware, that
> driver will not be able to turn off the clocks because the simplefb
> has forced them on. I can see the cases where you want to use
> simple-framebuffer early for splash screen or something before the
> real driver is up and use it to provide the default video mode for the
> real driver.

When we get a real driver, that will (and must) unregister simplefb, at
which point simplefb will release the clocks, and from then on that real
driver can turn off the clocks just fine.

Regards,

Hans

[PATCH v2 3/5] simplefb: formalize pseudo palette handling

[Hans de Goede]

From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>

Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: drop unnecessary void * cast]
Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/video/fbdev/simplefb.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 210f3a0..ec112c1 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -41,6 +41,8 @@ static struct fb_var_screeninfo simplefb_var = {
 	.vmode		= FB_VMODE_NONINTERLACED,
 };
 
+#define PSEUDO_PALETTE_SIZE 16
+
 static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 			      u_int transp, struct fb_info *info)
 {
@@ -50,7 +52,7 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
 	u32 cb = blue >> (16 - info->var.blue.length);
 	u32 value;
 
-	if (regno >= 16)
+	if (regno >= PSEUDO_PALETTE_SIZE)
 		return -EINVAL;
 
 	value = (cr << info->var.red.offset) |
@@ -163,11 +165,16 @@ static int simplefb_parse_pd(struct platform_device *pdev,
 	return 0;
 }
 
+struct simplefb_par {
+	u32 palette[PSEUDO_PALETTE_SIZE];
+};
+
 static int simplefb_probe(struct platform_device *pdev)
 {
 	int ret;
 	struct simplefb_params params;
 	struct fb_info *info;
+	struct simplefb_par *par;
 	struct resource *mem;
 
 	if (fb_get_options("simplefb", NULL))
@@ -188,11 +195,13 @@ static int simplefb_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
+	info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev);
 	if (!info)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, info);
 
+	par = info->par;
+
 	info->fix = simplefb_fix;
 	info->fix.smem_start = mem->start;
 	info->fix.smem_len = resource_size(mem);
@@ -225,7 +234,7 @@ static int simplefb_probe(struct platform_device *pdev)
 		framebuffer_release(info);
 		return -ENODEV;
 	}
-	info->pseudo_palette = (void *)(info + 1);
+	info->pseudo_palette = par->palette;
 
 	dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
 			     info->fix.smem_start, info->fix.smem_len,
-- 
2.1.0

[PATCH v2 4/5] simplefb: add goto error path to probe

[Hans de Goede]

From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>

Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/video/fbdev/simplefb.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index ec112c1..b7d5c08 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
 
 	info->apertures = alloc_apertures(1);
 	if (!info->apertures) {
-		framebuffer_release(info);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto error_fb_release;
 	}
 	info->apertures->ranges[0].base = info->fix.smem_start;
 	info->apertures->ranges[0].size = info->fix.smem_len;
@@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
 	info->screen_base = ioremap_wc(info->fix.smem_start,
 				       info->fix.smem_len);
 	if (!info->screen_base) {
-		framebuffer_release(info);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto error_fb_release;
 	}
 	info->pseudo_palette = par->palette;
 
@@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
 	ret = register_framebuffer(info);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
-		iounmap(info->screen_base);
-		framebuffer_release(info);
-		return ret;
+		goto error_unmap;
 	}
 
 	dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
 
 	return 0;
+
+ error_unmap:
+	iounmap(info->screen_base);
+
+ error_fb_release:
+	framebuffer_release(info);
+
+	return ret;
 }
 
 static int simplefb_remove(struct platform_device *pdev)
-- 
2.1.0

[PATCH v2 5/5] simplefb: add clock handling code

[Hans de Goede]

From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>

This claims and enables clocks listed in the simple framebuffer dt node.
This is needed so that the display engine, in case the required clocks
are known by the kernel code and are described in the dt, will remain
properly enabled.

Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
[hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: drop dev_err on kzalloc failure]
Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index b7d5c08..f329cc1 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
+#include <linux/clk-provider.h>
 
 static struct fb_fix_screeninfo simplefb_fix = {
 	.id		= "simple",
@@ -165,8 +166,98 @@ static int simplefb_parse_pd(struct platform_device *pdev,
 	return 0;
 }
 
+/*
+ * Clock handling code.
+ *
+ * Here we handle the clocks property of our "simple-framebuffer" dt node.
+ * This is necessary so that we can make sure that any clocks needed by
+ * the display engine that the bootloader set up for us (and for which it
+ * provided a simplefb dt node), stay up, for the life of the simplefb
+ * driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the clocks.
+ */
+struct simplefb_clock {
+	struct list_head list;
+	struct clk *clock;
+};
+
+/*
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the clock definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+static void
+simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
+{
+	struct device_node *np = pdev->dev.of_node;
+	int clock_count, i;
+
+	INIT_LIST_HEAD(list);
+
+	if (dev_get_platdata(&pdev->dev) || !np)
+		return;
+
+	clock_count = of_clk_get_parent_count(np);
+	for (i = 0; i < clock_count; i++) {
+		struct simplefb_clock *entry;
+		struct clk *clock = of_clk_get(np, i);
+		int ret;
+
+		if (IS_ERR(clock)) {
+			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
+			       __func__, i, PTR_ERR(clock));
+			continue;
+		}
+
+		ret = clk_prepare_enable(clock);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"%s: failed to enable clock %d: %d\n",
+			       __func__, i, ret);
+			clk_put(clock);
+			continue;
+		}
+
+		entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
+		if (!entry) {
+			clk_disable_unprepare(clock);
+			clk_put(clock);
+			continue;
+		}
+
+		entry->clock = clock;
+		/*
+		 * add to the front of the list, so we disable clocks in the
+		 * correct order.
+		 */
+		list_add(&entry->list, list);
+	}
+}
+
+static void
+simplefb_clocks_destroy(struct list_head *list)
+{
+	struct list_head *pos, *n;
+
+	list_for_each_safe(pos, n, list) {
+		struct simplefb_clock *entry =
+			container_of(pos, struct simplefb_clock, list);
+
+		list_del(&entry->list);
+
+		clk_disable_unprepare(entry->clock);
+		clk_put(entry->clock);
+		kfree(entry);
+	}
+}
+
 struct simplefb_par {
 	u32 palette[PSEUDO_PALETTE_SIZE];
+	struct list_head clock_list[1];
 };
 
 static int simplefb_probe(struct platform_device *pdev)
@@ -236,6 +327,8 @@ static int simplefb_probe(struct platform_device *pdev)
 	}
 	info->pseudo_palette = par->palette;
 
+	simplefb_clocks_init(pdev, par->clock_list);
+
 	dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
 			     info->fix.smem_start, info->fix.smem_len,
 			     info->screen_base);
@@ -247,14 +340,15 @@ static int simplefb_probe(struct platform_device *pdev)
 	ret = register_framebuffer(info);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
-		goto error_unmap;
+		goto error_clocks;
 	}
 
 	dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
 
 	return 0;
 
- error_unmap:
+ error_clocks:
+	simplefb_clocks_destroy(par->clock_list);
 	iounmap(info->screen_base);
 
  error_fb_release:
@@ -266,8 +360,10 @@ static int simplefb_probe(struct platform_device *pdev)
 static int simplefb_remove(struct platform_device *pdev)
 {
 	struct fb_info *info = platform_get_drvdata(pdev);
+	struct simplefb_par *par = info->par;
 
 	unregister_framebuffer(info);
+	simplefb_clocks_destroy(par->clock_list);
 	framebuffer_release(info);
 
 	return 0;
-- 
2.1.0

Re: [PATCH v2 5/5] simplefb: add clock handling code

[Maxime Ripard]

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

Hi,

On Fri, Oct 03, 2014 at 01:52:05PM +0200, Hans de Goede wrote:
> From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
> 
> This claims and enables clocks listed in the simple framebuffer dt node.
> This is needed so that the display engine, in case the required clocks
> are known by the kernel code and are described in the dt, will remain
> properly enabled.
> 
> Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
> [hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: drop dev_err on kzalloc failure]
> Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index b7d5c08..f329cc1 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -26,6 +26,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_data/simplefb.h>
>  #include <linux/platform_device.h>
> +#include <linux/clk-provider.h>
>  
>  static struct fb_fix_screeninfo simplefb_fix = {
>  	.id		= "simple",
> @@ -165,8 +166,98 @@ static int simplefb_parse_pd(struct platform_device *pdev,
>  	return 0;
>  }
>  
> +/*
> + * Clock handling code.
> + *
> + * Here we handle the clocks property of our "simple-framebuffer" dt node.
> + * This is necessary so that we can make sure that any clocks needed by
> + * the display engine that the bootloader set up for us (and for which it
> + * provided a simplefb dt node), stay up, for the life of the simplefb
> + * driver.
> + *
> + * When the driver unloads, we cleanly disable, and then release the clocks.
> + */
> +struct simplefb_clock {
> +	struct list_head list;
> +	struct clk *clock;
> +};
> +
> +/*
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up simplefb, and the clock definitions in the device tree. Chances are
> + * that there are no adverse effects, and if there are, a clean teardown of
> + * the fb probe will not help us much either. So just complain and carry on,
> + * and hope that the user actually gets a working fb at the end of things.
> + */
> +static void
> +simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int clock_count, i;
> +
> +	INIT_LIST_HEAD(list);
> +
> +	if (dev_get_platdata(&pdev->dev) || !np)
> +		return;
> +
> +	clock_count = of_clk_get_parent_count(np);

This looks like it does what you expect, but its name and the fact
that it's in the clk-provider.h file makes me wonder if you're not
using the wrong side of the abstraction.

> +	for (i = 0; i < clock_count; i++) {
> +		struct simplefb_clock *entry;
> +		struct clk *clock = of_clk_get(np, i);

devm_clk_get?

> +		int ret;
> +
> +		if (IS_ERR(clock)) {
> +			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
> +			       __func__, i, PTR_ERR(clock));
> +			continue;
> +		}
> +
> +		ret = clk_prepare_enable(clock);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"%s: failed to enable clock %d: %d\n",
> +			       __func__, i, ret);
> +			clk_put(clock);
> +			continue;
> +		}
> +
> +		entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
> +		if (!entry) {
> +			clk_disable_unprepare(clock);
> +			clk_put(clock);
> +			continue;
> +		}
> +
> +		entry->clock = clock;
> +		/*
> +		 * add to the front of the list, so we disable clocks in the
> +		 * correct order.
> +		 */
> +		list_add(&entry->list, list);

I really don't think this whole list dance is necessary, especially
after reading this comment. 

It doesn't make any sense to release the clock in a particular order,
when you don't even know in what order the bootloader actually set
them (and you haven't set any requirement on the order in the binding
either). For all you know, this could be a purely random order,
without any meaning, and it would be fine, so let's just not worry
about that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Re: [PATCH v2 5/5] simplefb: add clock handling code

[Hans de Goede]

Hi,

On 10/06/2014 10:55 AM, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Oct 03, 2014 at 01:52:05PM +0200, Hans de Goede wrote:
>> From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
>>
>> This claims and enables clocks listed in the simple framebuffer dt node.
>> This is needed so that the display engine, in case the required clocks
>> are known by the kernel code and are described in the dt, will remain
>> properly enabled.
>>
>> Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
>> [hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: drop dev_err on kzalloc failure]
>> Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 98 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> index b7d5c08..f329cc1 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/module.h>
>>  #include <linux/platform_data/simplefb.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/clk-provider.h>
>>  
>>  static struct fb_fix_screeninfo simplefb_fix = {
>>  	.id		= "simple",
>> @@ -165,8 +166,98 @@ static int simplefb_parse_pd(struct platform_device *pdev,
>>  	return 0;
>>  }
>>  
>> +/*
>> + * Clock handling code.
>> + *
>> + * Here we handle the clocks property of our "simple-framebuffer" dt node.
>> + * This is necessary so that we can make sure that any clocks needed by
>> + * the display engine that the bootloader set up for us (and for which it
>> + * provided a simplefb dt node), stay up, for the life of the simplefb
>> + * driver.
>> + *
>> + * When the driver unloads, we cleanly disable, and then release the clocks.
>> + */
>> +struct simplefb_clock {
>> +	struct list_head list;
>> +	struct clk *clock;
>> +};
>> +
>> +/*
>> + * We only complain about errors here, no action is taken as the most likely
>> + * error can only happen due to a mismatch between the bootloader which set
>> + * up simplefb, and the clock definitions in the device tree. Chances are
>> + * that there are no adverse effects, and if there are, a clean teardown of
>> + * the fb probe will not help us much either. So just complain and carry on,
>> + * and hope that the user actually gets a working fb at the end of things.
>> + */
>> +static void
>> +simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	int clock_count, i;
>> +
>> +	INIT_LIST_HEAD(list);
>> +
>> +	if (dev_get_platdata(&pdev->dev) || !np)
>> +		return;
>> +
>> +	clock_count = of_clk_get_parent_count(np);
> 
> This looks like it does what you expect, but its name and the fact
> that it's in the clk-provider.h file makes me wonder if you're not
> using the wrong side of the abstraction.

I'll check to see if there is something better, assuming Luc does not
beat me to it.

> 
>> +	for (i = 0; i < clock_count; i++) {
>> +		struct simplefb_clock *entry;
>> +		struct clk *clock = of_clk_get(np, i);
> 
> devm_clk_get?

Yes that would be better.

>> +		int ret;
>> +
>> +		if (IS_ERR(clock)) {
>> +			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
>> +			       __func__, i, PTR_ERR(clock));
>> +			continue;
>> +		}
>> +
>> +		ret = clk_prepare_enable(clock);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +				"%s: failed to enable clock %d: %d\n",
>> +			       __func__, i, ret);
>> +			clk_put(clock);
>> +			continue;
>> +		}
>> +
>> +		entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
>> +		if (!entry) {
>> +			clk_disable_unprepare(clock);
>> +			clk_put(clock);
>> +			continue;
>> +		}
>> +
>> +		entry->clock = clock;
>> +		/*
>> +		 * add to the front of the list, so we disable clocks in the
>> +		 * correct order.
>> +		 */
>> +		list_add(&entry->list, list);
> 
> I really don't think this whole list dance is necessary, especially
> after reading this comment. 

So you're suggesting to just make this an array, with say 5 entries, or ... ?

Regards,

Hans

Re: Re: [PATCH v2 5/5] simplefb: add clock handling code

[Maxime Ripard]

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

On Mon, Oct 06, 2014 at 11:11:44AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/06/2014 10:55 AM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Fri, Oct 03, 2014 at 01:52:05PM +0200, Hans de Goede wrote:
> >> From: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
> >>
> >> This claims and enables clocks listed in the simple framebuffer dt node.
> >> This is needed so that the display engine, in case the required clocks
> >> are known by the kernel code and are described in the dt, will remain
> >> properly enabled.
> >>
> >> Signed-off-by: Luc Verhaegen <libv-AgBVmzD5pcezQB+pC5nmwQ@public.gmane.org>
> >> [hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org: drop dev_err on kzalloc failure]
> >> Reviewed-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  drivers/video/fbdev/simplefb.c | 100 ++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 98 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> >> index b7d5c08..f329cc1 100644
> >> --- a/drivers/video/fbdev/simplefb.c
> >> +++ b/drivers/video/fbdev/simplefb.c
> >> @@ -26,6 +26,7 @@
> >>  #include <linux/module.h>
> >>  #include <linux/platform_data/simplefb.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/clk-provider.h>
> >>  
> >>  static struct fb_fix_screeninfo simplefb_fix = {
> >>  	.id		= "simple",
> >> @@ -165,8 +166,98 @@ static int simplefb_parse_pd(struct platform_device *pdev,
> >>  	return 0;
> >>  }
> >>  
> >> +/*
> >> + * Clock handling code.
> >> + *
> >> + * Here we handle the clocks property of our "simple-framebuffer" dt node.
> >> + * This is necessary so that we can make sure that any clocks needed by
> >> + * the display engine that the bootloader set up for us (and for which it
> >> + * provided a simplefb dt node), stay up, for the life of the simplefb
> >> + * driver.
> >> + *
> >> + * When the driver unloads, we cleanly disable, and then release the clocks.
> >> + */
> >> +struct simplefb_clock {
> >> +	struct list_head list;
> >> +	struct clk *clock;
> >> +};
> >> +
> >> +/*
> >> + * We only complain about errors here, no action is taken as the most likely
> >> + * error can only happen due to a mismatch between the bootloader which set
> >> + * up simplefb, and the clock definitions in the device tree. Chances are
> >> + * that there are no adverse effects, and if there are, a clean teardown of
> >> + * the fb probe will not help us much either. So just complain and carry on,
> >> + * and hope that the user actually gets a working fb at the end of things.
> >> + */
> >> +static void
> >> +simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
> >> +{
> >> +	struct device_node *np = pdev->dev.of_node;
> >> +	int clock_count, i;
> >> +
> >> +	INIT_LIST_HEAD(list);
> >> +
> >> +	if (dev_get_platdata(&pdev->dev) || !np)
> >> +		return;
> >> +
> >> +	clock_count = of_clk_get_parent_count(np);
> > 
> > This looks like it does what you expect, but its name and the fact
> > that it's in the clk-provider.h file makes me wonder if you're not
> > using the wrong side of the abstraction.
> 
> I'll check to see if there is something better, assuming Luc does not
> beat me to it.
> 
> > 
> >> +	for (i = 0; i < clock_count; i++) {
> >> +		struct simplefb_clock *entry;
> >> +		struct clk *clock = of_clk_get(np, i);
> > 
> > devm_clk_get?
> 
> Yes that would be better.
> 
> >> +		int ret;
> >> +
> >> +		if (IS_ERR(clock)) {
> >> +			dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
> >> +			       __func__, i, PTR_ERR(clock));
> >> +			continue;
> >> +		}
> >> +
> >> +		ret = clk_prepare_enable(clock);
> >> +		if (ret) {
> >> +			dev_err(&pdev->dev,
> >> +				"%s: failed to enable clock %d: %d\n",
> >> +			       __func__, i, ret);
> >> +			clk_put(clock);
> >> +			continue;
> >> +		}
> >> +
> >> +		entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
> >> +		if (!entry) {
> >> +			clk_disable_unprepare(clock);
> >> +			clk_put(clock);
> >> +			continue;
> >> +		}
> >> +
> >> +		entry->clock = clock;
> >> +		/*
> >> +		 * add to the front of the list, so we disable clocks in the
> >> +		 * correct order.
> >> +		 */
> >> +		list_add(&entry->list, list);
> > 
> > I really don't think this whole list dance is necessary, especially
> > after reading this comment. 
> 
> So you're suggesting to just make this an array, with say 5 entries,
> or ... ?

Maybe something smarter, like a kmalloc'd array with the number of
clocks needed?

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 5/5] simplefb: add clock handling code

[Geert Uytterhoeven]

On Fri, Oct 3, 2014 at 1:52 PM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> +static void
> +simplefb_clocks_init(struct platform_device *pdev, struct list_head *list)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       int clock_count, i;
> +
> +       INIT_LIST_HEAD(list);
> +
> +       if (dev_get_platdata(&pdev->dev) || !np)
> +               return;
> +
> +       clock_count = of_clk_get_parent_count(np);
> +       for (i = 0; i < clock_count; i++) {
> +               struct simplefb_clock *entry;
> +               struct clk *clock = of_clk_get(np, i);
> +               int ret;
> +
> +               if (IS_ERR(clock)) {
> +                       dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
> +                              __func__, i, PTR_ERR(clock));
> +                       continue;
> +               }
> +
> +               ret = clk_prepare_enable(clock);
> +               if (ret) {
> +                       dev_err(&pdev->dev,
> +                               "%s: failed to enable clock %d: %d\n",
> +                              __func__, i, ret);
> +                       clk_put(clock);
> +                       continue;
> +               }
> +
> +               entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL);
> +               if (!entry) {
> +                       clk_disable_unprepare(clock);
> +                       clk_put(clock);
> +                       continue;
> +               }

I'd allocate memory first, to avoid calling clk_disable_unprepare() (which
will kill your display) if memory allocation fails.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds