Message ID | 20151211154854.GK23396@atomide.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, Dec 11, 2015 at 07:48:54AM -0800, Tony Lindgren wrote: > There's a problem with MAX_CON_ID 16 hardcoded allocated name length. Absolutely right... > In this case I have 13 instances of plls with 3 - 4 outputs each and I'd > like to use "481c5040.adpll.clkout" style naming starting with the instance > address. Also "adpll5.clkdcoldo" style naming would work, Because it's a connection ID, not a clock _name_. I see that we're still making all the same mistakes with clocks that were made years ago, and which lead down the path of amazingly complex drivers having conditional clock names and the like. Is there a reason why you can't split this into separate device and input names, and use clk_get_sys() rather than passing NULL as the device to clk_get(). This is exactly why we've ended up with clk_get_sys(), to cater for the cases where there is no struct device to associate with the connection ID: the idea behind clk_get_sys() is that you pass the device name explicitly, along with the connection ID.
* Russell King - ARM Linux <linux@arm.linux.org.uk> [151211 08:16]: > On Fri, Dec 11, 2015 at 07:48:54AM -0800, Tony Lindgren wrote: > > There's a problem with MAX_CON_ID 16 hardcoded allocated name length. > > Absolutely right... Well adding the warning there allows people to at least see what's going on. > > In this case I have 13 instances of plls with 3 - 4 outputs each and I'd > > like to use "481c5040.adpll.clkout" style naming starting with the instance > > address. Also "adpll5.clkdcoldo" style naming would work, > > Because it's a connection ID, not a clock _name_. I see that we're still > making all the same mistakes with clocks that were made years ago, and > which lead down the path of amazingly complex drivers having conditional > clock names and the like. Yeah we certainly want to get out of the conditional name business. > Is there a reason why you can't split this into separate device and input > names, and use clk_get_sys() rather than passing NULL as the device to > clk_get(). This is exactly why we've ended up with clk_get_sys(), to > cater for the cases where there is no struct device to associate with the > connection ID: the idea behind clk_get_sys() is that you pass the device > name explicitly, along with the connection ID. For most code yes, there's still a pile of shared legacy code relying on the clock connection ID only. So what do you envision people using for the connection ID in cases like this? It needs to be unique, should relate to the documentation and fit MAX_CON_ID. Something like "pll5.clkdcoldo" will still overflow for pll13 unless also the dot is left out :) Regards, Tony -- 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
--- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -256,6 +256,10 @@ vclkdev_alloc(struct clk_hw *hw, const char *con_id, const char *dev_fmt, { struct clk_lookup_alloc *cla; + if (strlen(con_id) > MAX_CON_ID) + pr_warn("%s length of %s > %i, clock lookup can fail\n", + __func__, con_id, MAX_CON_ID); + cla = __clkdev_alloc(sizeof(*cla)); if (!cla) return NULL;