| Message ID | 201306111329.32749.heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 11, 2013 at 2:29 PM, Heiko Stübner <heiko@sntech.de> 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; -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Dienstag, 11. Juni 2013, 13:51:56 schrieb Andy Shevchenko: > On Tue, Jun 11, 2013 at 2:29 PM, Heiko Stübner <heiko@sntech.de> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 11, 2013 at 3:06 PM, Heiko Stübner <heiko@sntech.de> wrote: > Am Dienstag, 11. Juni 2013, 13:51:56 schrieb Andy Shevchenko: >> On Tue, Jun 11, 2013 at 2:29 PM, Heiko Stübner <heiko@sntech.de> wrote: [] >> > @@ -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. This variant I think fits: if (!(divider->flags & CLK_DIVIDER_EVEN)) return div < 2 || div % 2 == 0; You check for feature and do accordingly. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 11, 2013 at 3:37 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jun 11, 2013 at 3:06 PM, Heiko Stübner <heiko@sntech.de> wrote: >> Am Dienstag, 11. Juni 2013, 13:51:56 schrieb Andy Shevchenko: >>> On Tue, Jun 11, 2013 at 2:29 PM, Heiko Stübner <heiko@sntech.de> wrote: > This variant I think fits: > > if (!(divider->flags & CLK_DIVIDER_EVEN)) Oops, without "!()" of course. > return div < 2 || div % 2 == 0; > > You check for feature and do accordingly. -- With Best Regards, Andy Shevchenko -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Heiko Stübner (2013-06-11 04:29:32) > 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. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > drivers/clk/clk-divider.c | 14 ++++++++++++-- > include/linux/clk-provider.h | 2 ++ > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index ce5cfe9..bdee7cf 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -45,8 +45,16 @@ static unsigned int _get_table_maxdiv(const struct clk_div_table *table) > > static unsigned int _get_maxdiv(struct clk_divider *divider) > { > - if (divider->flags & CLK_DIVIDER_ONE_BASED) > - return div_mask(divider); > + if (divider->flags & CLK_DIVIDER_ONE_BASED) { > + unsigned int div = div_mask(divider); > + > + /* decrease to even number */ > + if (divider->flags & CLK_DIVIDER_EVEN) > + div--; > + > + return div; > + } > + > if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) > return 1 << div_mask(divider); > if (divider->table) > @@ -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) Is it correct to check for 'div != 1' here? Wouldn't that check only be valid in the presence of CLK_DIVIDER_ONE_BASED? Maybe something like this would be more correct: if (divider->flags & CLK_DIVIDER_EVEN && (div % 2) != 0) { if (divider->flags & CLK_DIVIDER_ONE_BASED && div == 1) return true; return false; } Regards, Mike > + return false; > return true; > } > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 1ec14a7..bd52e52 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -266,6 +266,7 @@ struct clk_div_table { > * of this register, and mask of divider bits are in higher 16-bit of this > * register. While setting the divider bits, higher 16-bit should also be > * updated to indicate changing divider bits. > + * CLK_DIVIDER_EVEN - only allow even divider values > */ > struct clk_divider { > struct clk_hw hw; > @@ -281,6 +282,7 @@ struct clk_divider { > #define CLK_DIVIDER_POWER_OF_TWO BIT(1) > #define CLK_DIVIDER_ALLOW_ZERO BIT(2) > #define CLK_DIVIDER_HIWORD_MASK BIT(3) > +#define CLK_DIVIDER_EVEN BIT(4) > > extern const struct clk_ops clk_divider_ops; > struct clk *clk_register_divider(struct device *dev, const char *name, > -- > 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Dienstag, 11. Juni 2013, 20:57:50 schrieb Mike Turquette: > Quoting Heiko Stübner (2013-06-11 04:29:32) > > > 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. > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > --- > > > > drivers/clk/clk-divider.c | 14 ++++++++++++-- > > include/linux/clk-provider.h | 2 ++ > > 2 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > > index ce5cfe9..bdee7cf 100644 > > --- a/drivers/clk/clk-divider.c > > +++ b/drivers/clk/clk-divider.c > > @@ -45,8 +45,16 @@ static unsigned int _get_table_maxdiv(const struct > > clk_div_table *table) > > > > static unsigned int _get_maxdiv(struct clk_divider *divider) > > { > > > > - if (divider->flags & CLK_DIVIDER_ONE_BASED) > > - return div_mask(divider); > > + if (divider->flags & CLK_DIVIDER_ONE_BASED) { > > + unsigned int div = div_mask(divider); > > + > > + /* decrease to even number */ > > + if (divider->flags & CLK_DIVIDER_EVEN) > > + div--; > > + > > + return div; > > + } > > + > > > > if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) > > > > return 1 << div_mask(divider); > > > > if (divider->table) > > > > @@ -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) > > Is it correct to check for 'div != 1' here? Wouldn't that check only be > valid in the presence of CLK_DIVIDER_ONE_BASED? > > Maybe something like this would be more correct: > > if (divider->flags & CLK_DIVIDER_EVEN && (div % 2) != 0) { > if (divider->flags & CLK_DIVIDER_ONE_BASED && div == 1) > return true; > return false; > } hmm, not necessarily. As I understand the doc DIVIDER_ONE_BASED describes how the divider is stored in the register, i.e. if the register value is div-1 or div. When div is 1, it of course means don't divide [rate/1], and CLK_DIVIDER_EVEN needs to just make sure that bigger dividers that really divide the rate are even values (so limiting the possible dividers to 1 2 4 ...), independent on how the divider value is stored in the register. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index ce5cfe9..bdee7cf 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -45,8 +45,16 @@ static unsigned int _get_table_maxdiv(const struct clk_div_table *table) static unsigned int _get_maxdiv(struct clk_divider *divider) { - if (divider->flags & CLK_DIVIDER_ONE_BASED) - return div_mask(divider); + if (divider->flags & CLK_DIVIDER_ONE_BASED) { + unsigned int div = div_mask(divider); + + /* decrease to even number */ + if (divider->flags & CLK_DIVIDER_EVEN) + div--; + + return div; + } + if (divider->flags & CLK_DIVIDER_POWER_OF_TWO) return 1 << div_mask(divider); if (divider->table) @@ -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; } diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index 1ec14a7..bd52e52 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -266,6 +266,7 @@ struct clk_div_table { * of this register, and mask of divider bits are in higher 16-bit of this * register. While setting the divider bits, higher 16-bit should also be * updated to indicate changing divider bits. + * CLK_DIVIDER_EVEN - only allow even divider values */ struct clk_divider { struct clk_hw hw; @@ -281,6 +282,7 @@ struct clk_divider { #define CLK_DIVIDER_POWER_OF_TWO BIT(1) #define CLK_DIVIDER_ALLOW_ZERO BIT(2) #define CLK_DIVIDER_HIWORD_MASK BIT(3) +#define CLK_DIVIDER_EVEN BIT(4) extern const struct clk_ops clk_divider_ops; struct clk *clk_register_divider(struct device *dev, const char *name,
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. Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- drivers/clk/clk-divider.c | 14 ++++++++++++-- include/linux/clk-provider.h | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-)