| Message ID | 1428064923-24950-6-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 03, 2015 at 02:42:02PM +0200, Geert Uytterhoeven wrote: > +int __init board_staging_register_clock(const struct board_staging_clk *bsc) > +{ > + struct clk *clk; > + int error; > + > + pr_debug("Registering clock %s for con_id %s dev_id %s\n", bsc->clk, > + bsc->con_id, bsc->dev_id); > + clk = clk_get(NULL, bsc->clk); > + if (IS_ERR(clk)) { > + error = PTR_ERR(clk); > + pr_err("Failed to get clock %s (%d)\n", bsc->clk, error); > + return error; > + } > + > + error = clk_register_clkdev(clk, bsc->con_id, bsc->dev_id); > + if (error) > + pr_err("Failed to register clock %s (%d)\n", bsc->clk, error); > + return error; Missing curly braces. Also it's weird that don't we need a clk_put() on the error patch as well as the success path? > + > + clk_put(clk); > + return 0; > +} regards, dan carpenter
On Fri, Apr 3, 2015 at 2:57 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: >> + error = clk_register_clkdev(clk, bsc->con_id, bsc->dev_id); >> + if (error) >> + pr_err("Failed to register clock %s (%d)\n", bsc->clk, error); >> + return error; > > Missing curly braces. Also it's weird that don't we need a clk_put() > on the error patch as well as the success path? Thanks! So it worked only by accident: with the new per-user struct clk instances clk_put() must not be called if clk_register_clkdev() succeeded. Will call clk_put() only if it failed. >> + >> + clk_put(clk); >> + return 0; 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
On Fri, Apr 03, 2015 at 03:57:27PM +0300, Dan Carpenter wrote: > On Fri, Apr 03, 2015 at 02:42:02PM +0200, Geert Uytterhoeven wrote: > > +int __init board_staging_register_clock(const struct board_staging_clk *bsc) > > +{ > > + struct clk *clk; > > + int error; > > + > > + pr_debug("Registering clock %s for con_id %s dev_id %s\n", bsc->clk, > > + bsc->con_id, bsc->dev_id); > > + clk = clk_get(NULL, bsc->clk); > > + if (IS_ERR(clk)) { > > + error = PTR_ERR(clk); > > + pr_err("Failed to get clock %s (%d)\n", bsc->clk, error); > > + return error; > > + } > > + > > + error = clk_register_clkdev(clk, bsc->con_id, bsc->dev_id); > > + if (error) > > + pr_err("Failed to register clock %s (%d)\n", bsc->clk, error); > > + return error; > > Missing curly braces. Also it's weird that don't we need a clk_put() > on the error patch as well as the success path? What's also concerning is that this is an abuse of this. clk_register_clkdev() is supposed to be used with clocks created with the CCF functions, it's not for creating aliases. We have clk_add_alias() which does *everything* that this function does, only in a less buggy way.
On Fri, Apr 03, 2015 at 03:27:40PM +0200, Geert Uytterhoeven wrote: > On Fri, Apr 3, 2015 at 2:57 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > >> + error = clk_register_clkdev(clk, bsc->con_id, bsc->dev_id); > >> + if (error) > >> + pr_err("Failed to register clock %s (%d)\n", bsc->clk, error); > >> + return error; > > > > Missing curly braces. Also it's weird that don't we need a clk_put() > > on the error patch as well as the success path? > > Thanks! > > So it worked only by accident: with the new per-user struct clk instances > clk_put() must not be called if clk_register_clkdev() succeeded. Yes, that's because the per-user struct clk messed quite a lot of things up - the patches were /not/ well tested before they went in. That's no excuse to work around the breakage they caused though. That said, I never did post the work I did earlier this month to fix the problems in clkdev which those patches caused... so, I guess it's time to post them and rush them in for the 4.1 merge window... (frankly, the per-user struct clk patches should've been reverted.)
Hi Geert, Thank you for the patch. On Friday 03 April 2015 14:42:02 Geert Uytterhoeven wrote: > Add support for easy registering of one ore more platform devices that > may: > - need clocks that are described in DT, > - need pin control configuration, > - rely on a configured GPIO, > - be part of a PM Domain. > > All these dependencies are optional. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/staging/board/board.c | 76 ++++++++++++++++++++++++++++++++++++++++ > drivers/staging/board/board.h | 31 ++++++++++++++++++ > 2 files changed, 107 insertions(+) > > diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c > index b84ac2837a20bf06..da2469e2d4262fac 100644 > --- a/drivers/staging/board/board.c > +++ b/drivers/staging/board/board.c > @@ -9,6 +9,9 @@ > > #define pr_fmt(fmt) "board_staging: " fmt > > +#include <linux/clk.h> > +#include <linux/clkdev.h> > +#include <linux/gpio.h> > #include <linux/init.h> > #include <linux/irq.h> > #include <linux/device.h> > @@ -16,6 +19,9 @@ > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > +#include <linux/pinctrl/machine.h> > +#include <linux/platform_device.h> > +#include <linux/pm_domain.h> > > #include "board.h" > > @@ -104,3 +110,73 @@ void __init board_staging_fixup_irq_resources(struct > resource *res, res[i].start = virq; > } > } > + > +int __init board_staging_register_clock(const struct board_staging_clk > *bsc) +{ > + struct clk *clk; > + int error; > + > + pr_debug("Registering clock %s for con_id %s dev_id %s\n", bsc->clk, > + bsc->con_id, bsc->dev_id); > + clk = clk_get(NULL, bsc->clk); > + if (IS_ERR(clk)) { > + error = PTR_ERR(clk); > + pr_err("Failed to get clock %s (%d)\n", bsc->clk, error); > + return error; > + } > + > + error = clk_register_clkdev(clk, bsc->con_id, bsc->dev_id); > + if (error) > + pr_err("Failed to register clock %s (%d)\n", bsc->clk, error); > + return error; > + > + clk_put(clk); > + return 0; > +} > + > +int __init board_staging_register_device(const struct board_staging_dev > *dev) +{ > + struct platform_device *pdev = dev->pdev; > + unsigned int i; > + int error; > + > + pr_debug("Trying to register device %s\n", pdev->name); > + if (board_staging_dt_node_available(pdev->resource, > + pdev->num_resources)) { > + pr_warn("Skipping %s, already in DT\n", pdev->name); > + return -EEXIST; > + } > + > + board_staging_fixup_irq_resources(pdev->resource, pdev->num_resources); > + > + for (i = 0; i < dev->nclocks; i++) > + board_staging_register_clock(&dev->clocks[i]); > + > + if (dev->npinmaps) > + pinctrl_register_mappings(dev->pinmaps, dev->npinmaps); > + > + for (i = 0; i < dev->ngpios; i++) > + gpio_request_one(dev->gpios[i].gpio, dev->gpios[i].flags, > + pdev->name); Aren't GPIO numbers dynamic too in DT-based systems ? Beside, shouldn't it be the responsibility of the drievr to request the GPIOs it needs ? > + error = platform_device_register(pdev); > + if (error) { > + pr_err("Failed to register device %s (%d)\n", pdev->name, > + error); > + return error; > + } > + > + if (dev->domain) > + __pm_genpd_name_add_device(dev->domain, &pdev->dev, NULL); > + > + return error; > +} > + > +void __init board_staging_register_devices(const struct board_staging_dev > *devs, + unsigned int ndevs) > +{ > + unsigned int i; > + > + for (i = 0; i < ndevs; i++) > + board_staging_register_device(&devs[i]); > +} > diff --git a/drivers/staging/board/board.h b/drivers/staging/board/board.h > index 4cedc3c46e287eb7..7aaa0f7d6fafb9e5 100644 > --- a/drivers/staging/board/board.h > +++ b/drivers/staging/board/board.h > @@ -4,12 +4,43 @@ > #include <linux/init.h> > #include <linux/of.h> > > +struct board_staging_clk { > + const char *clk; > + const char *con_id; > + const char *dev_id; > +}; > + > +struct board_staging_gpio { > + unsigned int gpio; > + unsigned long flags; /* See GPIOF_* */ > +}; > + > +struct board_staging_dev { > + /* Platform Device */ > + struct platform_device *pdev; > + /* Clocks (optional) */ > + const struct board_staging_clk *clocks; > + unsigned int nclocks; > + /* Pin Control Maps (optional) */ > + const struct pinctrl_map *pinmaps; > + unsigned int npinmaps; > + /* GPIOs (optional) */ > + const struct board_staging_gpio *gpios; > + unsigned int ngpios; > + /* PM Domain (optional) */ > + const char *domain; > +}; > + > struct resource; > > bool board_staging_dt_node_available(const struct resource *resource, > unsigned int num_resources); > int board_staging_setup_hwirq_xlate(const char *irqc_match, unsigned int > base); void board_staging_fixup_irq_resources(struct resource *res, > unsigned int nres); +int board_staging_register_clock(const struct > board_staging_clk *bsc); +int board_staging_register_device(const struct > board_staging_dev *dev); +void board_staging_register_devices(const struct > board_staging_dev *devs, + unsigned int ndevs); > > #define board_staging(str, fn) \ > static int __init runtime_board_check(void) \
Hi Russell, On Fri, Apr 3, 2015 at 7:04 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Apr 03, 2015 at 03:57:27PM +0300, Dan Carpenter wrote: >> On Fri, Apr 03, 2015 at 02:42:02PM +0200, Geert Uytterhoeven wrote: >> > +int __init board_staging_register_clock(const struct board_staging_clk *bsc) >> > +{ >> > + struct clk *clk; >> > + int error; >> > + >> > + pr_debug("Registering clock %s for con_id %s dev_id %s\n", bsc->clk, >> > + bsc->con_id, bsc->dev_id); >> > + clk = clk_get(NULL, bsc->clk); >> > + if (IS_ERR(clk)) { >> > + error = PTR_ERR(clk); >> > + pr_err("Failed to get clock %s (%d)\n", bsc->clk, error); >> > + return error; >> > + } >> > + >> > + error = clk_register_clkdev(clk, bsc->con_id, bsc->dev_id); >> > + if (error) >> > + pr_err("Failed to register clock %s (%d)\n", bsc->clk, error); >> > + return error; >> >> Missing curly braces. Also it's weird that don't we need a clk_put() >> on the error patch as well as the success path? > > What's also concerning is that this is an abuse of this. > > clk_register_clkdev() is supposed to be used with clocks created with > the CCF functions, it's not for creating aliases. > > We have clk_add_alias() which does *everything* that this function does, > only in a less buggy way. Thanks, I didn't know about clk_add_alias(). I had based the above on long gone code under arch/arm/mach-shmobile to use platform devices with CCF. 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
Hi Laurent, On Sat, Apr 4, 2015 at 2:46 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> + for (i = 0; i < dev->ngpios; i++) >> + gpio_request_one(dev->gpios[i].gpio, dev->gpios[i].flags, >> + pdev->name); > > Aren't GPIO numbers dynamic too in DT-based systems ? Beside, shouldn't it be Apparently not, as the old legacy number still works, and it doesn't work without. > the responsibility of the drievr to request the GPIOs it needs ? As far as I understand it, on Armadillo this is used more for platform configuration than for device configuration, as it affects multiple devices (the comment says DBGMD/LCDC0/FSIA MUX). I guess I could use a "gpio-hog" subnode in DT instead, but then we're already implementing the conversion to DT ;-) 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
Hi Geert, On Sunday 05 April 2015 11:00:56 Geert Uytterhoeven wrote: > On Sat, Apr 4, 2015 at 2:46 PM, Laurent Pinchart wrote: > >> + for (i = 0; i < dev->ngpios; i++) > >> + gpio_request_one(dev->gpios[i].gpio, dev->gpios[i].flags, > >> + pdev->name); > > > > Aren't GPIO numbers dynamic too in DT-based systems ? Beside, shouldn't it > > be > > Apparently not, as the old legacy number still works, and it doesn't work > without. I think we're just lucky there that the SoC main GPIO controller gets registered first and starts counting GPIOs with a zero offset. > > the responsibility of the drievr to request the GPIOs it needs ? > > As far as I understand it, on Armadillo this is used more for platform > configuration than for device configuration, as it affects multiple devices > (the comment says DBGMD/LCDC0/FSIA MUX). > > I guess I could use a "gpio-hog" subnode in DT instead, but then we're > already implementing the conversion to DT ;-) But that's the goal :-) I'd rather move GPIO and pinctrl to DT directly as we already have the infrastructure to do so.
diff --git a/drivers/staging/board/board.c b/drivers/staging/board/board.c index b84ac2837a20bf06..da2469e2d4262fac 100644 --- a/drivers/staging/board/board.c +++ b/drivers/staging/board/board.c @@ -9,6 +9,9 @@ #define pr_fmt(fmt) "board_staging: " fmt +#include <linux/clk.h> +#include <linux/clkdev.h> +#include <linux/gpio.h> #include <linux/init.h> #include <linux/irq.h> #include <linux/device.h> @@ -16,6 +19,9 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <linux/pinctrl/machine.h> +#include <linux/platform_device.h> +#include <linux/pm_domain.h> #include "board.h" @@ -104,3 +110,73 @@ void __init board_staging_fixup_irq_resources(struct resource *res, res[i].start = virq; } } + +int __init board_staging_register_clock(const struct board_staging_clk *bsc) +{ + struct clk *clk; + int error; + + pr_debug("Registering clock %s for con_id %s dev_id %s\n", bsc->clk, + bsc->con_id, bsc->dev_id); + clk = clk_get(NULL, bsc->clk); + if (IS_ERR(clk)) { + error = PTR_ERR(clk); + pr_err("Failed to get clock %s (%d)\n", bsc->clk, error); + return error; + } + + error = clk_register_clkdev(clk, bsc->con_id, bsc->dev_id); + if (error) + pr_err("Failed to register clock %s (%d)\n", bsc->clk, error); + return error; + + clk_put(clk); + return 0; +} + +int __init board_staging_register_device(const struct board_staging_dev *dev) +{ + struct platform_device *pdev = dev->pdev; + unsigned int i; + int error; + + pr_debug("Trying to register device %s\n", pdev->name); + if (board_staging_dt_node_available(pdev->resource, + pdev->num_resources)) { + pr_warn("Skipping %s, already in DT\n", pdev->name); + return -EEXIST; + } + + board_staging_fixup_irq_resources(pdev->resource, pdev->num_resources); + + for (i = 0; i < dev->nclocks; i++) + board_staging_register_clock(&dev->clocks[i]); + + if (dev->npinmaps) + pinctrl_register_mappings(dev->pinmaps, dev->npinmaps); + + for (i = 0; i < dev->ngpios; i++) + gpio_request_one(dev->gpios[i].gpio, dev->gpios[i].flags, + pdev->name); + + error = platform_device_register(pdev); + if (error) { + pr_err("Failed to register device %s (%d)\n", pdev->name, + error); + return error; + } + + if (dev->domain) + __pm_genpd_name_add_device(dev->domain, &pdev->dev, NULL); + + return error; +} + +void __init board_staging_register_devices(const struct board_staging_dev *devs, + unsigned int ndevs) +{ + unsigned int i; + + for (i = 0; i < ndevs; i++) + board_staging_register_device(&devs[i]); +} diff --git a/drivers/staging/board/board.h b/drivers/staging/board/board.h index 4cedc3c46e287eb7..7aaa0f7d6fafb9e5 100644 --- a/drivers/staging/board/board.h +++ b/drivers/staging/board/board.h @@ -4,12 +4,43 @@ #include <linux/init.h> #include <linux/of.h> +struct board_staging_clk { + const char *clk; + const char *con_id; + const char *dev_id; +}; + +struct board_staging_gpio { + unsigned int gpio; + unsigned long flags; /* See GPIOF_* */ +}; + +struct board_staging_dev { + /* Platform Device */ + struct platform_device *pdev; + /* Clocks (optional) */ + const struct board_staging_clk *clocks; + unsigned int nclocks; + /* Pin Control Maps (optional) */ + const struct pinctrl_map *pinmaps; + unsigned int npinmaps; + /* GPIOs (optional) */ + const struct board_staging_gpio *gpios; + unsigned int ngpios; + /* PM Domain (optional) */ + const char *domain; +}; + struct resource; bool board_staging_dt_node_available(const struct resource *resource, unsigned int num_resources); int board_staging_setup_hwirq_xlate(const char *irqc_match, unsigned int base); void board_staging_fixup_irq_resources(struct resource *res, unsigned int nres); +int board_staging_register_clock(const struct board_staging_clk *bsc); +int board_staging_register_device(const struct board_staging_dev *dev); +void board_staging_register_devices(const struct board_staging_dev *devs, + unsigned int ndevs); #define board_staging(str, fn) \ static int __init runtime_board_check(void) \
Add support for easy registering of one ore more platform devices that may: - need clocks that are described in DT, - need pin control configuration, - rely on a configured GPIO, - be part of a PM Domain. All these dependencies are optional. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/staging/board/board.c | 76 +++++++++++++++++++++++++++++++++++++++++++ drivers/staging/board/board.h | 31 ++++++++++++++++++ 2 files changed, 107 insertions(+)