| Message ID | 1309192391-12410-3-git-send-email-b-cousson@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Benoit Cousson |
| Headers | show |
On Mon, Jun 27, 2011 at 06:33:06PM +0200, Benoit Cousson wrote: ... > + r = clk_get_sys(dev_name(&od->pdev.dev), clk_alias); > + if (!IS_ERR(r)) { > + pr_warning("omap_device: %s: %s already exist\n", > + dev_name(&od->pdev.dev), clk_alias); I believe a clk_put(r) is appropriate here. > + return; > + } > + > + r = omap_clk_get_by_name(clk_name); > + if (IS_ERR(r)) { > + pr_err("omap_device: %s: omap_clk_get_by_name for %s failed\n", > + dev_name(&od->pdev.dev), clk_name); > + return; > + } > + > + l = clkdev_alloc(r, clk_alias, dev_name(&od->pdev.dev)); > + if (!l) { > + pr_err("omap_device: %s: clkdev_alloc for %s failed\n", > + dev_name(&od->pdev.dev), clk_alias); And here. > + return; > + } > + > + clkdev_add(l); And here. Todd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/27/2011 8:56 PM, Todd Poynor wrote: > On Mon, Jun 27, 2011 at 06:33:06PM +0200, Benoit Cousson wrote: > ... >> + r = clk_get_sys(dev_name(&od->pdev.dev), clk_alias); >> + if (!IS_ERR(r)) { >> + pr_warning("omap_device: %s: %s already exist\n", >> + dev_name(&od->pdev.dev), clk_alias); > > I believe a clk_put(r) is appropriate here. Appropriate I don't know, but useless for sure :-) This clk_put is a no-op for every ARM platforms (I found one exception). I do not know why it was originally done like that, but that api is clearly not a put/get kind of API. I'm OK to add that, but I think it is a little bit misleading. > >> + return; >> + } >> + >> + r = omap_clk_get_by_name(clk_name); >> + if (IS_ERR(r)) { >> + pr_err("omap_device: %s: omap_clk_get_by_name for %s failed\n", >> + dev_name(&od->pdev.dev), clk_name); >> + return; >> + } >> + >> + l = clkdev_alloc(r, clk_alias, dev_name(&od->pdev.dev)); >> + if (!l) { >> + pr_err("omap_device: %s: clkdev_alloc for %s failed\n", >> + dev_name(&od->pdev.dev), clk_alias); > > And here. No, it is not needed in that case because the omap_clk_get_by_name is not using the clk_get API. > >> + return; >> + } >> + >> + clkdev_add(l); > > And here. Not needed either, no clk_get used. Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 28, 2011 at 04:10:55PM +0200, Cousson, Benoit wrote: > On 6/27/2011 8:56 PM, Todd Poynor wrote: > >On Mon, Jun 27, 2011 at 06:33:06PM +0200, Benoit Cousson wrote: > >... > >>+ r = clk_get_sys(dev_name(&od->pdev.dev), clk_alias); > >>+ if (!IS_ERR(r)) { > >>+ pr_warning("omap_device: %s: %s already exist\n", > >>+ dev_name(&od->pdev.dev), clk_alias); > > > >I believe a clk_put(r) is appropriate here. > > Appropriate I don't know, but useless for sure :-) > This clk_put is a no-op for every ARM platforms (I found one exception). I haven't followed the design of common struct clk closely, but it probably will do ref counting on these, so it's best to keep these matched up. > >>+ r = omap_clk_get_by_name(clk_name); > >>+ if (IS_ERR(r)) { > >>+ pr_err("omap_device: %s: omap_clk_get_by_name for %s failed\n", > >>+ dev_name(&od->pdev.dev), clk_name); > >>+ return; > >>+ } > >>+ > >>+ l = clkdev_alloc(r, clk_alias, dev_name(&od->pdev.dev)); > >>+ if (!l) { > >>+ pr_err("omap_device: %s: clkdev_alloc for %s failed\n", > >>+ dev_name(&od->pdev.dev), clk_alias); > > > >And here. > > No, it is not needed in that case because the omap_clk_get_by_name > is not using the clk_get API. Ah, didn't check that one, sorry. That's an unfortunate use of "get" in the API name IMO. When common struct clk takes over, this may need some tweaking. Todd -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/28/2011 8:21 PM, Todd Poynor wrote: > On Tue, Jun 28, 2011 at 04:10:55PM +0200, Cousson, Benoit wrote: >> On 6/27/2011 8:56 PM, Todd Poynor wrote: >>> On Mon, Jun 27, 2011 at 06:33:06PM +0200, Benoit Cousson wrote: >>> ... >>>> + r = clk_get_sys(dev_name(&od->pdev.dev), clk_alias); >>>> + if (!IS_ERR(r)) { >>>> + pr_warning("omap_device: %s: %s already exist\n", >>>> + dev_name(&od->pdev.dev), clk_alias); >>> >>> I believe a clk_put(r) is appropriate here. >> >> Appropriate I don't know, but useless for sure :-) >> This clk_put is a no-op for every ARM platforms (I found one exception). > > I haven't followed the design of common struct clk closely, but it > probably will do ref counting on these, so it's best to keep these > matched up. I didn't follow the design either, but it was posted in 2008, and still nobody is doing any reference counting... That being said we can expect the common clock fmwk to use that and even Paul might want to do some stuff with that. Let's start using it. > >>>> + r = omap_clk_get_by_name(clk_name); >>>> + if (IS_ERR(r)) { >>>> + pr_err("omap_device: %s: omap_clk_get_by_name for %s failed\n", >>>> + dev_name(&od->pdev.dev), clk_name); >>>> + return; >>>> + } >>>> + >>>> + l = clkdev_alloc(r, clk_alias, dev_name(&od->pdev.dev)); >>>> + if (!l) { >>>> + pr_err("omap_device: %s: clkdev_alloc for %s failed\n", >>>> + dev_name(&od->pdev.dev), clk_alias); >>> >>> And here. >> >> No, it is not needed in that case because the omap_clk_get_by_name >> is not using the clk_get API. > > Ah, didn't check that one, sorry. That's an unfortunate use of "get" > in the API name IMO. When common struct clk takes over, this may need > some tweaking. Yep, I do agree. Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c index 49fc0df..8a854a3 100644 --- a/arch/arm/plat-omap/omap_device.c +++ b/arch/arm/plat-omap/omap_device.c @@ -241,56 +241,70 @@ static inline struct omap_device *_find_by_pdev(struct platform_device *pdev) return container_of(pdev, struct omap_device, pdev); } +static void _add_clkdev(struct omap_device *od, const char *clk_alias, + const char *clk_name) +{ + struct clk *r; + struct clk_lookup *l; + + if (!clk_alias || !clk_name) + return; + + pr_debug("omap_device: %s: Creating %s -> %s\n", + dev_name(&od->pdev.dev), clk_alias, clk_name); + + r = clk_get_sys(dev_name(&od->pdev.dev), clk_alias); + if (!IS_ERR(r)) { + pr_warning("omap_device: %s: %s already exist\n", + dev_name(&od->pdev.dev), clk_alias); + return; + } + + r = omap_clk_get_by_name(clk_name); + if (IS_ERR(r)) { + pr_err("omap_device: %s: omap_clk_get_by_name for %s failed\n", + dev_name(&od->pdev.dev), clk_name); + return; + } + + l = clkdev_alloc(r, clk_alias, dev_name(&od->pdev.dev)); + if (!l) { + pr_err("omap_device: %s: clkdev_alloc for %s failed\n", + dev_name(&od->pdev.dev), clk_alias); + return; + } + + clkdev_add(l); +} + /** - * _add_optional_clock_clkdev - Add clkdev entry for hwmod optional clocks + * _add_hwmod_clocks_clkdev - Add clkdev entry for hwmod optional clocks + * and main clock * @od: struct omap_device *od + * @oh: struct omap_hwmod *oh * - * For every optional clock present per hwmod per omap_device, this function - * adds an entry in the clkdev table of the form <dev-id=dev_name, con-id=role> - * if it does not exist already. + * For the main clock and every optional clock present per hwmod per + * omap_device, this function adds an entry in the clkdev table of the + * form <dev-id=dev_name, con-id=role> if it does not exist already. * * The function is called from inside omap_device_build_ss(), after * omap_device_register. * * This allows drivers to get a pointer to its optional clocks based on its role * by calling clk_get(<dev*>, <role>). + * In the case of the main clock, a "fck" alias is used. * * No return value. */ -static void _add_optional_clock_clkdev(struct omap_device *od, - struct omap_hwmod *oh) +static void _add_hwmod_clocks_clkdev(struct omap_device *od, + struct omap_hwmod *oh) { int i; - for (i = 0; i < oh->opt_clks_cnt; i++) { - struct omap_hwmod_opt_clk *oc; - struct clk *r; - struct clk_lookup *l; - - oc = &oh->opt_clks[i]; - - if (!oc->_clk) - continue; - - r = clk_get_sys(dev_name(&od->pdev.dev), oc->role); - if (!IS_ERR(r)) - continue; /* clkdev entry exists */ + _add_clkdev(od, "fck", oh->main_clk); - r = omap_clk_get_by_name((char *)oc->clk); - if (IS_ERR(r)) { - pr_err("omap_device: %s: omap_clk_get_by_name for %s failed\n", - dev_name(&od->pdev.dev), oc->clk); - continue; - } - - l = clkdev_alloc(r, oc->role, dev_name(&od->pdev.dev)); - if (!l) { - pr_err("omap_device: %s: clkdev_alloc for %s failed\n", - dev_name(&od->pdev.dev), oc->role); - return; - } - clkdev_add(l); - } + for (i = 0; i < oh->opt_clks_cnt; i++) + _add_clkdev(od, oh->opt_clks[i].role, oh->opt_clks[i].clk); } @@ -497,7 +511,7 @@ struct omap_device *omap_device_build_ss(const char *pdev_name, int pdev_id, for (i = 0; i < oh_cnt; i++) { hwmods[i]->od = od; - _add_optional_clock_clkdev(od, hwmods[i]); + _add_hwmod_clocks_clkdev(od, hwmods[i]); } if (ret)
Extend the existing function to create clkdev for every optional clocks to add a well one "fck" alias for the main_clk of the omap_hwmod. It will allow to remove these static clkdev entries from the clockXXX_data.c file. Signed-off-by: Benoit Cousson <b-cousson@ti.com> Cc: Paul Walmsley <paul@pwsan.com> Cc: Kevin Hilman <khilman@ti.com> --- arch/arm/plat-omap/omap_device.c | 84 ++++++++++++++++++++++---------------- 1 files changed, 49 insertions(+), 35 deletions(-)