| Message ID | 1412337125-14388-6-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, Oct 03, 2014 at 01:52:05PM +0200, Hans de Goede wrote: > 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] > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > 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); 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
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@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] >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> 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); > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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] > >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > >> 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); > > > > 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?
On Fri, Oct 3, 2014 at 1:52 PM, Hans de Goede <hdegoede@redhat.com> 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@linux-m68k.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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;