Re: [PATCH 3/6] cpufreq: governor: New data type for management part of dbs_data

[Rafael J. Wysocki]

On Thu, Mar 3, 2016 at 6:53 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> On 02-03-16, 03:08, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>
>> In addition to fields representing governor tunables, struct dbs_data
>> contains some fields needed for the management of objects of that
>> type.  As it turns out, that part of struct dbs_data may be shared
>> with (future) governors that won't use the common code used by
>> "ondemand" and "conservative", so move it to a separate struct type
>> and modify the code using struct dbs_data to follow.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> ---
>>  drivers/cpufreq/cpufreq_conservative.c |   15 +++--
>>  drivers/cpufreq/cpufreq_governor.c     |   90 ++++++++++++++++++++-------------
>>  drivers/cpufreq/cpufreq_governor.h     |   36 +++++++------
>>  drivers/cpufreq/cpufreq_ondemand.c     |   19 ++++--
>>  4 files changed, 97 insertions(+), 63 deletions(-)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
>> ===================================================================
>> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
>> +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
>> @@ -41,6 +41,13 @@
>>  /* Ondemand Sampling types */
>>  enum {OD_NORMAL_SAMPLE, OD_SUB_SAMPLE};
>>
>> +struct gov_tunables {
>> +     struct kobject kobj;
>> +     struct list_head policy_list;
>> +     struct mutex update_lock;
>> +     int usage_count;
>> +};
>
> Everything else looks fine, but I don't think that you have named it
> properly. Every thing else present in struct dbs_data are tunables,
> but not this. And so gov_tunables doesn't suit at all here..

So this is a totally bicycle shed discussion argument which makes it
seriously irritating.

Does it really matter so much how this structure is called?
Essentially, it is something to build your tunables structure around
and you can treat it as a counterpart of a C++ abstract class.  So the
name *does* make sense in that context.

That said, what about gov_attr_set?

Thanks,
Rafael