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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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.