Re: [PATCH] cpufreq: acpi: Add comment under ACPI_ADR_SPACE_SYSTEM_IO case

Rafael J. Wysocki
2013-10-12T20:41:08Z

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:

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.