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 <[email protected]> > >> > >> 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 <[email protected]> > >> [[email protected]: drop dev_err on kzalloc failure] > >> Reviewed-by: Hans de Goede <[email protected]> > >> Signed-off-by: Hans de Goede <[email protected]> > >> --- > >> 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
signature.asc
Description: Digital signature
