Re: [PATCH] ARM: EXYNOS5: Support Exynos5-bus devfreq driver

[Abhilash Kesavan]

On Sun, Dec 30, 2012 at 11:45 AM, Olof Johansson <olof@xxxxxxxxx> wrote:
> Hi,
Hi Olof,
>> +# ppmu support
>> +
>> +obj-$(CONFIG_EXYNOS5250_PPMU)                += exynos_ppmu.o exynos5_ppmu.o
>
> Quite obvious that it's ppmu support from the file names. No need for
> a comment.
Will improve the commenting in the file.
Also, will fix the whitespace, empty lines in the file.
>
>> diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c
>> index 578a610..a285080 100644
>> --- a/arch/arm/mach-exynos/common.c
>> +++ b/arch/arm/mach-exynos/common.c
>> @@ -308,6 +308,31 @@ static struct map_desc exynos5_iodesc[] __initdata = {
>>               .pfn            = __phys_to_pfn(EXYNOS5_PA_UART),
>>               .length         = SZ_512K,
>>               .type           = MT_DEVICE,
>> +     }, {
>> +             .virtual        = (unsigned long)S5P_VA_PPMU_CPU,
>> +             .pfn            = __phys_to_pfn(EXYNOS5_PA_PPMU_CPU),
>> +             .length         = SZ_8K,
>> +             .type           = MT_DEVICE,
>> +     }, {
>> +             .virtual        = (unsigned long)S5P_VA_PPMU_DDR_C,
>> +             .pfn            = __phys_to_pfn(EXYNOS5_PA_PPMU_DDR_C),
>> +             .length         = SZ_8K,
>> +             .type           = MT_DEVICE,
>> +     }, {
>> +             .virtual        = (unsigned long)S5P_VA_PPMU_DDR_R1,
>> +             .pfn            = __phys_to_pfn(EXYNOS5_PA_PPMU_DDR_R1),
>> +             .length         = SZ_8K,
>> +             .type           = MT_DEVICE,
>> +     }, {
>> +             .virtual        = (unsigned long)S5P_VA_PPMU_DDR_L,
>> +             .pfn            = __phys_to_pfn(EXYNOS5_PA_PPMU_DDR_L),
>> +             .length         = SZ_8K,
>> +             .type           = MT_DEVICE,
>> +     }, {
>> +             .virtual        = (unsigned long)S5P_VA_PPMU_RIGHT,
>> +             .pfn            = __phys_to_pfn(EXYNOS5_PA_PPMU_RIGHT),
>> +             .length         = SZ_8K,
>> +             .type           = MT_DEVICE,
>>       },
>
> You should add the ppmu device to the device tree, and get the addresses from
> there instead (via ioremap).
>
> That way you can make this driver probe using regular methods too.
Will re-work to remove these static mappings.
>
>> +
>> +#include <mach/exynos_ppmu.h>
>> +#include <mach/exynos5_ppmu.h>
>
> Can you avoid adding new mach includes for this, perhaps? We're working hard on
> removing them for all platforms, even though exynos is lagging behind on it.
> Local defines that are used in just one C file can either go in that file, or
> in a header file that sits next to it instead of in the shared directory. For
> the devfreq driver, include/linux/* is a better location.
Will do.
>
>> +#define FIXED_POINT_OFFSET 8
>> +#define FIXED_POINT_MASK ((1 << FIXED_POINT_OFFSET) - 1)
>
> 0xff. Easier to read for a single entry like this.
OK

Thanks,
Abhilash
--
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/