Re: [RFC PATCH] PM / OPP: move cpufreq specific OPP functions out of generic OPP library

[Viresh Kumar]

On 2 May 2014 10:48, Nishanth Menon <nm@xxxxxx> wrote:
> On Thu, May 1, 2014 at 11:30 PM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>>> diff --git a/drivers/cpufreq/cpufreq_opp.c b/drivers/cpufreq/cpufreq_opp.c
>>> new file mode 100644
>>> index 0000000..2602ff8
>>> --- /dev/null
>>> +++ b/drivers/cpufreq/cpufreq_opp.c
>>> @@ -0,0 +1,102 @@
>>> +/*
>>> + * Generic OPP Interface for CPUFREQ drivers
>>> + *
>>> + * Copyright (C) 2009-2014 Texas Instruments Incorporated.
>>> + *     Nishanth Menon
>>> + *     Romit Dasgupta
>>> + *     Kevin Hilman
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>
>> I hope you have just copy pasted routines to this file, and haven't done
>> even the most minor modification in those, as its hard to review it.
>
> there is code replacement ofcourse ->
> * the logic of walking down the list holding a mutex has been replaced
> with rcu locks,
> * instead of reading internal data structure and generating the list,
> use the existing search API that does exactly the same.
> * Documentation update for the same.

Hmm, actually if I would have written this patch, then probably I would
have done the same thing, but looking from the reviewers perspective,
it would be much more easy if we can separate things into patches.

So, maybe do these changes first in opp.c only and then finally a
patch that just moves things around.

> Both are needed if you have to move the code out. functionally, both
> are equivalent

That's an assumption and we never know when we might have screwed
the code :) .. And so more careful review of those parts is required :)

>>> diff --git a/drivers/cpufreq/cpufreq_opp.h b/drivers/cpufreq/cpufreq_opp.h
>>
>> Two problems, driver may lie in arch/ as well, though we don't recommend
>> them, secondly move these in cpufreq.h, don't need a header here for sure.
>
> There are none at the moment. ideally, we'd like to discourage folks

Yes, we do. :)

> putting cpufreq drivers in arch/ given the amount of effort you have
> undertaken in bringing them all here. but I have personally no strong
> objection to getting rid of the private header and using the generic
> cpufreq header.
>
> Otherwise, I assume you are ok with this approach and will post a
> formal patch by monday.

Yep.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/