From patchwork Tue Mar 5 16:38:36 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 2220441 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork1.kernel.org (Postfix) with ESMTP id 5B0253FCF6 for ; Tue, 5 Mar 2013 16:41:56 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1UCusy-0001A8-Rh; Tue, 05 Mar 2013 16:38:40 +0000 Received: from mail-oa0-f44.google.com ([209.85.219.44]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1UCusw-00019p-Fd for linux-arm-kernel@lists.infradead.org; Tue, 05 Mar 2013 16:38:38 +0000 Received: by mail-oa0-f44.google.com with SMTP id h1so11226578oag.3 for ; Tue, 05 Mar 2013 08:38:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type:x-gm-message-state; bh=G/jZDcFDP5x35nuBsiyx9VNm76mdh+IPVoRxylTDyno=; b=pIljnyvBzNZOzxmM7C4Ag8ZQ8xitm5ZaPReoMoTVG4lZ/MUN3SlwmT3S6graUPuk0U JNi5F0Ho9WfiRRHn/wyce1vWMeqpItjBzGNppg1OUgK01Df35nXATY5VzqdUN50jf/cw EVFHj4DlwTC7rbBNKcxppGWhzAPX77cPtOA8AstjbkGZw2MJLTAwbQmh7WeMyXeQKjSo 0WThfcT8qHJcmne3NCRRyMH7aTduhsa4zVZL02Fmp82RjWO2eucHCdEsE8xaMhgbXMNo bvMKOP3zAKYTsldxGc1cYcdrit7Ops4ikUxVOMeP96huiKq4psILsICnCf+awPxESl8a Hsnw== MIME-Version: 1.0 X-Received: by 10.60.170.177 with SMTP id an17mr19463299oec.62.1362501516537; Tue, 05 Mar 2013 08:38:36 -0800 (PST) Received: by 10.182.69.20 with HTTP; Tue, 5 Mar 2013 08:38:36 -0800 (PST) In-Reply-To: <20130305105251.GL17833@n2100.arm.linux.org.uk> References: <20130305105251.GL17833@n2100.arm.linux.org.uk> Date: Wed, 6 Mar 2013 00:38:36 +0800 Message-ID: Subject: Re: [PATCH] cpufreq: ARM big LITTLE: Add generic cpufreq driver and its DT glue From: Viresh Kumar To: Russell King - ARM Linux X-Gm-Message-State: ALoCoQnLUiShWUDFvA/w1uRxbSZzB5+dDCKYHyC2NIIWCB0Z0TCwMKyaVWl03MKuE3osIMPQlJcs X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130305_113838_583827_5230D051 X-CRM114-Status: GOOD ( 18.24 ) X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.219.44 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Steve.Bannister@arm.com, linux-pm@vger.kernel.org, Sudeep KarkadaNagesha , Liviu.Dudau@arm.com, linux-kernel@vger.kernel.org, cpufreq@vger.kernel.org, rjw@sisk.pl, robin.randhawa@arm.com, mark.hambleton@broadcom.com, linaro-kernel@lists.linaro.org, linux-arm-kernel@lists.infradead.org, charles.garcia-tobin@arm.com X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On 5 March 2013 18:52, Russell King - ARM Linux wrote: > On Tue, Mar 05, 2013 at 12:52:41PM +0800, Viresh Kumar wrote: >> +static void put_cluster_clk_and_freq_table(u32 cluster) >> +{ >> + if (!atomic_dec_return(&cluster_usage[cluster])) { >> + clk_put(clk[cluster]); >> + clk[cluster] = NULL; > > What's the point in setting the clk to NULL? I couldn't find one and the same is true for freq_table[] too. >> +static int get_cluster_clk_and_freq_table(u32 cluster) >> +{ >> + char name[9] = "cluster"; >> + int count; >> + >> + if (atomic_inc_return(&cluster_usage[cluster]) != 1) >> + return 0; >> + >> + freq_table[cluster] = arm_bL_ops->get_freq_tbl(cluster, &count); >> + if (!freq_table[cluster]) >> + goto atomic_dec; >> + >> + name[7] = cluster + '0'; >> + clk[cluster] = clk_get(NULL, name); >> + if (!IS_ERR_OR_NULL(clk[cluster])) { > > NAK. Two reasons. > > 1. IS_ERR_OR_NULL. You know about this, it's been on the list several > times. AAHHHHHH .. How can i mess up with this concept.. I am really feeling bad now. > 2. Maybe clk_get_sys() rather than clk_get() and use a sensible device > name ("cpu-cluster.%u" maybe) rather than a connection name with a > NULL device? That's a good comment (rather than pointing at some stupid mistake), I will probably keep the same name for the device as well. So how does below fix look to you? ----------x-----------------x----------------- diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index 0d6de0e..2486b9a 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -140,9 +140,7 @@ static void put_cluster_clk_and_freq_table(u32 cluster) { if (!atomic_dec_return(&cluster_usage[cluster])) { clk_put(clk[cluster]); - clk[cluster] = NULL; arm_bL_ops->put_freq_tbl(cluster); - freq_table[cluster] = NULL; pr_debug("%s: cluster: %d\n", __func__, cluster); } } @@ -160,8 +158,8 @@ static int get_cluster_clk_and_freq_table(u32 cluster) goto atomic_dec; name[7] = cluster + '0'; - clk[cluster] = clk_get(NULL, name); - if (!IS_ERR_OR_NULL(clk[cluster])) { + clk[cluster] = clk_get_sys(name, NULL); + if (!IS_ERR(clk[cluster])) { pr_debug("%s: clk: %p & freq table: %p, cluster: %d\n", __func__, clk[cluster], freq_table[cluster], cluster);