From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko =?utf-8?q?St=C3=BCbner?= Subject: Re: [PATCH v3 1/7] clk: divider: add flag to limit possible dividers to even numbers Date: Tue, 11 Jun 2013 14:06:14 +0200 Message-ID: <201306111406.14850.heiko@sntech.de> References: <201306111328.52679.heiko@sntech.de> <201306111329.32749.heiko@sntech.de> Mime-Version: 1.0 Content-Type: Text/Plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from gloria.sntech.de ([95.129.55.99]:38930 "EHLO gloria.sntech.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753125Ab3FKMGZ (ORCPT ); Tue, 11 Jun 2013 08:06:25 -0400 In-Reply-To: Sender: linux-mmc-owner@vger.kernel.org List-Id: linux-mmc@vger.kernel.org To: Andy Shevchenko Cc: "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Mike Turquette , Seungwon Jeon , Jaehoon Chung , Chris Ball , "linux-mmc@vger.kernel.org" , Grant Likely , Rob Herring , Linus Walleij , Devicetree Discuss , Russell King , Arnd Bergmann , Olof Johansson , Thomas Petazzoni Am Dienstag, 11. Juni 2013, 13:51:56 schrieb Andy Shevchenko: > On Tue, Jun 11, 2013 at 2:29 PM, Heiko St=C3=BCbner = wrote: > > SoCs like the Rockchip Cortex-A9 ones contain divider some clocks > > that use the regular mechanisms for storage but allow only even > > dividers and 1 to be used. > >=20 > > Therefore add a flag that lets _is_valid_div limit the valid divide= rs > > to these values. _get_maxdiv is also adapted to return even values > > for the CLK_DIVIDER_ONE_BASED case. >=20 > Just one nitpick below (I'm okay with current implementation, but you > might find my proposal useful). >=20 > > --- a/drivers/clk/clk-divider.c > > +++ b/drivers/clk/clk-divider.c > >=20 > > @@ -141,6 +149,8 @@ static bool _is_valid_div(struct clk_divider > > *divider, unsigned int div) > >=20 > > return is_power_of_2(div); > > =20 > > if (divider->table) > > =20 > > return _is_valid_table_div(divider->table, div); > >=20 > > + if (divider->flags & CLK_DIVIDER_EVEN && div !=3D 1 && (div= % 2) !=3D > > 0) + return false; > >=20 > > return true; > > =20 > > } >=20 > What if rewrite like >=20 > if (divider->flags & CLK_DIVIDER_EVEN =3D=3D 0) > return true; >=20 > return div < 2 || div % 2 =3D=3D 0; hmm, the current structure is of the form of testing for each feature a= nd=20 doing a applicable action if the flag is set. So it also is extensible = for=20 future flags and checking for the absence of an attribute while the res= t of=20 the conditionals check for the presence also might make the code harder= to=20 read. So for me the current variant somehow looks more intuitive. But I'll just let the majority decide ;-) Heiko From mboxrd@z Thu Jan 1 00:00:00 1970 From: heiko@sntech.de (Heiko =?utf-8?q?St=C3=BCbner?=) Date: Tue, 11 Jun 2013 14:06:14 +0200 Subject: [PATCH v3 1/7] clk: divider: add flag to limit possible dividers to even numbers In-Reply-To: References: <201306111328.52679.heiko@sntech.de> <201306111329.32749.heiko@sntech.de> Message-ID: <201306111406.14850.heiko@sntech.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Am Dienstag, 11. Juni 2013, 13:51:56 schrieb Andy Shevchenko: > On Tue, Jun 11, 2013 at 2:29 PM, Heiko St?bner wrote: > > SoCs like the Rockchip Cortex-A9 ones contain divider some clocks > > that use the regular mechanisms for storage but allow only even > > dividers and 1 to be used. > > > > Therefore add a flag that lets _is_valid_div limit the valid dividers > > to these values. _get_maxdiv is also adapted to return even values > > for the CLK_DIVIDER_ONE_BASED case. > > Just one nitpick below (I'm okay with current implementation, but you > might find my proposal useful). > > > --- a/drivers/clk/clk-divider.c > > +++ b/drivers/clk/clk-divider.c > > > > @@ -141,6 +149,8 @@ static bool _is_valid_div(struct clk_divider > > *divider, unsigned int div) > > > > return is_power_of_2(div); > > > > if (divider->table) > > > > return _is_valid_table_div(divider->table, div); > > > > + if (divider->flags & CLK_DIVIDER_EVEN && div != 1 && (div % 2) != > > 0) + return false; > > > > return true; > > > > } > > What if rewrite like > > if (divider->flags & CLK_DIVIDER_EVEN == 0) > return true; > > return div < 2 || div % 2 == 0; hmm, the current structure is of the form of testing for each feature and doing a applicable action if the flag is set. So it also is extensible for future flags and checking for the absence of an attribute while the rest of the conditionals check for the presence also might make the code harder to read. So for me the current variant somehow looks more intuitive. But I'll just let the majority decide ;-) Heiko