From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PM-WIP-OPP][PATCH 1/2 v2] omap3: pm: cpufreq: BUG_ON cleanup Date: Tue, 20 Apr 2010 16:41:18 -0700 Message-ID: <87d3xtbskh.fsf@deeprootsystems.com> References: <1271792994-31816-1-git-send-email-nm@ti.com> <1271792994-31816-2-git-send-email-nm@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-pw0-f46.google.com ([209.85.160.46]:62354 "EHLO mail-pw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752320Ab0DTXlV (ORCPT ); Tue, 20 Apr 2010 19:41:21 -0400 Received: by pwj9 with SMTP id 9so4721656pwj.19 for ; Tue, 20 Apr 2010 16:41:21 -0700 (PDT) In-Reply-To: <1271792994-31816-2-git-send-email-nm@ti.com> (Nishanth Menon's message of "Tue\, 20 Apr 2010 14\:49\:53 -0500") Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Nishanth Menon Cc: linux-omap , Ambresh K , Benoit Cousson , Eduardo Valentin , Phil Carmody , Sanjeev Premi , Tero Kristo , Thara Gopinath Nishanth Menon writes: > BUG_ON should not ideally contain a functional code. > Ref: http://marc.info/?l=linux-kernel&m=109391212925546&w=2 > > To do this, we change the return of omap3_pm_init_opp from > void to int and return back error value for caller to adequately > handle further decisions. to reduce code duplication, the > registration and error handling are done in loop now. > > Cc: Ambresh K > Cc: Benoit Cousson > Cc: Eduardo Valentin > Cc: Kevin Hilman > Cc: Phil Carmody > Cc: Sanjeev Premi > Cc: Tero Kristo > Cc: Thara Gopinath > > Signed-off-by: Nishanth Menon > --- > Ref: > v1: https://patchwork.kernel.org/patch/86793/ > v2 changes: > removed BUG_ON entirely. instead have introduced int > return value allowing for board files which call to > handle the return results intelligently. Thanks, I like this better. > arch/arm/mach-omap2/cpufreq34xx.c | 36 ++++++++++++++++++++++++++++++++---- > arch/arm/mach-omap2/omap3-opp.h | 5 +++-- > 2 files changed, 35 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/mach-omap2/cpufreq34xx.c b/arch/arm/mach-omap2/cpufreq34xx.c > index 189c42e..01cf98f 100644 > --- a/arch/arm/mach-omap2/cpufreq34xx.c > +++ b/arch/arm/mach-omap2/cpufreq34xx.c > @@ -25,6 +25,7 @@ > > #include > #include > +#include "omap3-opp.h" > > static struct omap_opp_def __initdata omap34xx_mpu_rate_table[] = { > /* OPP1 */ > @@ -109,8 +110,9 @@ static struct omap_opp_def __initdata omap36xx_dsp_rate_table[] = { > OMAP_OPP_DEF(0, 0, 0) > }; > > -void __init omap3_pm_init_opp_table(void) > +int __init omap3_pm_init_opp_table(void) > { > + int i, r; > struct omap_opp_def **omap3_opp_def_list; > struct omap_opp_def *omap34xx_opp_def_list[] = { > omap34xx_mpu_rate_table, > @@ -122,12 +124,38 @@ void __init omap3_pm_init_opp_table(void) > omap36xx_l3_rate_table, > omap36xx_dsp_rate_table > }; > + enum opp_t omap3_opps[] = { > + OPP_MPU, > + OPP_L3, > + OPP_DSP > + }; Aren't these already defined in ? > > omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list : > omap34xx_opp_def_list; > > - BUG_ON(opp_init_list(OPP_MPU, omap3_opp_def_list[0])); > - BUG_ON(opp_init_list(OPP_L3, omap3_opp_def_list[1])); > - BUG_ON(opp_init_list(OPP_DSP, omap3_opp_def_list[2])); > + for (i = 0; i < ARRAY_SIZE(omap3_opps); i++) { > + r = opp_init_list(omap3_opps[i], omap3_opp_def_list[i]); > + if (r) > + break; > + } > + if (!r) > + return 0; > + > + /* Cascading error handling - disable all enabled OPPs */ > + pr_err("%s: Failed to register %d OPP type\n", __func__, > + omap3_opps[i]); > + i--; > + while (i != -1) { > + struct omap_opp *opp; > + unsigned long freq = 0; insert blank line > + while (!IS_ERR(opp = opp_find_freq_ceil(omap3_opps[i], > + &freq))) { for redability, would rather see the line-wrap avoided. Just put the IS_ERR() on a separate line. > + opp_disable(opp); > + freq++; > + } > + i--; > + } > + > + return r; > } > > diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h > index 1ba85fc..3e88d8c 100644 > --- a/arch/arm/mach-omap2/omap3-opp.h > +++ b/arch/arm/mach-omap2/omap3-opp.h > @@ -9,10 +9,11 @@ > * table after the basic initialization > */ > #ifdef CONFIG_CPU_FREQ > -extern void omap3_pm_init_opp_table(void); > +extern int omap3_pm_init_opp_table(void); > #else > -static inline void omap3_pm_init_opp_table(void) > +static inline int omap3_pm_init_opp_table(void) > { > + return 0; > } > #endif > > -- > 1.6.3.3 Kevin