Re: [RFC PATCH v3 10/12] cpufreq: add hwdom-cpufreq driver

[Julien Grall <julien.grall@linaro.org>]

On 10/24/2014 02:05 PM, Oleksandr Dmytryshyn wrote:
> On Fri, Oct 24, 2014 at 2:45 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> On 10/24/2014 11:30 AM, Oleksandr Dmytryshyn wrote:
>>> On Thu, Oct 23, 2014 at 7:42 PM, Julien Grall <julien.grall@linaro.org> wrote:
>>>> Hi Oleksandr,
>>>>
>>>> On 10/23/2014 04:07 PM, Oleksandr Dmytryshyn wrote:
>>>>> This driver uses hwdom to change frequencies on CPUs
>>>>>
>>>>> Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
>>>>> ---
>>>>>  xen/Rules.mk                        |   1 +
>>>>>  xen/drivers/cpufreq/Makefile        |   1 +
>>>>>  xen/drivers/cpufreq/hwdom-cpufreq.c | 220 ++++++++++++++++++++++++++++++++++++
>>>>>  xen/include/public/xen.h            |   1 +
>>>>>  4 files changed, 223 insertions(+)
>>>>>  create mode 100644 xen/drivers/cpufreq/hwdom-cpufreq.c
>>>>>
>>>>> diff --git a/xen/Rules.mk b/xen/Rules.mk
>>>>> index 3b0b89b..cccbc72 100644
>>>>> --- a/xen/Rules.mk
>>>>> +++ b/xen/Rules.mk
>>>>> @@ -56,6 +56,7 @@ CFLAGS-$(perfc_arrays)  += -DPERF_ARRAYS
>>>>>  CFLAGS-$(lock_profile)  += -DLOCK_PROFILE
>>>>>  CFLAGS-$(HAS_ACPI)      += -DHAS_ACPI
>>>>>  CFLAGS-$(HAS_CPUFREQ)   += -DHAS_CPUFREQ
>>>>> +CFLAGS-$(HAS_HWDOM_CPUFREQ) += -DHAS_HWDOM_CPUFREQ
>>>>>  CFLAGS-$(HAS_PM)        += -DHAS_PM
>>>>>  CFLAGS-$(HAS_CPU_TURBO) += -DHAS_CPU_TURBO
>>>>>  CFLAGS-$(HAS_GDBSX)     += -DHAS_GDBSX
>>>>> diff --git a/xen/drivers/cpufreq/Makefile b/xen/drivers/cpufreq/Makefile
>>>>> index b87d127..891997c 100644
>>>>> --- a/xen/drivers/cpufreq/Makefile
>>>>> +++ b/xen/drivers/cpufreq/Makefile
>>>>> @@ -2,3 +2,4 @@ obj-y += cpufreq.o
>>>>>  obj-y += cpufreq_ondemand.o
>>>>>  obj-y += cpufreq_misc_governors.o
>>>>>  obj-y += utility.o
>>>>> +obj-$(HAS_HWDOM_CPUFREQ) += hwdom-cpufreq.o
>>>>> diff --git a/xen/drivers/cpufreq/hwdom-cpufreq.c b/xen/drivers/cpufreq/hwdom-cpufreq.c
>>>>> new file mode 100644
>>>>> index 0000000..67c9e1d
>>>>> --- /dev/null
>>>>> +++ b/xen/drivers/cpufreq/hwdom-cpufreq.c
>>>>> @@ -0,0 +1,220 @@
>>>>> +/*
>>>>> + *  Copyright (C) 2014 GlobalLogic Inc.
>>>>
>>>> A part of this file has been copied from xen/arch/x86/acpi/cpufreq.c. I
>>>> would keep the copyright from this file and add yours.
>>> I'll do this in the next patch-set.
>>>
>>>> Maybe we could share the initialization code (and others parts?) with
>>>> this file? For instance the structure looks the same...
>>> I don't think that we could simple share the initialization code and
>>> others parts.
>>> A lot of code looks the same. But I've introduced a new structure
>>> hwdom_cpufreq_data which has different field names (non-ACPI meaning).
>>
>> Except the name, the type of each fields are the same (except the third
>> one which doesn't exist here).
>>
>> IHMO, those names could be renamed if it could avoid to duplicate tens
>> lines of code.
>>
>> It would be easier for maintaining the code later.
> In this case I'll back to the 'acpi_cpufreq_data' structure and I'll try
> to create an additional file with common code for those drivers.

I'm not the maintainers, so I would wait any input from maintainers
before doing a such big change :).

Regards,

-- 
Julien Grall