policy->cur is now set by cpufreq core when cpufreq_driver->get() is defined and so drivers aren't required to set it. When space_id is ACPI_ADR_SPACE_SYSTEM_IO for acpi cpufreq driver it doesn't set ->get to a valid function pointer and so policy->cur is required to be set by driver.
This is already followed in acpi-cpufreq driver. This patch adds a comment describing why we need to set policy->cur from driver.
Suggested-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- This change was requested by Rafael here: http://www.spinics.net/lists/linux-acpi/msg46748.html
drivers/cpufreq/acpi-cpufreq.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index a8dac7b..8ecd74e 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -838,6 +838,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) switch (perf->control_register.space_id) { case ACPI_ADR_SPACE_SYSTEM_IO: /* Current speed is unknown and not detectable by IO port */ + /* ->cur wouldn't be set by core as ->get() is NULL */ policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu); break; case ACPI_ADR_SPACE_FIXED_HARDWARE:
On Saturday, October 12, 2013 07:06:03 AM Viresh Kumar wrote:
policy->cur is now set by cpufreq core when cpufreq_driver->get() is defined and so drivers aren't required to set it. When space_id is ACPI_ADR_SPACE_SYSTEM_IO for acpi cpufreq driver it doesn't set ->get to a valid function pointer and so policy->cur is required to be set by driver.
This is already followed in acpi-cpufreq driver. This patch adds a comment describing why we need to set policy->cur from driver.
Suggested-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
This change was requested by Rafael here: http://www.spinics.net/lists/linux-acpi/msg46748.html
drivers/cpufreq/acpi-cpufreq.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index a8dac7b..8ecd74e 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -838,6 +838,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy) switch (perf->control_register.space_id) { case ACPI_ADR_SPACE_SYSTEM_IO: /* Current speed is unknown and not detectable by IO port */
/* ->cur wouldn't be set by core as ->get() is NULL */
Well, please merge it with the existing comment and use the usual format for comments that are longer than two lines.
policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu); break;case ACPI_ADR_SPACE_FIXED_HARDWARE:
On 12/10/2013, Rafael J. Wysocki rjw@sisk.pl wrote:
Well, please merge it with the existing comment and use the usual format for comments that are longer than two lines.
I thought these are separate comments and so keeping them separate might be better, so that this one doesn't get deleted in case somebody is removing the other one.. and vice versa..
Anyway its fixed in attached commit now :)
commit ecb9ef81b50eb5e8559f7d132ef46803c8272091 Author: Viresh Kumar viresh.kumar@linaro.org Date: Sat Oct 12 07:00:01 2013 +0530
cpufreq: acpi: Add comment under ACPI_ADR_SPACE_SYSTEM_IO case
policy->cur is now set by cpufreq core when cpufreq_driver->get() is defined and so drivers aren't required to set it. When space_id is ACPI_ADR_SPACE_SYSTEM_IO for acpi cpufreq driver it doesn't set ->get to a valid function pointer and so policy->cur is required to be set by driver.
This is already followed in acpi-cpufreq driver. This patch adds a comment describing why we need to set policy->cur from driver.
Suggested-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/acpi-cpufreq.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index a8dac7b..d2df543 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -837,7 +837,12 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
switch (perf->control_register.space_id) { case ACPI_ADR_SPACE_SYSTEM_IO: - /* Current speed is unknown and not detectable by IO port */ + /* + * Current speed is unknown and not detectable by IO port. + * policy->cur wouldn't be set by core as cpufreq_driver->get() + * is NULL for this space_id and so we need to set policy->cur + * here. + */ policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu); break; case ACPI_ADR_SPACE_FIXED_HARDWARE:
On Saturday, October 12, 2013 08:58:10 PM Viresh Kumar wrote:
On 12/10/2013, Rafael J. Wysocki rjw@sisk.pl wrote:
Well, please merge it with the existing comment and use the usual format for comments that are longer than two lines.
I thought these are separate comments and so keeping them separate might be better,
No, comments that go like
/* Comment 1. */ /* Comment 2. */
do not even *look* separate.
In general you can do something like
/* * Comment 1. * * Comment 2. */
in such cases, but again in this particular case the comments aren't even logically separate in my opinion. ->
so that this one doesn't get deleted in case somebody is removing the other one.. and vice versa..
Anyway its fixed in attached commit now :)
commit ecb9ef81b50eb5e8559f7d132ef46803c8272091 Author: Viresh Kumar viresh.kumar@linaro.org Date: Sat Oct 12 07:00:01 2013 +0530
cpufreq: acpi: Add comment under ACPI_ADR_SPACE_SYSTEM_IO case policy->cur is now set by cpufreq core when cpufreq_driver->get()is defined and so drivers aren't required to set it. When space_id is ACPI_ADR_SPACE_SYSTEM_IO for acpi cpufreq driver it doesn't set ->get to a valid function pointer and so policy->cur is required to be set by driver.
This is already followed in acpi-cpufreq driver. This patch adds a comment describing why we need to set policy->cur from driver. Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
drivers/cpufreq/acpi-cpufreq.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c index a8dac7b..d2df543 100644 --- a/drivers/cpufreq/acpi-cpufreq.c +++ b/drivers/cpufreq/acpi-cpufreq.c @@ -837,7 +837,12 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
switch (perf->control_register.space_id) { case ACPI_ADR_SPACE_SYSTEM_IO:
/* Current speed is unknown and not detectable by IO port */
/** Current speed is unknown and not detectable by IO port.* policy->cur wouldn't be set by core as cpufreq_driver->get()* is NULL for this space_id and so we need to set policy->cur* here.*/
-> What about this:
/* * The core will not set policy->cur, because cpufreq_driver->get is NULL, * so we need to set it here. However, we have to guess it, because the * current speed is unknown and not detectable via IO ports. */
policy->cur = acpi_cpufreq_guess_freq(data, policy->cpu); break; case ACPI_ADR_SPACE_FIXED_HARDWARE:
On 13/10/2013, Rafael J. Wysocki rjw@sisk.pl wrote:
-> What about this:
/* * The core will not set policy->cur, because cpufreq_driver->get is NULL, * so we need to set it here. However, we have to guess it, because the * current speed is unknown and not detectable via IO ports. */
Far better. Attached again. Thanks.
On 13 October 2013 06:07, Viresh Kumar viresh.kumar@linaro.org wrote:
On 13/10/2013, Rafael J. Wysocki rjw@sisk.pl wrote:
-> What about this:
/* * The core will not set policy->cur, because cpufreq_driver->get is NULL, * so we need to set it here. However, we have to guess it, because the * current speed is unknown and not detectable via IO ports. */Far better. Attached again. Thanks.
Have you missed applying this one? Attached again..
On Monday, October 21, 2013 12:43:32 PM Viresh Kumar wrote:
On 13 October 2013 06:07, Viresh Kumar viresh.kumar@linaro.org wrote:
On 13/10/2013, Rafael J. Wysocki rjw@sisk.pl wrote:
-> What about this:
/* * The core will not set policy->cur, because cpufreq_driver->get is NULL, * so we need to set it here. However, we have to guess it, because the * current speed is unknown and not detectable via IO ports. */Far better. Attached again. Thanks.
Have you missed applying this one? Attached again..
I don't think I've missed it. It should be commit 1bab64d in my tree.
Thanks!
On 22 October 2013 04:09, Rafael J. Wysocki rjw@sisk.pl wrote:
I don't think I've missed it. It should be commit 1bab64d in my tree.
I fetched your tree yesterday and this wasn't there in linux-next or bleeding-edge branch.. When I fetched it now again, it is there..
Thanks.
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Viresh Kumar