[PATCH 4/4] simplefb: add clock handling code

[PATCH 4/4] simplefb: add clock handling code

[Luc Verhaegen]

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@skynet.be>
---
 drivers/video/fbdev/simplefb.c |  103 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 74c4b2a..481dbfd 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",
@@ -191,8 +192,101 @@ 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) {
+			dev_err(&pdev->dev,
+				"%s: failed to alloc clock %d list entry.\n",
+			       __func__, i);
+			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)
@@ -265,6 +359,8 @@ static int simplefb_probe(struct platform_device *pdev)
 	}
 	info->pseudo_palette = (void *) 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);
@@ -276,14 +372,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:
@@ -299,8 +396,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);
 	if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node)
 		simplefb_dt_disable(pdev);
-- 
1.7.7

Re: [PATCH 4/4] simplefb: add clock handling code

[Stephen Warren]

On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
> 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.

I think this make simplefb not simple any more, which rather goes 
against the whole point of it.

I specifically conceived simplefb to know about nothing more than the 
memory address and pixel layout of the memory buffer. I certainly don't 
like the idea of expanding simplefb to anything beyond that, and IIRC 
*not* extending is was a condition agreed when it was first merged. If 
more knowledge than that is required, then there needs to be a 
HW-specific driver to manage any clocks/resets/video registers, etc.

The correct way to handle this without a complete DRM/KMS/... driver is 
to avoid the clocks in question being turned off at boot. I thought 
there was a per-clock flag to prevent disabling an unused clock? If not, 
perhaps the clock driver should force the clock to be enabled (perhaps 
only if the DRM/KMS driver isn't enabled?). For example, the Tegra clock 
driver has a clock initialization table which IIRC was used for this 
purpose before we got a DRM/KMS driver. That way, all the details are 
kept inside the kernel code, and don't end up influencing the DT 
representation of simplefb.

Re: [PATCH 4/4] simplefb: add clock handling code

[Luc Verhaegen]

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>
> I think this make simplefb not simple any more, which rather goes  
> against the whole point of it.
>
> I specifically conceived simplefb to know about nothing more than the  
> memory address and pixel layout of the memory buffer. I certainly don't  
> like the idea of expanding simplefb to anything beyond that, and IIRC  
> *not* extending is was a condition agreed when it was first merged. If  
> more knowledge than that is required, then there needs to be a  
> HW-specific driver to manage any clocks/resets/video registers, etc.

Yes. Simplefb quickly becomes anything but, doesn't it. Perhaps DenialFB 
would've been a better name for it ;p

> The correct way to handle this without a complete DRM/KMS/... driver is  
> to avoid the clocks in question being turned off at boot. I thought  
> there was a per-clock flag to prevent disabling an unused clock? If not,  
> perhaps the clock driver should force the clock to be enabled (perhaps  
> only if the DRM/KMS driver isn't enabled?). For example, the Tegra clock  
> driver has a clock initialization table which IIRC was used for this  
> purpose before we got a DRM/KMS driver. That way, all the details are  
> kept inside the kernel code, and don't end up influencing the DT  
> representation of simplefb.

How was simplefb handled on tegra? Where is the code for that? I didn't 
see anything in u-boot for instance.

But the code for handling clocks where they are supposed to be handled 
is pretty generic from where i sit.

Luc Verhaegen.

Re: [PATCH 4/4] simplefb: add clock handling code

[Maxime Ripard]

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

On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
> >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.
> 
> I think this make simplefb not simple any more, which rather goes
> against the whole point of it.
> 
> I specifically conceived simplefb to know about nothing more than
> the memory address and pixel layout of the memory buffer. I
> certainly don't like the idea of expanding simplefb to anything
> beyond that, and IIRC *not* extending is was a condition agreed when
> it was first merged. If more knowledge than that is required, then
> there needs to be a HW-specific driver to manage any
> clocks/resets/video registers, etc.

I'm sorry, but how is that not simple? clocks enabling is step 1 in a
driver in order to communicate somehow with the controller. Reset is a
different story, because arguably, if simplefb is there, the
controller is already out of reset.

And I don't see why video registers are coming into the discussion
here. The code Luc posted doesn't access any register, at all. It just
makes sure the needed controller keep going.

> The correct way to handle this without a complete DRM/KMS/... driver
> is to avoid the clocks in question being turned off at boot.

Which is exactly what this code does, using the generic DT bindings to
express dependency for a given clock. How is this wrong?

> I thought there was a per-clock flag to prevent disabling an unused
> clock?

No, last time I heard, Mike Turquette was against it.

> If not, perhaps the clock driver should force the clock to be
> enabled (perhaps only if the DRM/KMS driver isn't enabled?).

I'm sorry, but I'm not going to take any code that will do that in our
clock driver.

I'm not going to have a huge list of ifdef depending on configuration
options to know which clock to enable, especially when clk_get should
have the consumer device as an argument.

> For example, the Tegra clock driver has a clock initialization table
> which IIRC was used for this purpose before we got a DRM/KMS driver.
> That way, all the details are kept inside the kernel code, and don't
> end up influencing the DT representation of simplefb.

I don't really see how the optional usage of a generic property
influences badly the DT representation of simplefb.

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: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

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

On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
[...]
> > If not, perhaps the clock driver should force the clock to be
> > enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> 
> I'm sorry, but I'm not going to take any code that will do that in our
> clock driver.
> 
> I'm not going to have a huge list of ifdef depending on configuration
> options to know which clock to enable, especially when clk_get should
> have the consumer device as an argument.

Are you saying is that you want to solve a platform-specific problem by
pushing code into simple, generic drivers so that your platform code can
stay "clean"?

Thierry

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

Re: [PATCH 4/4] simplefb: add clock handling code

[Maxime Ripard]

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

On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> [...]
> > > If not, perhaps the clock driver should force the clock to be
> > > enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> > 
> > I'm sorry, but I'm not going to take any code that will do that in our
> > clock driver.
> > 
> > I'm not going to have a huge list of ifdef depending on configuration
> > options to know which clock to enable, especially when clk_get should
> > have the consumer device as an argument.
> 
> Are you saying is that you want to solve a platform-specific problem by
> pushing code into simple, generic drivers so that your platform code can
> stay "clean"?

Are you saying that this driver would become "dirty" with such a patch?

If so, we really have an issue in the kernel.

-- 
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 4/4] simplefb: add clock handling code

[Thierry Reding]

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

On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> > On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> > [...]
> > > > If not, perhaps the clock driver should force the clock to be
> > > > enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> > > 
> > > I'm sorry, but I'm not going to take any code that will do that in our
> > > clock driver.
> > > 
> > > I'm not going to have a huge list of ifdef depending on configuration
> > > options to know which clock to enable, especially when clk_get should
> > > have the consumer device as an argument.
> > 
> > Are you saying is that you want to solve a platform-specific problem by
> > pushing code into simple, generic drivers so that your platform code can
> > stay "clean"?
> 
> Are you saying that this driver would become "dirty" with such a patch?

Yes. Others have said the same and even provided alternative solutions
on how to solve what's seemingly a platform-specific problem in a
platform-specific way.

> If so, we really have an issue in the kernel.

Can you elaborate?

Thierry

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

[PATCH 1/4] 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@redhat.com>
---
 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

[PATCH 2/4] simplefb: formalize pseudo palette handling

[Hans de Goede]

From: Luc Verhaegen <libv@skynet.be>

Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede@redhat.com: drop unnecessary void * cast]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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 3/4] simplefb: add goto error path to probe

[Hans de Goede]

From: Luc Verhaegen <libv@skynet.be>

Signed-off-by: Luc Verhaegen <libv@skynet.be>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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 4/4] simplefb: add clock handling code

[Hans de Goede]

From: Luc Verhaegen <libv@skynet.be>

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@skynet.be>
[hdegoede@redhat.com: drop dev_err on kzalloc failure]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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 1/4] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Mike Turquette]

On Sun, Sep 28, 2014 at 5:43 AM, Hans de Goede <hdegoede@redhat.com> 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@redhat.com>

Acked-by: Mike Turquette <mturquette@linaro.org>
or
Reviewed-by: Mike Turquette <mturquette@linaro.org>

I don't know what is the right thing with these binding definitions...

Also I have one suggestion below:

> ---
>  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 {

It might be nice to add the clocks property to the example. Something like:

clocks = <&osc 1>, <&ref 0>;

Regards,
Mike

> --
> 2.1.0
>

Re: [PATCH 1/4] dt-bindings: Add a clocks property to the simple-framebuffer binding

[Hans de Goede]

Hi,

On 09/28/2014 08:37 PM, Mike Turquette wrote:
> On Sun, Sep 28, 2014 at 5:43 AM, Hans de Goede <hdegoede@redhat.com> 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@redhat.com>
> 
> Acked-by: Mike Turquette <mturquette@linaro.org>
> or
> Reviewed-by: Mike Turquette <mturquette@linaro.org>
> 
> I don't know what is the right thing with these binding definitions...
> 
> Also I have one suggestion below:
> 
>> ---
>>  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 {
> 
> It might be nice to add the clocks property to the example. Something like:
> 
> clocks = <&osc 1>, <&ref 0>;

Given the optional nature of the clocks, I've deliberately left them out,
but if people want to I can do a v2 with a clocks property added to the example.

Regards,

Hans