Nishanth Menon <nm@xxxxxx> 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 <ambresh@xxxxxx> > Cc: Benoit Cousson <b-cousson@xxxxxx> > Cc: Eduardo Valentin <eduardo.valentin@xxxxxxxxx> > Cc: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx> > Cc: Phil Carmody <ext-phil.2.carmody@xxxxxxxxx> > Cc: Sanjeev Premi <premi@xxxxxx> > Cc: Tero Kristo <tero.kristo@xxxxxxxxx> > Cc: Thara Gopinath <thara@xxxxxx> > > Signed-off-by: Nishanth Menon <nm@xxxxxx> > --- > 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 <plat/opp.h> > #include <plat/cpu.h> > +#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 <plat/opp.h> ? > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
