| Message ID | 69073c98beae28a6a75103dd202ec2a6b8b1a674.1379063063.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 09/13/2013 07:02 AM, Viresh Kumar wrote: > Most of the drivers do following in their ->target_index() routines: > > struct cpufreq_freqs freqs; > freqs.old = old freq... > freqs.new = new freq... > > cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); > > /* Change rate here */ > > cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); > > This is replicated over all cpufreq drivers today and there doesn't exists a > good enough reason why this shouldn't be moved to cpufreq core instead. > > Earlier patches have added support in cpufreq core to do cpufreq notification on > frequency change, this one removes it from this driver. > > Some related minor cleanups are also done along with it. > diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c > @@ -121,21 +117,10 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy, > else > clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */ > > - cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); ... > - cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); I wonder if this series is bisectable? Perhaps I should just go and read the rest of the series, but I presume there's a patch somewhere else that adds those two cpufreq_notify_transition() to the cpufreq core. Either that happens before this patch (in which case listeners will get two notifications each time; perhaps that is safe?), or after this patch (in which case with just this patch applied, no notifications will be sent until a later patch! Aside from that, all the Tegra-specific patches in this series, Acked-by: Stephen Warren <swarren@nvidia.com>
On 14 September 2013 04:22, Stephen Warren <swarren@wwwdotorg.org> wrote: > I wonder if this series is bisectable? Perhaps I should just go and read > the rest of the series, but I presume there's a patch somewhere else > that adds those two cpufreq_notify_transition() to the cpufreq core. > Either that happens before this patch (in which case listeners will get > two notifications each time; perhaps that is safe?), or after this patch > (in which case with just this patch applied, no notifications will be > sent until a later patch! Hmm.. Good Catch.. So, yes git bisect would be compilable but not runnable.. As we are already serialized notifications and so two PRE notifications will generate a crash.. But I don't want to get all that in a single patch as that would be: 40 files changed, 192 insertions(+), 623 deletions(-) And that would be hard to review it.. Any suggestions? > Aside from that, all the Tegra-specific patches in this series, > Acked-by: Stephen Warren <swarren@nvidia.com> Thanks..
On Sat, Sep 14, 2013 at 09:39:31AM +0530, Viresh Kumar wrote: > On 14 September 2013 04:22, Stephen Warren <swarren@wwwdotorg.org> wrote: > > I wonder if this series is bisectable? Perhaps I should just go and read > > the rest of the series, but I presume there's a patch somewhere else > > that adds those two cpufreq_notify_transition() to the cpufreq core. > > Either that happens before this patch (in which case listeners will get > > two notifications each time; perhaps that is safe?), or after this patch > > (in which case with just this patch applied, no notifications will be > > sent until a later patch! > > Hmm.. Good Catch.. > > So, yes git bisect would be compilable but not runnable.. As we are > already serialized notifications and so two PRE notifications will > generate a crash.. > > But I don't want to get all that in a single patch as that would be: > > 40 files changed, 192 insertions(+), 623 deletions(-) > > And that would be hard to review it.. > > Any suggestions? > It reminds me time of removing CONFIG_HOTPLUG and following __dev* attributes. At least stats for 48c68c4 "Drivers: video: remove __dev* attributes" is: 135 files changed, 1017 insertions(+), 1129 deletions(-) Maybe coccinelle script, which maintainers could run it on their trees, would help? Vladimir > > Aside from that, all the Tegra-specific patches in this series, > > Acked-by: Stephen Warren <swarren@nvidia.com> > > Thanks.. > > _______________________________________________ > linaro-kernel mailing list > linaro-kernel@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/linaro-kernel
On Saturday, September 14, 2013 09:39:31 AM Viresh Kumar wrote: > On 14 September 2013 04:22, Stephen Warren <swarren@wwwdotorg.org> wrote: > > I wonder if this series is bisectable? Perhaps I should just go and read > > the rest of the series, but I presume there's a patch somewhere else > > that adds those two cpufreq_notify_transition() to the cpufreq core. > > Either that happens before this patch (in which case listeners will get > > two notifications each time; perhaps that is safe?), or after this patch > > (in which case with just this patch applied, no notifications will be > > sent until a later patch! > > Hmm.. Good Catch.. > > So, yes git bisect would be compilable but not runnable.. As we are > already serialized notifications and so two PRE notifications will > generate a crash.. > > But I don't want to get all that in a single patch as that would be: > > 40 files changed, 192 insertions(+), 623 deletions(-) > > And that would be hard to review it.. > > Any suggestions? Well, I guess you can assume that everyone has a chance to review the series by now and send it as one patch in the next iteration. A patch that adds 192 lines of code isn't shockingly large by any measure. Thanks, Rafael
On 15 September 2013 10:35, Rafael J. Wysocki <rjw@sisk.pl> wrote: > Well, I guess you can assume that everyone has a chance to review the series > by now and send it as one patch in the next iteration. Yeah, that's what I thought.. > A patch that adds 192 lines of code isn't shockingly large by any measure. It was more about removal of things and the number of files it touches.. But yes, its good for review to send them separately but in the end they can be merged..
diff --git a/drivers/cpufreq/tegra-cpufreq.c b/drivers/cpufreq/tegra-cpufreq.c index bd7d89c..f42df7e 100644 --- a/drivers/cpufreq/tegra-cpufreq.c +++ b/drivers/cpufreq/tegra-cpufreq.c @@ -102,12 +102,8 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy, unsigned long rate) { int ret = 0; - struct cpufreq_freqs freqs; - freqs.old = tegra_getspeed(0); - freqs.new = rate; - - if (freqs.old == freqs.new) + if (tegra_getspeed(0) == rate) return ret; /* @@ -121,21 +117,10 @@ static int tegra_update_cpu_speed(struct cpufreq_policy *policy, else clk_set_rate(emc_clk, 100000000); /* emc 50Mhz */ - cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); - -#ifdef CONFIG_CPU_FREQ_DEBUG - printk(KERN_DEBUG "cpufreq-tegra: transition: %u --> %u\n", - freqs.old, freqs.new); -#endif - - ret = tegra_cpu_clk_set_rate(freqs.new * 1000); - if (ret) { - pr_err("cpu-tegra: Failed to set cpu frequency to %d kHz\n", - freqs.new); - freqs.new = freqs.old; - } - - cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); + ret = tegra_cpu_clk_set_rate(rate * 1000); + if (ret) + pr_err("cpu-tegra: Failed to set cpu frequency to %lu kHz\n", + rate); return ret; }
Most of the drivers do following in their ->target_index() routines: struct cpufreq_freqs freqs; freqs.old = old freq... freqs.new = new freq... cpufreq_notify_transition(policy, &freqs, CPUFREQ_PRECHANGE); /* Change rate here */ cpufreq_notify_transition(policy, &freqs, CPUFREQ_POSTCHANGE); This is replicated over all cpufreq drivers today and there doesn't exists a good enough reason why this shouldn't be moved to cpufreq core instead. Earlier patches have added support in cpufreq core to do cpufreq notification on frequency change, this one removes it from this driver. Some related minor cleanups are also done along with it. Cc: Stephen Warren <swarren@nvidia.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/tegra-cpufreq.c | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-)