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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAM4voakbSS08cJ19Yh1Vgh7E7w0VnPK42A3HzHg90Bm3qTzJ+Q@mail.gmail.com>
Date:	Mon, 31 Dec 2012 07:42:32 +0530
From:	Abhilash Kesavan <kesavan.abhilash@...il.com>
To:	Olof Johansson <olof@...om.net>
Cc:	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	kgene.kim@...sung.com, myungjoo.ham@...sung.com,
	kyungmin.park@...sung.com, rjw@...k.pl, jhbird.choi@...sung.com
Subject: Re: [PATCH] ARM: EXYNOS5: Support Exynos5-bus devfreq driver

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