On Saturday, October 12, 2013 08:58:10 PM Viresh Kumar wrote:
> On 12/10/2013, Rafael J. Wysocki <rjw@xxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxx>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
