On Sun, Dec 30, 2012 at 11:45 AM, Olof Johansson <o...@lixom.net> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
