From: Nishanth Menon <nm@ti.com> To: Tero Kristo <t-kristo@ti.com> Cc: paul@pwsan.com, khilman@linaro.org, mturquette@linaro.org, tony@atomide.com, devicetree-discuss@lists.ozlabs.org, rnayak@ti.com, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCHv4 03/33] CLK: OMAP4: Add DPLL clock support Date: Tue, 30 Jul 2013 11:23:31 -0500 [thread overview] Message-ID: <51F7E883.7000000@ti.com> (raw) In-Reply-To: <1374564028-11352-4-git-send-email-t-kristo@ti.com> This patch probably was submitted in the wrong sequence - fails build and few other issues below. On 07/23/2013 02:19 AM, Tero Kristo wrote: > The OMAP clock driver now supports DPLL clock type. This patch also > adds support for DT DPLL nodes. Then why is $subject specific to OMAP4? is that because of of_omap4_dpll_setup? > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/omap/Makefile | 2 +- > drivers/clk/omap/clk.c | 1 + > drivers/clk/omap/dpll.c | 295 +++++++++++++++++++++++++++++++++++++++++++++ Device Tree Binding documentation? > 3 files changed, 297 insertions(+), 1 deletion(-) > create mode 100644 drivers/clk/omap/dpll.c > > diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile > index 8195931..4cad480 100644 > --- a/drivers/clk/omap/Makefile > +++ b/drivers/clk/omap/Makefile > @@ -1 +1 @@ > -obj-y += clk.o > +obj-y += clk.o dpll.o > diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c > index 4bf1929..1dafdaa 100644 > --- a/drivers/clk/omap/clk.c > +++ b/drivers/clk/omap/clk.c > @@ -28,6 +28,7 @@ static const struct of_device_id clk_match[] = { > .data = of_fixed_factor_clk_setup, }, > {.compatible = "divider-clock", .data = of_divider_clk_setup, }, > {.compatible = "gate-clock", .data = of_gate_clk_setup, }, > + {.compatible = "ti,omap4-dpll-clock", .data = of_omap4_dpll_setup, }, > {}, > }; you would not need this if you did just of_clk_init(NULL); ? Further, at this patch, build fails with: drivers/clk/omap/clk.c:31:55: error: undefined identifier 'of_omap4_dpll_setup' drivers/clk/omap/clk.c:31:48: error: ‘of_omap4_dpll_setup’ undeclared here (not in a function) which makes sense since we did not export the function. > > diff --git a/drivers/clk/omap/dpll.c b/drivers/clk/omap/dpll.c > new file mode 100644 > index 0000000..66e82be > --- /dev/null > +++ b/drivers/clk/omap/dpll.c > @@ -0,0 +1,295 @@ > +/* > + * OMAP DPLL clock support > + * > + * Copyright (C) 2013 Texas Instruments, Inc. > + * > + * Tero Kristo <t-kristo@ti.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/io.h> > +#include <linux/err.h> > +#include <linux/string.h> > +#include <linux/log2.h> > +#include <linux/of.h> > +#include <linux/of_address.h> after a quick check, are all these required? > +#include <linux/clk/omap.h> why? > + > +static const struct clk_ops dpll_m4xen_ck_ops = { > + .enable = &omap3_noncore_dpll_enable, > + .disable = &omap3_noncore_dpll_disable, > + .recalc_rate = &omap4_dpll_regm4xen_recalc, > + .round_rate = &omap4_dpll_regm4xen_round_rate, > + .set_rate = &omap3_noncore_dpll_set_rate, > + .get_parent = &omap2_init_dpll_parent, > +}; > + > +static const struct clk_ops dpll_core_ck_ops = { > + .recalc_rate = &omap3_dpll_recalc, > + .get_parent = &omap2_init_dpll_parent, > +}; > + > +static const struct clk_ops dpll_ck_ops = { > + .enable = &omap3_noncore_dpll_enable, > + .disable = &omap3_noncore_dpll_disable, > + .recalc_rate = &omap3_dpll_recalc, > + .round_rate = &omap2_dpll_round_rate, > + .set_rate = &omap3_noncore_dpll_set_rate, > + .get_parent = &omap2_init_dpll_parent, > + .init = &omap2_init_clk_clkdm, > +}; > + > +static const struct clk_ops dpll_x2_ck_ops = { > + .recalc_rate = &omap3_clkoutx2_recalc, > +}; none of these are defined at this stage of the patch, generates a huge build error for dpll.c http://pastebin.com/GJucv1A5 > + > +struct clk *omap_clk_register_dpll(struct device *dev, const char *name, > + const char **parent_names, int num_parents, unsigned long flags, > + struct dpll_data *dpll_data, const char *clkdm_name, > + const struct clk_ops *ops) why should this be public? > +{ > + struct clk *clk; > + struct clk_init_data init; init = { 0 }; just to future proof? > + struct clk_hw_omap *clk_hw; does not exist yet in generic header? I am assuming you do not do parameter check as you expect clk_register to do the same? > + > + /* allocate the divider */ > + clk_hw = kzalloc(sizeof(struct clk_hw_omap), GFP_KERNEL); checkpatch complained: CHECK: Prefer kzalloc(sizeof(*clk_hw)...) over kzalloc(sizeof(struct clk_hw_omap)...) or given we have dev, devm_kzalloc? > + if (!clk_hw) { > + pr_err("%s: could not allocate clk_hw_omap\n", __func__); > + return ERR_PTR(-ENOMEM); > + } > + > + clk_hw->dpll_data = dpll_data; > + clk_hw->ops = &clkhwops_omap3_dpll; > + clk_hw->clkdm_name = clkdm_name; > + clk_hw->hw.init = &init; > + > + init.name = name; > + init.ops = ops; > + init.flags = flags; > + init.parent_names = parent_names; > + init.num_parents = num_parents; > + > + /* register the clock */ > + clk = clk_register(dev, &clk_hw->hw); > + > + if (IS_ERR(clk)) > + kfree(clk_hw); > + else > + omap2_init_clk_hw_omap_clocks(clk); what if init fails? and it is in mach-omap2 at this point in time? > + > + return clk; > +} > + > +struct clk *omap_clk_register_dpll_x2(struct device *dev, const char *name, > + const char *parent_name, void __iomem *reg, > + const struct clk_ops *ops) same question here as well > +{ > + struct clk *clk; > + struct clk_init_data init; > + struct clk_hw_omap *clk_hw; > + > + if (!parent_name) { > + pr_err("%s: dpll_x2 must have parent\n", __func__); > + return ERR_PTR(-EINVAL); > + } > + > + clk_hw = kzalloc(sizeof(struct clk_hw_omap), GFP_KERNEL); checkpatch complained: CHECK: Prefer kzalloc(sizeof(*clk_hw)...) over kzalloc(sizeof(struct clk_hw_omap)...) or devm_kzalloc? > + if (!clk_hw) { > + pr_err("%s: could not allocate clk_hw_omap\n", __func__); > + return ERR_PTR(-ENOMEM); > + } > + > + clk_hw->ops = &clkhwops_omap4_dpllmx; > + clk_hw->clksel_reg = reg; > + clk_hw->hw.init = &init; > + > + init.name = name; > + init.ops = ops; > + init.parent_names = &parent_name; > + init.num_parents = 1; > + > + /* register the clock */ > + clk = clk_register(dev, &clk_hw->hw); > + > + if (IS_ERR(clk)) > + kfree(clk_hw); > + else > + omap2_init_clk_hw_omap_clocks(clk); > + > + return clk; > +} this vaguely sounds like a replica of omap_clk_register_dpll with num_parents and clk_hw->ops different. why not merge the two? > + > +#ifdef CONFIG_OF why not build the entire thing *iff* CONFIG_OF (Makefile/Kconfig dep)? that way, we can drop this #ifdef stuff from drivers that dont need to have dual support. > + > +/** > + * of_omap_dpll_setup() - Setup function for OMAP DPLL clocks node and ops not documented. > + */ > +static void __init of_omap_dpll_setup(struct device_node *node, > + const struct clk_ops *ops) > +{ > + struct clk *clk; > + const char *clk_name = node->name; > + int num_parents; > + const char **parent_names; > + const char *clkdm_name = NULL; > + struct of_phandle_args clkspec; > + u8 dpll_flags = 0; > + struct dpll_data *dd; > + u32 idlest_mask = 0x1; > + u32 enable_mask = 0x7; > + u32 autoidle_mask = 0x7; > + u32 mult_mask = 0x7ff << 8; > + u32 div1_mask = 0x7f; > + u32 max_multiplier = 2047; > + u32 max_divider = 128; > + u32 min_divider = 1; > + int i; > + > + dd = kzalloc(sizeof(struct dpll_data), GFP_KERNEL); kzalloc sizeof(*dd) ? > + if (!dd) { > + pr_err("%s: could not allocate dpll_data\n", __func__); > + return; > + } > + > + of_property_read_string(node, "clock-output-names", &clk_name); > + > + num_parents = of_clk_get_parent_count(node); > + if (num_parents < 1) { > + pr_err("%s: omap dpll %s must have parent(s)\n", > + __func__, node->name); checkpatch complained: CHECK: Alignment should match open parenthesis #212: FILE: drivers/clk/omap/dpll.c:171: After applying the patch, I think you should make __func__ aligned with " and not % > + goto cleanup; > + } > + > + parent_names = kzalloc(sizeof(char *) * num_parents, GFP_KERNEL); > + > + for (i = 0; i < num_parents; i++) > + parent_names[i] = of_clk_get_parent_name(node, i); > + > + of_property_read_u32(node, "ti,idlest-mask", &idlest_mask); > + > + of_property_read_u32(node, "ti,enable-mask", &enable_mask); > + > + of_property_read_u32(node, "ti,autoidle-mask", &autoidle_mask); are these going to be different? or can we catch with compatible flag? > + > + clkspec.np = of_parse_phandle(node, "ti,clk-ref", 0); > + dd->clk_ref = of_clk_get_from_provider(&clkspec); > + if (!dd->clk_ref) { > + pr_err("%s: ti,clk-ref for %s not found\n", __func__, > + clk_name); CHECK: Alignment should match open parenthesis #231: FILE: drivers/clk/omap/dpll.c:190: similar issue here. > + goto cleanup; > + } > + > + clkspec.np = of_parse_phandle(node, "ti,clk-bypass", 0); > + dd->clk_bypass = of_clk_get_from_provider(&clkspec); > + if (!dd->clk_bypass) { > + pr_err("%s: ti,clk-bypass for %s not found\n", __func__, > + clk_name); here as well > + goto cleanup; > + } > + > + of_property_read_string(node, "ti,clkdm-name", &clkdm_name); > + > + dd->control_reg = of_iomap(node, 0); > + dd->idlest_reg = of_iomap(node, 1); > + dd->autoidle_reg = of_iomap(node, 2); > + dd->mult_div1_reg = of_iomap(node, 3); if dts has errors, should we not verify mandatory parameters? > + > + dd->idlest_mask = idlest_mask; > + dd->enable_mask = enable_mask; > + dd->autoidle_mask = autoidle_mask; > + > + dd->modes = 0xa0; what is 0xa0? > + > + if (of_property_read_bool(node, "ti,dpll-j-type")) { > + dd->sddiv_mask = 0xff000000; > + mult_mask = 0xfff << 8; > + div1_mask = 0xff; > + max_multiplier = 4095; > + max_divider = 256; > + } > + > + if (of_property_read_bool(node, "ti,dpll-regm4xen")) { I think we need bindings to understand this better. > + dd->m4xen_mask = 0x800; > + dd->lpmode_mask = 1 << 10; > + } > + > + dd->mult_mask = mult_mask; > + dd->div1_mask = div1_mask; > + dd->max_multiplier = max_multiplier; > + dd->max_divider = max_divider; > + dd->min_divider = min_divider; > + > + clk = omap_clk_register_dpll(NULL, clk_name, parent_names, > + num_parents, dpll_flags, dd, > + clkdm_name, ops); > + > + if (!IS_ERR(clk)) > + of_clk_add_provider(node, of_clk_src_simple_get, clk); error check? > + return; > + > +cleanup: kfree(parent_names) ? > + kfree(dd); > + return; > +} > + > +static void __init of_omap_dpll_x2_setup(struct device_node *node) > +{ > + struct clk *clk; > + const char *clk_name = node->name; > + void __iomem *reg; > + const char *parent_name; > + > + of_property_read_string(node, "clock-output-names", &clk_name); > + > + parent_name = of_clk_get_parent_name(node, 0); > + > + reg = of_iomap(node, 0); if dts has errors, should we not verify mandatory parameters? > + > + clk = omap_clk_register_dpll_x2(NULL, clk_name, parent_name, > + reg, &dpll_x2_ck_ops); > + > + if (!IS_ERR(clk)) > + of_clk_add_provider(node, of_clk_src_simple_get, clk); error check? gentle request - in this setup function we dont see a return of error value (which makes sense), but more importantly - log saying that node was not setup > +} > + > +__init void of_omap3_dpll_setup(struct device_node *node) ^^ void __init? further, you could make this static. > +{ > + /* XXX: to be done */ > +} > +EXPORT_SYMBOL_GPL(of_omap3_dpll_setup); you can drop the export if you use of_clk_init(NULL); > +CLK_OF_DECLARE(omap3_dpll_clock, "ti,omap3-dpll-clock", of_omap3_dpll_setup); > + > +__init void of_omap4_dpll_setup(struct device_node *node) ^^ void __init? further, you could make this static. > +{ > + const struct clk_ops *ops; > + > + ops = &dpll_ck_ops; > + > + if (of_property_read_bool(node, "ti,dpll-regm4xen")) > + ops = &dpll_m4xen_ck_ops; > + > + if (of_property_read_bool(node, "ti,dpll-core")) > + ops = &dpll_core_ck_ops; > + > + if (of_property_read_bool(node, "ti,dpll-clk-x2")) { > + of_omap_dpll_x2_setup(node); > + return; > + } > + > + of_omap_dpll_setup(node, ops); > +} > +EXPORT_SYMBOL_GPL(of_omap4_dpll_setup); you can drop the export if you use of_clk_init(NULL); > +CLK_OF_DECLARE(omap4_dpll_clock, "ti,omap4-dpll-clock", of_omap4_dpll_setup); > +#endif > -- Regards, Nishanth Menon
WARNING: multiple messages have this Message-ID (diff)
From: nm@ti.com (Nishanth Menon) To: linux-arm-kernel@lists.infradead.org Subject: [PATCHv4 03/33] CLK: OMAP4: Add DPLL clock support Date: Tue, 30 Jul 2013 11:23:31 -0500 [thread overview] Message-ID: <51F7E883.7000000@ti.com> (raw) In-Reply-To: <1374564028-11352-4-git-send-email-t-kristo@ti.com> This patch probably was submitted in the wrong sequence - fails build and few other issues below. On 07/23/2013 02:19 AM, Tero Kristo wrote: > The OMAP clock driver now supports DPLL clock type. This patch also > adds support for DT DPLL nodes. Then why is $subject specific to OMAP4? is that because of of_omap4_dpll_setup? > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/clk/omap/Makefile | 2 +- > drivers/clk/omap/clk.c | 1 + > drivers/clk/omap/dpll.c | 295 +++++++++++++++++++++++++++++++++++++++++++++ Device Tree Binding documentation? > 3 files changed, 297 insertions(+), 1 deletion(-) > create mode 100644 drivers/clk/omap/dpll.c > > diff --git a/drivers/clk/omap/Makefile b/drivers/clk/omap/Makefile > index 8195931..4cad480 100644 > --- a/drivers/clk/omap/Makefile > +++ b/drivers/clk/omap/Makefile > @@ -1 +1 @@ > -obj-y += clk.o > +obj-y += clk.o dpll.o > diff --git a/drivers/clk/omap/clk.c b/drivers/clk/omap/clk.c > index 4bf1929..1dafdaa 100644 > --- a/drivers/clk/omap/clk.c > +++ b/drivers/clk/omap/clk.c > @@ -28,6 +28,7 @@ static const struct of_device_id clk_match[] = { > .data = of_fixed_factor_clk_setup, }, > {.compatible = "divider-clock", .data = of_divider_clk_setup, }, > {.compatible = "gate-clock", .data = of_gate_clk_setup, }, > + {.compatible = "ti,omap4-dpll-clock", .data = of_omap4_dpll_setup, }, > {}, > }; you would not need this if you did just of_clk_init(NULL); ? Further, at this patch, build fails with: drivers/clk/omap/clk.c:31:55: error: undefined identifier 'of_omap4_dpll_setup' drivers/clk/omap/clk.c:31:48: error: ?of_omap4_dpll_setup? undeclared here (not in a function) which makes sense since we did not export the function. > > diff --git a/drivers/clk/omap/dpll.c b/drivers/clk/omap/dpll.c > new file mode 100644 > index 0000000..66e82be > --- /dev/null > +++ b/drivers/clk/omap/dpll.c > @@ -0,0 +1,295 @@ > +/* > + * OMAP DPLL clock support > + * > + * Copyright (C) 2013 Texas Instruments, Inc. > + * > + * Tero Kristo <t-kristo@ti.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/io.h> > +#include <linux/err.h> > +#include <linux/string.h> > +#include <linux/log2.h> > +#include <linux/of.h> > +#include <linux/of_address.h> after a quick check, are all these required? > +#include <linux/clk/omap.h> why? > + > +static const struct clk_ops dpll_m4xen_ck_ops = { > + .enable = &omap3_noncore_dpll_enable, > + .disable = &omap3_noncore_dpll_disable, > + .recalc_rate = &omap4_dpll_regm4xen_recalc, > + .round_rate = &omap4_dpll_regm4xen_round_rate, > + .set_rate = &omap3_noncore_dpll_set_rate, > + .get_parent = &omap2_init_dpll_parent, > +}; > + > +static const struct clk_ops dpll_core_ck_ops = { > + .recalc_rate = &omap3_dpll_recalc, > + .get_parent = &omap2_init_dpll_parent, > +}; > + > +static const struct clk_ops dpll_ck_ops = { > + .enable = &omap3_noncore_dpll_enable, > + .disable = &omap3_noncore_dpll_disable, > + .recalc_rate = &omap3_dpll_recalc, > + .round_rate = &omap2_dpll_round_rate, > + .set_rate = &omap3_noncore_dpll_set_rate, > + .get_parent = &omap2_init_dpll_parent, > + .init = &omap2_init_clk_clkdm, > +}; > + > +static const struct clk_ops dpll_x2_ck_ops = { > + .recalc_rate = &omap3_clkoutx2_recalc, > +}; none of these are defined at this stage of the patch, generates a huge build error for dpll.c http://pastebin.com/GJucv1A5 > + > +struct clk *omap_clk_register_dpll(struct device *dev, const char *name, > + const char **parent_names, int num_parents, unsigned long flags, > + struct dpll_data *dpll_data, const char *clkdm_name, > + const struct clk_ops *ops) why should this be public? > +{ > + struct clk *clk; > + struct clk_init_data init; init = { 0 }; just to future proof? > + struct clk_hw_omap *clk_hw; does not exist yet in generic header? I am assuming you do not do parameter check as you expect clk_register to do the same? > + > + /* allocate the divider */ > + clk_hw = kzalloc(sizeof(struct clk_hw_omap), GFP_KERNEL); checkpatch complained: CHECK: Prefer kzalloc(sizeof(*clk_hw)...) over kzalloc(sizeof(struct clk_hw_omap)...) or given we have dev, devm_kzalloc? > + if (!clk_hw) { > + pr_err("%s: could not allocate clk_hw_omap\n", __func__); > + return ERR_PTR(-ENOMEM); > + } > + > + clk_hw->dpll_data = dpll_data; > + clk_hw->ops = &clkhwops_omap3_dpll; > + clk_hw->clkdm_name = clkdm_name; > + clk_hw->hw.init = &init; > + > + init.name = name; > + init.ops = ops; > + init.flags = flags; > + init.parent_names = parent_names; > + init.num_parents = num_parents; > + > + /* register the clock */ > + clk = clk_register(dev, &clk_hw->hw); > + > + if (IS_ERR(clk)) > + kfree(clk_hw); > + else > + omap2_init_clk_hw_omap_clocks(clk); what if init fails? and it is in mach-omap2 at this point in time? > + > + return clk; > +} > + > +struct clk *omap_clk_register_dpll_x2(struct device *dev, const char *name, > + const char *parent_name, void __iomem *reg, > + const struct clk_ops *ops) same question here as well > +{ > + struct clk *clk; > + struct clk_init_data init; > + struct clk_hw_omap *clk_hw; > + > + if (!parent_name) { > + pr_err("%s: dpll_x2 must have parent\n", __func__); > + return ERR_PTR(-EINVAL); > + } > + > + clk_hw = kzalloc(sizeof(struct clk_hw_omap), GFP_KERNEL); checkpatch complained: CHECK: Prefer kzalloc(sizeof(*clk_hw)...) over kzalloc(sizeof(struct clk_hw_omap)...) or devm_kzalloc? > + if (!clk_hw) { > + pr_err("%s: could not allocate clk_hw_omap\n", __func__); > + return ERR_PTR(-ENOMEM); > + } > + > + clk_hw->ops = &clkhwops_omap4_dpllmx; > + clk_hw->clksel_reg = reg; > + clk_hw->hw.init = &init; > + > + init.name = name; > + init.ops = ops; > + init.parent_names = &parent_name; > + init.num_parents = 1; > + > + /* register the clock */ > + clk = clk_register(dev, &clk_hw->hw); > + > + if (IS_ERR(clk)) > + kfree(clk_hw); > + else > + omap2_init_clk_hw_omap_clocks(clk); > + > + return clk; > +} this vaguely sounds like a replica of omap_clk_register_dpll with num_parents and clk_hw->ops different. why not merge the two? > + > +#ifdef CONFIG_OF why not build the entire thing *iff* CONFIG_OF (Makefile/Kconfig dep)? that way, we can drop this #ifdef stuff from drivers that dont need to have dual support. > + > +/** > + * of_omap_dpll_setup() - Setup function for OMAP DPLL clocks node and ops not documented. > + */ > +static void __init of_omap_dpll_setup(struct device_node *node, > + const struct clk_ops *ops) > +{ > + struct clk *clk; > + const char *clk_name = node->name; > + int num_parents; > + const char **parent_names; > + const char *clkdm_name = NULL; > + struct of_phandle_args clkspec; > + u8 dpll_flags = 0; > + struct dpll_data *dd; > + u32 idlest_mask = 0x1; > + u32 enable_mask = 0x7; > + u32 autoidle_mask = 0x7; > + u32 mult_mask = 0x7ff << 8; > + u32 div1_mask = 0x7f; > + u32 max_multiplier = 2047; > + u32 max_divider = 128; > + u32 min_divider = 1; > + int i; > + > + dd = kzalloc(sizeof(struct dpll_data), GFP_KERNEL); kzalloc sizeof(*dd) ? > + if (!dd) { > + pr_err("%s: could not allocate dpll_data\n", __func__); > + return; > + } > + > + of_property_read_string(node, "clock-output-names", &clk_name); > + > + num_parents = of_clk_get_parent_count(node); > + if (num_parents < 1) { > + pr_err("%s: omap dpll %s must have parent(s)\n", > + __func__, node->name); checkpatch complained: CHECK: Alignment should match open parenthesis #212: FILE: drivers/clk/omap/dpll.c:171: After applying the patch, I think you should make __func__ aligned with " and not % > + goto cleanup; > + } > + > + parent_names = kzalloc(sizeof(char *) * num_parents, GFP_KERNEL); > + > + for (i = 0; i < num_parents; i++) > + parent_names[i] = of_clk_get_parent_name(node, i); > + > + of_property_read_u32(node, "ti,idlest-mask", &idlest_mask); > + > + of_property_read_u32(node, "ti,enable-mask", &enable_mask); > + > + of_property_read_u32(node, "ti,autoidle-mask", &autoidle_mask); are these going to be different? or can we catch with compatible flag? > + > + clkspec.np = of_parse_phandle(node, "ti,clk-ref", 0); > + dd->clk_ref = of_clk_get_from_provider(&clkspec); > + if (!dd->clk_ref) { > + pr_err("%s: ti,clk-ref for %s not found\n", __func__, > + clk_name); CHECK: Alignment should match open parenthesis #231: FILE: drivers/clk/omap/dpll.c:190: similar issue here. > + goto cleanup; > + } > + > + clkspec.np = of_parse_phandle(node, "ti,clk-bypass", 0); > + dd->clk_bypass = of_clk_get_from_provider(&clkspec); > + if (!dd->clk_bypass) { > + pr_err("%s: ti,clk-bypass for %s not found\n", __func__, > + clk_name); here as well > + goto cleanup; > + } > + > + of_property_read_string(node, "ti,clkdm-name", &clkdm_name); > + > + dd->control_reg = of_iomap(node, 0); > + dd->idlest_reg = of_iomap(node, 1); > + dd->autoidle_reg = of_iomap(node, 2); > + dd->mult_div1_reg = of_iomap(node, 3); if dts has errors, should we not verify mandatory parameters? > + > + dd->idlest_mask = idlest_mask; > + dd->enable_mask = enable_mask; > + dd->autoidle_mask = autoidle_mask; > + > + dd->modes = 0xa0; what is 0xa0? > + > + if (of_property_read_bool(node, "ti,dpll-j-type")) { > + dd->sddiv_mask = 0xff000000; > + mult_mask = 0xfff << 8; > + div1_mask = 0xff; > + max_multiplier = 4095; > + max_divider = 256; > + } > + > + if (of_property_read_bool(node, "ti,dpll-regm4xen")) { I think we need bindings to understand this better. > + dd->m4xen_mask = 0x800; > + dd->lpmode_mask = 1 << 10; > + } > + > + dd->mult_mask = mult_mask; > + dd->div1_mask = div1_mask; > + dd->max_multiplier = max_multiplier; > + dd->max_divider = max_divider; > + dd->min_divider = min_divider; > + > + clk = omap_clk_register_dpll(NULL, clk_name, parent_names, > + num_parents, dpll_flags, dd, > + clkdm_name, ops); > + > + if (!IS_ERR(clk)) > + of_clk_add_provider(node, of_clk_src_simple_get, clk); error check? > + return; > + > +cleanup: kfree(parent_names) ? > + kfree(dd); > + return; > +} > + > +static void __init of_omap_dpll_x2_setup(struct device_node *node) > +{ > + struct clk *clk; > + const char *clk_name = node->name; > + void __iomem *reg; > + const char *parent_name; > + > + of_property_read_string(node, "clock-output-names", &clk_name); > + > + parent_name = of_clk_get_parent_name(node, 0); > + > + reg = of_iomap(node, 0); if dts has errors, should we not verify mandatory parameters? > + > + clk = omap_clk_register_dpll_x2(NULL, clk_name, parent_name, > + reg, &dpll_x2_ck_ops); > + > + if (!IS_ERR(clk)) > + of_clk_add_provider(node, of_clk_src_simple_get, clk); error check? gentle request - in this setup function we dont see a return of error value (which makes sense), but more importantly - log saying that node was not setup > +} > + > +__init void of_omap3_dpll_setup(struct device_node *node) ^^ void __init? further, you could make this static. > +{ > + /* XXX: to be done */ > +} > +EXPORT_SYMBOL_GPL(of_omap3_dpll_setup); you can drop the export if you use of_clk_init(NULL); > +CLK_OF_DECLARE(omap3_dpll_clock, "ti,omap3-dpll-clock", of_omap3_dpll_setup); > + > +__init void of_omap4_dpll_setup(struct device_node *node) ^^ void __init? further, you could make this static. > +{ > + const struct clk_ops *ops; > + > + ops = &dpll_ck_ops; > + > + if (of_property_read_bool(node, "ti,dpll-regm4xen")) > + ops = &dpll_m4xen_ck_ops; > + > + if (of_property_read_bool(node, "ti,dpll-core")) > + ops = &dpll_core_ck_ops; > + > + if (of_property_read_bool(node, "ti,dpll-clk-x2")) { > + of_omap_dpll_x2_setup(node); > + return; > + } > + > + of_omap_dpll_setup(node, ops); > +} > +EXPORT_SYMBOL_GPL(of_omap4_dpll_setup); you can drop the export if you use of_clk_init(NULL); > +CLK_OF_DECLARE(omap4_dpll_clock, "ti,omap4-dpll-clock", of_omap4_dpll_setup); > +#endif > -- Regards, Nishanth Menon
next prev parent reply other threads:[~2013-07-30 16:23 UTC|newest] Thread overview: 204+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-07-23 7:19 [PATCHv4 00/33] ARM: OMAP: clock conversion to DT Tero Kristo 2013-07-23 7:19 ` Tero Kristo 2013-07-23 7:19 ` [PATCHv4 01/33] CLK: clkdev: add support for looking up clocks from DT Tero Kristo 2013-07-23 7:19 ` Tero Kristo 2013-07-30 15:04 ` Nishanth Menon 2013-07-30 15:04 ` Nishanth Menon 2013-07-31 8:43 ` Tero Kristo 2013-07-31 8:43 ` Tero Kristo 2013-07-23 7:19 ` [PATCHv4 02/33] clk: omap: introduce clock driver Tero Kristo 2013-07-23 7:19 ` Tero Kristo 2013-07-30 15:21 ` Nishanth Menon 2013-07-30 15:21 ` Nishanth Menon 2013-07-31 8:59 ` Tero Kristo 2013-07-31 8:59 ` Tero Kristo 2013-08-01 13:44 ` Nishanth Menon 2013-08-01 13:44 ` Nishanth Menon 2013-08-01 14:59 ` Tero Kristo 2013-08-01 14:59 ` Tero Kristo 2013-07-23 7:19 ` [PATCHv4 03/33] CLK: OMAP4: Add DPLL clock support Tero Kristo 2013-07-23 7:19 ` Tero Kristo 2013-07-30 16:23 ` Nishanth Menon [this message] 2013-07-30 16:23 ` Nishanth Menon 2013-07-31 9:46 ` Tero Kristo 2013-07-31 9:46 ` Tero Kristo 2013-08-01 14:00 ` Nishanth Menon 2013-08-01 14:00 ` Nishanth Menon 2013-08-01 15:08 ` Tero Kristo 2013-08-01 15:08 ` Tero Kristo 2013-08-01 15:13 ` Nishanth Menon 2013-08-01 15:13 ` Nishanth Menon 2013-08-01 8:29 ` Rajendra Nayak 2013-08-01 8:29 ` Rajendra Nayak 2013-08-01 15:10 ` Nishanth Menon 2013-08-01 15:10 ` Nishanth Menon 2013-08-01 15:41 ` Tero Kristo 2013-08-01 15:41 ` Tero Kristo 2013-07-23 7:19 ` [PATCHv4 04/33] CLK: omap: move part of the machine specific clock header contents to driver Tero Kristo 2013-07-23 7:19 ` Tero Kristo 2013-07-30 18:22 ` Nishanth Menon 2013-07-30 18:22 ` Nishanth Menon 2013-07-31 9:59 ` Tero Kristo 2013-07-31 9:59 ` Tero Kristo 2013-08-01 14:04 ` Nishanth Menon 2013-08-01 14:04 ` Nishanth Menon 2013-08-01 15:12 ` Tero Kristo 2013-08-01 15:12 ` Tero Kristo 2013-08-01 15:21 ` Nishanth Menon 2013-08-01 15:21 ` Nishanth Menon 2013-07-23 7:20 ` [PATCHv4 05/33] CLK: omap: add DT duplicate clock registration mechanism Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 18:40 ` Nishanth Menon 2013-07-30 18:40 ` Nishanth Menon 2013-07-31 10:07 ` Tero Kristo 2013-07-31 10:07 ` Tero Kristo 2013-08-01 14:25 ` Nishanth Menon 2013-08-01 14:25 ` Nishanth Menon 2013-08-01 15:18 ` Tero Kristo 2013-08-01 15:18 ` Tero Kristo 2013-08-01 15:24 ` Nishanth Menon 2013-08-01 15:24 ` Nishanth Menon 2013-08-01 15:30 ` Tero Kristo 2013-08-01 15:30 ` Tero Kristo 2013-08-02 7:22 ` Tony Lindgren 2013-08-02 7:22 ` Tony Lindgren 2013-07-23 7:20 ` [PATCHv4 06/33] CLK: omap: add autoidle support Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 18:56 ` Nishanth Menon 2013-07-30 18:56 ` Nishanth Menon 2013-07-31 10:13 ` Tero Kristo 2013-07-31 10:13 ` Tero Kristo 2013-08-01 14:11 ` Nishanth Menon 2013-08-01 14:11 ` Nishanth Menon 2013-08-01 15:22 ` Tero Kristo 2013-08-01 15:22 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 07/33] CLK: omap: add support for OMAP gate clock Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 19:17 ` Nishanth Menon 2013-07-30 19:17 ` Nishanth Menon 2013-07-31 14:45 ` Tero Kristo 2013-07-31 14:45 ` Tero Kristo 2013-08-01 14:33 ` Nishanth Menon 2013-08-01 14:33 ` Nishanth Menon 2013-08-01 15:29 ` Tero Kristo 2013-08-01 15:29 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 08/33] ARM: dts: omap4 clock data Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 19:27 ` Nishanth Menon 2013-07-30 19:27 ` Nishanth Menon 2013-07-31 14:49 ` Tero Kristo 2013-07-31 14:49 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 09/33] CLK: omap: add omap4 clock init file Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 19:33 ` Nishanth Menon 2013-07-30 19:33 ` Nishanth Menon 2013-07-31 14:52 ` Tero Kristo 2013-07-31 14:52 ` Tero Kristo 2013-08-01 14:40 ` Nishanth Menon 2013-08-01 14:40 ` Nishanth Menon 2013-08-01 15:34 ` Tero Kristo 2013-08-01 15:34 ` Tero Kristo 2013-08-01 16:10 ` Nishanth Menon 2013-08-01 16:10 ` Nishanth Menon 2013-07-23 7:20 ` [PATCHv4 10/33] ARM: OMAP4: remove old clock data and link in new clock init code Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 19:42 ` Nishanth Menon 2013-07-30 19:42 ` Nishanth Menon 2013-07-31 14:55 ` Tero Kristo 2013-07-31 14:55 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 11/33] ARM: dts: omap5 clock data Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 12/33] CLK: omap: add omap5 clock init file Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 13/33] ARM: dts: dra7 clock data Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 14/33] CLK: omap: add dra7 clock init file Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 15/33] CLK: OMAP: DPLL: add support for DT property ti,dpll-no-gate Tero Kristo 2013-07-23 7:20 ` [PATCHv4 15/33] CLK: OMAP: DPLL: add support for DT property ti, dpll-no-gate Tero Kristo 2013-07-30 19:18 ` [PATCHv4 15/33] CLK: OMAP: DPLL: add support for DT property ti,dpll-no-gate Nishanth Menon 2013-07-30 19:18 ` Nishanth Menon 2013-07-31 14:56 ` Tero Kristo 2013-07-31 14:56 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 16/33] CLK: OMAP: DPLL: do not of_iomap NULL autoidle register Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 19:49 ` Nishanth Menon 2013-07-30 19:49 ` Nishanth Menon 2013-07-31 14:57 ` Tero Kristo 2013-07-31 14:57 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 17/33] CLK: DT: add support for set-rate-parent flag Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 19:58 ` Nishanth Menon 2013-07-30 19:58 ` Nishanth Menon 2013-07-23 7:20 ` [PATCHv4 18/33] ARM: dts: am33xx clock data Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 19/33] CLK: omap: add am33xx clock init file Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 20:00 ` Nishanth Menon 2013-07-30 20:00 ` Nishanth Menon 2013-07-31 14:59 ` Tero Kristo 2013-07-31 14:59 ` Tero Kristo 2013-08-01 14:43 ` Nishanth Menon 2013-08-01 14:43 ` Nishanth Menon 2013-08-01 15:35 ` Tero Kristo 2013-08-01 15:35 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 20/33] ARM: AM33xx: remove old clock data and link in new clock init code Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 21/33] CLK: OMAP: DPLL: add omap3 dpll support Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 20:08 ` Nishanth Menon 2013-07-30 20:08 ` Nishanth Menon 2013-07-31 15:03 ` Tero Kristo 2013-07-31 15:03 ` Tero Kristo 2013-08-01 14:46 ` Nishanth Menon 2013-08-01 14:46 ` Nishanth Menon 2013-07-23 7:20 ` [PATCHv4 22/33] CLK: OMAP: update gate clock setup for OMAP3 Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 20:13 ` Nishanth Menon 2013-07-30 20:13 ` Nishanth Menon 2013-07-31 15:05 ` Tero Kristo 2013-07-31 15:05 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 23/33] CLK: OMAP: add interface clock support " Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-30 20:23 ` Nishanth Menon 2013-07-30 20:23 ` Nishanth Menon 2013-07-31 15:09 ` Tero Kristo 2013-07-31 15:09 ` Tero Kristo 2013-08-01 14:50 ` Nishanth Menon 2013-08-01 14:50 ` Nishanth Menon 2013-07-23 7:20 ` [PATCHv4 24/33] CLK: OMAP: move some defines from machine to driver header Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 25/33] ARM: OMAP: hwmod: fix an incorrect clk type cast with _get_clkdm Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 26/33] CLK: omap: gate: add support for OMAP36xx dpllx_mx_ck:s Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 27/33] ARM: OMAP3: hwmod: initialize clkdm from clkdm_name Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 28/33] ARM: dts: omap3 clock data Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 30/33] clk: OMAP: DRA7: Add APLL support Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 31/33] ARM: dts: clk: Add apll related clocks Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 32/33] clk: OMAP: DRA7: Change apll_pcie_m2_ck to fixed factor clock Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 7:20 ` [PATCHv4 33/33] clk: DTS: DRA7: Add PCIe related clock nodes Tero Kristo 2013-07-23 7:20 ` Tero Kristo 2013-07-23 8:24 ` [PATCHv4 00/33] ARM: OMAP: clock conversion to DT Tero Kristo 2013-07-23 8:24 ` Tero Kristo 2013-07-24 14:16 ` Roger Quadros 2013-07-24 14:16 ` Roger Quadros 2013-07-24 14:29 ` Tero Kristo 2013-07-24 14:29 ` Tero Kristo 2013-07-24 14:34 ` Roger Quadros 2013-07-24 14:34 ` Roger Quadros 2013-07-24 14:43 ` Tero Kristo 2013-07-24 14:43 ` Tero Kristo [not found] ` <1374564028-11352-30-git-send-email-t-kristo@ti.com> 2013-07-30 20:19 ` [PATCHv4 29/33] CLK: omap: add omap3 clock init file Nishanth Menon 2013-07-30 20:19 ` Nishanth Menon 2013-07-31 6:35 ` Tony Lindgren 2013-07-31 6:35 ` Tony Lindgren 2013-07-31 15:10 ` Tero Kristo 2013-07-31 15:10 ` Tero Kristo 2013-08-02 7:24 ` Tony Lindgren 2013-08-02 7:24 ` Tony Lindgren
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=51F7E883.7000000@ti.com \ --to=nm@ti.com \ --cc=devicetree-discuss@lists.ozlabs.org \ --cc=khilman@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=mturquette@linaro.org \ --cc=paul@pwsan.com \ --cc=rnayak@ti.com \ --cc=t-kristo@ti.com \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.