Re: [PATCH RFC] clk: ux500: add range to usleep_range
On Thu, Apr 11, 2019 at 06:12:49AM -0700, Joe Perches wrote: > On Thu, 2019-04-11 at 14:59 +0200, Nicholas Mc Guire wrote: > > On Thu, Apr 11, 2019 at 04:51:25AM -0700, Joe Perches wrote: > > > On Thu, 2019-04-11 at 04:56 +0200, Nicholas Mc Guire wrote: > > > > On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote: > > > > > Quoting Nicholas Mc Guire (2019-04-06 20:13:24) > > > > > > Providing a range for usleep_range() allows the hrtimer subsystem to > > > > > > coalesce timers - the delay is runtime configurable so a factor 2 > > > > > > is taken to provide the range. > > > > > > > > > > > > Signed-off-by: Nicholas Mc Guire > > > > > > --- > > > > > > > > > > I think this driver is in maintenance mode. I'll wait for Ulf to ack > > > > > or > > > > > review this change before applying. > > > > > > > > > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c > > > > > > b/drivers/clk/ux500/clk-sysctrl.c > > > > > > index 7c0403b..a1fa3fb 100644 > > > > > > --- a/drivers/clk/ux500/clk-sysctrl.c > > > > > > +++ b/drivers/clk/ux500/clk-sysctrl.c > > > > > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) > > > > > > clk->reg_bits[0]); > > > > > > > > > > > > if (!ret && clk->enable_delay_us) > > > > > > - usleep_range(clk->enable_delay_us, > > > > > > clk->enable_delay_us); > > > > > > + usleep_range(clk->enable_delay_us, > > > > > > clk->enable_delay_us*2); > > > > > > > > > > Please add space around that multiply. > > > > > > > > > I can do that but it does not seem common and also checkpatch > > > > did not complain about this - now a simple grep -re "\*10" on the > > > > kernel shows that it seems more common not to use spaces around * > > > > that to use them. > > > > > > Not really > > > > > > $ git grep -P '\*\s*10' | grep -oh -P '\*\s*10' | \ > > > sort | uniq -c | sort -rn | head > > > 11800 * 10 > > >1705 *10 > > > 179 * 10 > > > 74 * 10 > > > 67 * 10 > > > 20 * 10 > > > 20 * 10 > > > 14 *10 > > > 14 * 10 > > > 12 * 10 > > > > yup - my bad - If you restrict it to code lines - its 1:10 > > not quite sure how I got the first numbers - sloppy check. > > > > hofrat@debian:~/git/linux-next$ grep -re '.*\*\s*10.*;$' * | grep -oh > > '\*\s*10' | sort | uniq -c | sort -nr | more > >8568 * 10 > > 860 *10 > > The ratio is about the same in any case. > > > Anyway - is there a reason checkpatch will not flag this ? > > Because the style is not mentioned in coding style. > checkpatch flags it only when using the --strict option > hmm...It sees it is: 3.1) Spaces *** ... Use one space around (on each side of) most binary and ternary operators, such as any of these:: = + - < > * / % | & ^ <= >= == != ? : thx! hofrat
Re: [PATCH RFC] clk: ux500: add range to usleep_range
On Thu, 2019-04-11 at 14:59 +0200, Nicholas Mc Guire wrote: > On Thu, Apr 11, 2019 at 04:51:25AM -0700, Joe Perches wrote: > > On Thu, 2019-04-11 at 04:56 +0200, Nicholas Mc Guire wrote: > > > On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote: > > > > Quoting Nicholas Mc Guire (2019-04-06 20:13:24) > > > > > Providing a range for usleep_range() allows the hrtimer subsystem to > > > > > coalesce timers - the delay is runtime configurable so a factor 2 > > > > > is taken to provide the range. > > > > > > > > > > Signed-off-by: Nicholas Mc Guire > > > > > --- > > > > > > > > I think this driver is in maintenance mode. I'll wait for Ulf to ack or > > > > review this change before applying. > > > > > > > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c > > > > > b/drivers/clk/ux500/clk-sysctrl.c > > > > > index 7c0403b..a1fa3fb 100644 > > > > > --- a/drivers/clk/ux500/clk-sysctrl.c > > > > > +++ b/drivers/clk/ux500/clk-sysctrl.c > > > > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) > > > > > clk->reg_bits[0]); > > > > > > > > > > if (!ret && clk->enable_delay_us) > > > > > - usleep_range(clk->enable_delay_us, > > > > > clk->enable_delay_us); > > > > > + usleep_range(clk->enable_delay_us, > > > > > clk->enable_delay_us*2); > > > > > > > > Please add space around that multiply. > > > > > > > I can do that but it does not seem common and also checkpatch > > > did not complain about this - now a simple grep -re "\*10" on the > > > kernel shows that it seems more common not to use spaces around * > > > that to use them. > > > > Not really > > > > $ git grep -P '\*\s*10' | grep -oh -P '\*\s*10' | \ > > sort | uniq -c | sort -rn | head > > 11800 * 10 > >1705 *10 > > 179 * 10 > > 74 * 10 > > 67 * 10 > > 20 * 10 > > 20 * 10 > > 14 *10 > > 14 * 10 > > 12 * 10 > > yup - my bad - If you restrict it to code lines - its 1:10 > not quite sure how I got the first numbers - sloppy check. > > hofrat@debian:~/git/linux-next$ grep -re '.*\*\s*10.*;$' * | grep -oh > '\*\s*10' | sort | uniq -c | sort -nr | more >8568 * 10 > 860 *10 The ratio is about the same in any case. > Anyway - is there a reason checkpatch will not flag this ? Because the style is not mentioned in coding style. checkpatch flags it only when using the --strict option
Re: [PATCH RFC] clk: ux500: add range to usleep_range
On Thu, Apr 11, 2019 at 04:51:25AM -0700, Joe Perches wrote: > On Thu, 2019-04-11 at 04:56 +0200, Nicholas Mc Guire wrote: > > On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote: > > > Quoting Nicholas Mc Guire (2019-04-06 20:13:24) > > > > Providing a range for usleep_range() allows the hrtimer subsystem to > > > > coalesce timers - the delay is runtime configurable so a factor 2 > > > > is taken to provide the range. > > > > > > > > Signed-off-by: Nicholas Mc Guire > > > > --- > > > > > > I think this driver is in maintenance mode. I'll wait for Ulf to ack or > > > review this change before applying. > > > > > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c > > > > b/drivers/clk/ux500/clk-sysctrl.c > > > > index 7c0403b..a1fa3fb 100644 > > > > --- a/drivers/clk/ux500/clk-sysctrl.c > > > > +++ b/drivers/clk/ux500/clk-sysctrl.c > > > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) > > > > clk->reg_bits[0]); > > > > > > > > if (!ret && clk->enable_delay_us) > > > > - usleep_range(clk->enable_delay_us, > > > > clk->enable_delay_us); > > > > + usleep_range(clk->enable_delay_us, > > > > clk->enable_delay_us*2); > > > > > > Please add space around that multiply. > > > > > I can do that but it does not seem common and also checkpatch > > did not complain about this - now a simple grep -re "\*10" on the > > kernel shows that it seems more common not to use spaces around * > > that to use them. > > Not really > > $ git grep -P '\*\s*10' | grep -oh -P '\*\s*10' | \ > sort | uniq -c | sort -rn | head > 11800 * 10 >1705 *10 > 179 * 10 > 74 * 10 > 67 * 10 > 20 * 10 > 20 * 10 > 14 *10 > 14 * 10 > 12 * 10 yup - my bad - If you restrict it to code lines - its 1:10 not quite sure how I got the first numbers - sloppy check. hofrat@debian:~/git/linux-next$ grep -re '.*\*\s*10.*;$' * | grep -oh '\*\s*10' | sort | uniq -c | sort -nr | more 8568 * 10 860 *10 Anyway - is there a reason checkpatch will not flag this ? thx! hofrat > > > Greping specifically for cases using usleep_range() > > (not that many) it seems more or less evenly devided between space > > and no space - so the concern is overlooking that factor 2 ? > > > > thx! > > hofrat >
Re: [PATCH RFC] clk: ux500: add range to usleep_range
On Thu, 2019-04-11 at 04:56 +0200, Nicholas Mc Guire wrote: > On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote: > > Quoting Nicholas Mc Guire (2019-04-06 20:13:24) > > > Providing a range for usleep_range() allows the hrtimer subsystem to > > > coalesce timers - the delay is runtime configurable so a factor 2 > > > is taken to provide the range. > > > > > > Signed-off-by: Nicholas Mc Guire > > > --- > > > > I think this driver is in maintenance mode. I'll wait for Ulf to ack or > > review this change before applying. > > > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c > > > b/drivers/clk/ux500/clk-sysctrl.c > > > index 7c0403b..a1fa3fb 100644 > > > --- a/drivers/clk/ux500/clk-sysctrl.c > > > +++ b/drivers/clk/ux500/clk-sysctrl.c > > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) > > > clk->reg_bits[0]); > > > > > > if (!ret && clk->enable_delay_us) > > > - usleep_range(clk->enable_delay_us, clk->enable_delay_us); > > > + usleep_range(clk->enable_delay_us, > > > clk->enable_delay_us*2); > > > > Please add space around that multiply. > > > I can do that but it does not seem common and also checkpatch > did not complain about this - now a simple grep -re "\*10" on the > kernel shows that it seems more common not to use spaces around * > that to use them. Not really $ git grep -P '\*\s*10' | grep -oh -P '\*\s*10' | \ sort | uniq -c | sort -rn | head 11800 * 10 1705 *10 179 * 10 74 * 10 67 * 10 20 * 10 20 * 10 14 *10 14 * 10 12 * 10 > Greping specifically for cases using usleep_range() > (not that many) it seems more or less evenly devided between space > and no space - so the concern is overlooking that factor 2 ? > > thx! > hofrat
Re: [PATCH RFC] clk: ux500: add range to usleep_range
On Thu, Apr 11, 2019 at 11:36:45AM +0200, Ulf Hansson wrote: > On Sun, 7 Apr 2019 at 05:13, Nicholas Mc Guire wrote: > > > > Providing a range for usleep_range() allows the hrtimer subsystem to > > coalesce timers - the delay is runtime configurable so a factor 2 > > is taken to provide the range. > > > > Signed-off-by: Nicholas Mc Guire > > --- > > > > Problem located with an experimental coccinelle script > > > > Q: Basically usleep_range() with min == max never makes much sense notably > >in non-atomic context. If the factor of 2 is tolerable or a fixed > >offset of e.g. 1000 would be more suitable is not clear to me - maybe > >someone familiar with that driver can clarify this. > > > > Patch was compile tested with: u8500_defconfig (implies COMMON_CLK=y) > > (with some sparse warnings about not implemented system calls) > > > > Patch is against 5.1-rc3 (localversion-next is next=20190405) > > > > drivers/clk/ux500/clk-sysctrl.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c > > b/drivers/clk/ux500/clk-sysctrl.c > > index 7c0403b..a1fa3fb 100644 > > --- a/drivers/clk/ux500/clk-sysctrl.c > > +++ b/drivers/clk/ux500/clk-sysctrl.c > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) > > clk->reg_bits[0]); > > > > if (!ret && clk->enable_delay_us) > > - usleep_range(clk->enable_delay_us, clk->enable_delay_us); > > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2); > > The range being used is actually in ms, so not sure we actually need > to double it for the range. > > How about adding ~25% instead, along the lines of below: > usleep_range(clk->enable_delay_us, clk->enable_delay_us + > (clk->enable_delay_us >> 2)); > that would be perfectly sufficient for hrtimers - if the range is in ms - so I´ll send out a V2 shortly. thx! hofrat
Re: [PATCH RFC] clk: ux500: add range to usleep_range
On Sun, 7 Apr 2019 at 05:13, Nicholas Mc Guire wrote: > > Providing a range for usleep_range() allows the hrtimer subsystem to > coalesce timers - the delay is runtime configurable so a factor 2 > is taken to provide the range. > > Signed-off-by: Nicholas Mc Guire > --- > > Problem located with an experimental coccinelle script > > Q: Basically usleep_range() with min == max never makes much sense notably >in non-atomic context. If the factor of 2 is tolerable or a fixed >offset of e.g. 1000 would be more suitable is not clear to me - maybe >someone familiar with that driver can clarify this. > > Patch was compile tested with: u8500_defconfig (implies COMMON_CLK=y) > (with some sparse warnings about not implemented system calls) > > Patch is against 5.1-rc3 (localversion-next is next=20190405) > > drivers/clk/ux500/clk-sysctrl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c > index 7c0403b..a1fa3fb 100644 > --- a/drivers/clk/ux500/clk-sysctrl.c > +++ b/drivers/clk/ux500/clk-sysctrl.c > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) > clk->reg_bits[0]); > > if (!ret && clk->enable_delay_us) > - usleep_range(clk->enable_delay_us, clk->enable_delay_us); > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2); The range being used is actually in ms, so not sure we actually need to double it for the range. How about adding ~25% instead, along the lines of below: usleep_range(clk->enable_delay_us, clk->enable_delay_us + (clk->enable_delay_us >> 2)); Kind regards Uffe
Re: [PATCH RFC] clk: ux500: add range to usleep_range
On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote: > Quoting Nicholas Mc Guire (2019-04-06 20:13:24) > > Providing a range for usleep_range() allows the hrtimer subsystem to > > coalesce timers - the delay is runtime configurable so a factor 2 > > is taken to provide the range. > > > > Signed-off-by: Nicholas Mc Guire > > --- > > I think this driver is in maintenance mode. I'll wait for Ulf to ack or > review this change before applying. > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c > > b/drivers/clk/ux500/clk-sysctrl.c > > index 7c0403b..a1fa3fb 100644 > > --- a/drivers/clk/ux500/clk-sysctrl.c > > +++ b/drivers/clk/ux500/clk-sysctrl.c > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) > > clk->reg_bits[0]); > > > > if (!ret && clk->enable_delay_us) > > - usleep_range(clk->enable_delay_us, clk->enable_delay_us); > > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2); > > Please add space around that multiply. > I can do that but it does not seem common and also checkpatch did not complain about this - now a simple grep -re "\*10" on the kernel shows that it seems more common not to use spaces around * that to use them. Greping specifically for cases using usleep_range() (not that many) it seems more or less evenly devided between space and no space - so the concern is overlooking that factor 2 ? thx! hofrat
Re: [PATCH RFC] clk: ux500: add range to usleep_range
Quoting Nicholas Mc Guire (2019-04-06 20:13:24) > Providing a range for usleep_range() allows the hrtimer subsystem to > coalesce timers - the delay is runtime configurable so a factor 2 > is taken to provide the range. > > Signed-off-by: Nicholas Mc Guire > --- I think this driver is in maintenance mode. I'll wait for Ulf to ack or review this change before applying. > diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c > index 7c0403b..a1fa3fb 100644 > --- a/drivers/clk/ux500/clk-sysctrl.c > +++ b/drivers/clk/ux500/clk-sysctrl.c > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) > clk->reg_bits[0]); > > if (!ret && clk->enable_delay_us) > - usleep_range(clk->enable_delay_us, clk->enable_delay_us); > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2); Please add space around that multiply.
[PATCH RFC] clk: ux500: add range to usleep_range
Providing a range for usleep_range() allows the hrtimer subsystem to coalesce timers - the delay is runtime configurable so a factor 2 is taken to provide the range. Signed-off-by: Nicholas Mc Guire --- Problem located with an experimental coccinelle script Q: Basically usleep_range() with min == max never makes much sense notably in non-atomic context. If the factor of 2 is tolerable or a fixed offset of e.g. 1000 would be more suitable is not clear to me - maybe someone familiar with that driver can clarify this. Patch was compile tested with: u8500_defconfig (implies COMMON_CLK=y) (with some sparse warnings about not implemented system calls) Patch is against 5.1-rc3 (localversion-next is next=20190405) drivers/clk/ux500/clk-sysctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c index 7c0403b..a1fa3fb 100644 --- a/drivers/clk/ux500/clk-sysctrl.c +++ b/drivers/clk/ux500/clk-sysctrl.c @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw) clk->reg_bits[0]); if (!ret && clk->enable_delay_us) - usleep_range(clk->enable_delay_us, clk->enable_delay_us); + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2); return ret; } -- 2.1.4
