Hi Morimoto-san,
On Tuesday 05 November 2013 00:52:29 Kuninori Morimoto wrote:
> Hi Laurent
>
> > +Required Properties:
> > +
> > + - compatible: Must be "renesas,r8a7790-cpg-clocks"
> > + - reg: Base address and length of the memory resource used by the CPG
> > + - clocks: Reference to the parent clock
> > + - #clock-cells: Must be 1
> > + - clock-output-names: The name of the clocks, must be "main", "pll1",
> > + "pll3", "lb", "qspi", "sdh", "sd0", "sd1".
>
> (snip)
>
> > + for (i = 0; i < CPG_NUM_CLOCKS; ++i) {
> > + const struct clk_div_table *table = NULL;
> > + const char *parent_name = "main";
> > + const char *name;
> > + unsigned int shift;
> > + unsigned int mult = 1;
> > + unsigned int div = 1;
> > + struct clk *clk;
> > +
> > + of_property_read_string_index(np, "clock-output-names", i,
> > + &name);
> > +
> > + switch (i) {
> > + case R8A7790_CLK_MAIN:
> > + parent_name = of_clk_get_parent_name(np, 0);
> > + div = config->extal_div;
> > + break;
> > + case R8A7790_CLK_PLL1:
> > + mult = config->pll1_mult / 2;
> > + break;
> > + case R8A7790_CLK_PLL3:
> > + mult = config->pll3_mult;
> > + break;
> > + case R8A7790_CLK_LB:
> > + div = cpg_mode & BIT(18) ? 36 : 24;
> > + break;
> > + case R8A7790_CLK_QSPI:
> > + div = (cpg_mode & (BIT(3) | BIT(2) | BIT(1))) == BIT(2)
> > + ? 16 : 20;
> > + break;
> > + case R8A7790_CLK_SDH:
> > + table = cpg_sdh_div_table;
> > + shift = 8;
> > + break;
> > + case R8A7790_CLK_SD0:
> > + table = cpg_sd01_div_table;
> > + shift = 4;
> > + break;
> > + case R8A7790_CLK_SD1:
> > + table = cpg_sd01_div_table;
> > + shift = 0;
> > + break;
> > + }
>
> Is this clock-output-names realy "Required" property ?
> The "name" and "order" seem fixed, then,
> I guess it can simply use "name array" ?
The clock-output-names property is required by the of_clk_get_parent_name()
function. The property is mandatory for all clocks that need to be referenced
by name, which is the case of all non-leaf clocks on our platforms. We thus
need it.
> > +static void __init r8a7790_cpg_clocks_init(struct device_node *np)
> > +{
> > + const struct cpg_pll_config *config;
> > + struct r8a7790_cpg *cpg;
> > + struct clk **clks;
> > + unsigned int i;
> > +
> > + cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> > + clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> > + if (cpg == NULL || clks == NULL) {
> > + kfree(clks);
> > + kfree(cpg);
> > + pr_err("%s: failed to allocate cpg\n", __func__);
> > + return;
> > + }
> > +
> > + spin_lock_init(&cpg->lock);
> > +
> > + cpg->data.clks = clks;
> > + cpg->data.clk_num = CPG_NUM_CLOCKS;
> > +
> > + cpg->reg = of_iomap(np, 0);
> > + if (WARN_ON(cpg->reg == NULL)) {
> > + kfree(cpg->data.clks);
> > + kfree(cpg);
> > + return;
> > + }
>
> Above 2 error cases are doing same thing.
> If can use goto xxx_error.
Good point.
Given that a failure to allocate memory or ioremap registers will lead to a
boot failure, I wonder whether we couldn't just remove the kfree() calls and
leak memory. Is there a point in cleaning up behind us if the system will no
boot due to missing core clocks anyway ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
