On Tue, Aug 30, 2016 at 9:19 PM, Stephen Boyd [off-list ref] wrote:

quoted

+             return PTR_ERR(dwmac->m250_mux_clk);
+
+     /* create the m250_div */
+     snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev));
+     init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
+     init.ops = &clk_divider_ops;
+     init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+     clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk);
+     init.parent_names = clk_div_parents;
+     init.num_parents = ARRAY_SIZE(clk_div_parents);
+
+     dwmac->m250_div.reg = dwmac->regs + PRG_ETH0;
+     dwmac->m250_div.shift = PRG_ETH0_CLK_M250_DIV_SHIFT;
+     dwmac->m250_div.width = PRG_ETH0_CLK_M250_DIV_WIDTH;
+     dwmac->m250_div.hw.init = &init;
+     dwmac->m250_div.flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO;
+
+     dwmac->m250_div_clk = devm_clk_register(dev, &dwmac->m250_div.hw);

We've been trying to move away from devm_clk_register() to
devm_clk_hw_register() so that clk providers aren't also clk
consumers. Obviously in this case this driver is a provider and a
consumer, so this isn't as important. Kevin did something similar
in the mmc driver, so I'll reiterate what I said on that patch.
Perhaps we should make __clk_create_clk() into a real clk
provider API so that we can use devm_clk_hw_register() here and
then generate a clk for this device. That would allow us to have
proper consumer tracking without relying on the clk that is
returned from clk_register() (the intent is to make that clk
instance internal to the framework).

please correct me if I'm wrong but I read this as "this code is OK for
now, but it should be changed once the clk framework has API for
that".
If still you want me to change the code then please send a NACK
(preferably on the updated series which I am preparing right now).

quoted

+     if (WARN_ON(PTR_ERR_OR_ZERO(dwmac->m250_div_clk)))
+             return PTR_ERR(dwmac->m250_div_clk);
+
+     /* create the m25_div */
+     snprintf(clk_name, sizeof(clk_name), "%s#m25_div", dev_name(dev));
+     init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL);
+     init.ops = &clk_divider_ops;
+     init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+     clk_div_parents[0] = __clk_get_name(dwmac->m250_div_clk);
+     init.parent_names = clk_div_parents;
+     init.num_parents = ARRAY_SIZE(clk_div_parents);
+
+     dwmac->m25_div.reg = dwmac->regs + PRG_ETH0;
+     dwmac->m25_div.shift = PRG_ETH0_CLK_M25_DIV_SHIFT;
+     dwmac->m25_div.width = PRG_ETH0_CLK_M25_DIV_WIDTH;
+     dwmac->m25_div.table = clk_25m_div_table;
+     dwmac->m25_div.hw.init = &init;
+     dwmac->m25_div.flags = CLK_DIVIDER_ALLOW_ZERO;
+
+     dwmac->m25_div_clk = devm_clk_register(dev, &dwmac->m25_div.hw);
+     if (WARN_ON(PTR_ERR_OR_ZERO(dwmac->m25_div_clk)))
+             return PTR_ERR(dwmac->m25_div_clk);
+
+     return 0;

This could be return WARN_ON(PTR_ERR_OR_ZERO(...))

This would work as well but I prefer the way it is right now (as one
could easily extend the code without having to touch any existing code
apart from the last return).
However, as it's always the case with personal preference: if
coding-style requires me to change it then I'll do so, just let me
know.

I have addressed all other issues you found (thanks for that!) in v4
(which I am about to send in the next few minutes).


Thanks,
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html