From patchwork Fri Dec 11 15:48:54 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tony Lindgren X-Patchwork-Id: 7830831 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id E2B36BEEE1 for ; Fri, 11 Dec 2015 15:49:03 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id E55B0205BE for ; Fri, 11 Dec 2015 15:49:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C8F33205B1 for ; Fri, 11 Dec 2015 15:49:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754316AbbLKPtA (ORCPT ); Fri, 11 Dec 2015 10:49:00 -0500 Received: from muru.com ([72.249.23.125]:52285 "EHLO muru.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753957AbbLKPs7 (ORCPT ); Fri, 11 Dec 2015 10:48:59 -0500 Received: from atomide.com (localhost [127.0.0.1]) by muru.com (Postfix) with ESMTPS id B38948103; Fri, 11 Dec 2015 15:49:57 +0000 (UTC) Date: Fri, 11 Dec 2015 07:48:54 -0800 From: Tony Lindgren To: Russell King - ARM Linux Cc: Michael Turquette , Stephen Boyd , Tero Kristo , Neil Armstrong , Matthijs van Duin , Delio Brignoli , Philipp Rosenberger , Brian Hutchinson , linux-omap@vger.kernel.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2] clk: ti: Add support for dm814x ADPLL Message-ID: <20151211154854.GK23396@atomide.com> References: <1449800792-5551-1-git-send-email-tony@atomide.com> <20151211135237.GG30871@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20151211135237.GG30871@n2100.arm.linux.org.uk> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP * Russell King - ARM Linux [151211 05:53]: > On Thu, Dec 10, 2015 at 06:26:32PM -0800, Tony Lindgren wrote: > > + /* Released with kfree() by clkdev_drop() */ > > + cl = kzalloc(sizeof(*cl), GFP_KERNEL); > > + if (!cl) > > + return -ENOMEM; > > + > > + /* Use clkdev_add, clk_register_clkdev limits length to MAX_CON_ID */ > > + cl->con_id = name; > > + cl->clk = clock; > > + cl->clk_hw = __clk_get_hw(clock); > > + clkdev_add(cl); > > + d->clocks[index].cl = cl; > > NAK. I've no idea why you're open-coding the clkdev internals (which > seems to have been a historical habbit in OMAP code.) Please stop > doing this. > > You are provided with clkdev_alloc() which will allocate the structure > and initialise it for you, and clkdev_add() which will add the allocated > and initialised struct to the list of lookups. Everything you're doing > above can be done with clkdev_alloc() + clkdev_add() which have been > there for a _very_ long time. They're even documented (thanks for > providing me with more proof that documentation is nothing but a waste > of time. :)) I tried, but I was seeing mysterious silent failures for some clocks. > Even better is clkdev_create() which eliminates the two step clkdev_alloc() > and clkdev_add() process. > > So, the whole of the above can be reduced down to: > > cl = clkdev_create(clock, name, NULL); > if (!cl) > return -ENOMEM; > There's a problem with MAX_CON_ID 16 hardcoded allocated name length. 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, Below is a patch we should probably apply as a band aid to produce a proper warning. Naturally we don't want to allocate longer names for all the clocks.. And we already do have some const names coming from device tree we could use. What if we optionally allow passing a name to clkdev and add some flag that tells clkdev code not to attempt to free it? Or do you have better ideas in mind to fix the issue? Regards, Tony 8< ---------------- From: Tony Lindgren Date: Fri, 11 Dec 2015 07:28:36 -0800 Subject: [PATCH] clkdev: Make silent failures of clk_get() produce a proper warning Otherwise we can see mysterious failures with some clocks where the name is longer than MAX_CON_ID 16. This can easily happen with clock names coming from device tree for example in the form of "481c5040.adpll.clkout" or "adpll1.clkdcoldo". Let's make things a bit easier to debug by adding a warning for the clock name. Note that we don't want to error out here as the string matching still probably does the right thing for most clocks. Also note that we should also remove the hardcoded name allocation in clkdev by adding functions that allow passing a name to clkdev optionally. Signed-off-by: Tony Lindgren --- 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;