On Tue, Jan 31, 2012 at 05:34:02PM +0100, Sylwester Nawrocki wrote: > Hi Kukjin, > > I have few comments below... > > On 01/31/2012 04:50 PM, Kukjin Kim wrote: > > From: Sangsu Park <sangsu4u.park@xxxxxxxxxxx> > > > > This patch adds follwing. > > s/follwing/following. > > nit: AFAIK it's a good habit not to start a commit description > with "This patch.." > "This patch..." descriptions are just fine. I write a lot of patches that way, but I do expect description of not just what a patch does, but why the patch is necessary. > > -/* EXYNOS4 GPIO number definitions */ > > +/* GPIO number definitions */ > > #define EXYNOS4_GPA0(_nr) (EXYNOS4_GPIO_A0_START + (_nr)) > > #define EXYNOS4_GPA1(_nr) (EXYNOS4_GPIO_A1_START + (_nr)) > > #define EXYNOS4_GPB(_nr) (EXYNOS4_GPIO_B_START + (_nr)) > > @@ -140,10 +139,138 @@ enum s5p_gpio_number { > > > > /* the end of the EXYNOS4 specific gpios */ > > #define EXYNOS4_GPIO_END (EXYNOS4_GPZ(EXYNOS4_GPIO_Z_NR) + 1) > > -#define S3C_GPIO_END EXYNOS4_GPIO_END > > > > -/* define the number of gpios we need to the one after the GPZ() range */ > > -#define ARCH_NR_GPIOS (EXYNOS4_GPZ(EXYNOS4_GPIO_Z_NR) + \ > > - CONFIG_SAMSUNG_GPIO_EXTRA + 1) > > +/* EXYNOS5 serise */ > > +/* GPIO bank sizes */ > > +#define EXYNOS5_GPIO_A0_NR (8) > > nit: It's been always a mystery to me, what are the parentheses around the > numbers helpful for ? IMHO even if there is more things like this in > the file it might be better to skip extra parentheses here. It protects against the preprocessor combining a macro with other code in unpredictable ways. For example: #define SIZE 10 + 20 int i = SIZE * 5; Without the parenthesis the result of i is 110, when the programmer would expect 150. For single integers like these, the parenthesis aren't actually necessary, but I given that for every other #define it is good practice, I don't object to seeing them on single integers also. g. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html