From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Mon, 14 Apr 2014 17:15:19 +0200 Subject: [U-Boot] [PATCH v6 1/4] EXYNOS5: Add gpio pin numbering feature In-Reply-To: <1397295812-4010-2-git-send-email-Akshay.s@samsung.com> References: <1397295812-4010-1-git-send-email-Akshay.s@samsung.com> <1397295812-4010-2-git-send-email-Akshay.s@samsung.com> Message-ID: <534BFB87.5020503@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello, I like this idea. This is a good feature for easy and fast gpio maintaining. I have few comments to this. On 04/12/2014 11:43 AM, Akshay Saraswat wrote: > From: Rajeshwari Shinde > > This patch adds gpio pin numbering support for EXYNOS 5250 & 5420. > To have consistent 0..n-1 GPIO numbering the banks are divided > into different parts where ever they have holes in them. > > Signed-off-by: Leela Krishna Amudala > Signed-off-by: Rajeshwari Shinde > Signed-off-by: Akshay Saraswat > --- You use quite magic numbers in gpio names like "EXYNOS5_GPIO_A05", maybe better is to add "PIN" word here like this: EXYNOS5_GPIO_A0_PIN5. So then we really know what we are using and I think this just looks better. > diff --git a/arch/arm/include/asm/arch-exynos/gpio.h b/arch/arm/include/asm/arch-exynos/gpio.h > index d6868fa..211383d 100644 > --- a/arch/arm/include/asm/arch-exynos/gpio.h > +++ b/arch/arm/include/asm/arch-exynos/gpio.h > @@ -141,14 +141,16 @@ struct exynos5420_gpio_part1 { > It seems that those all exynos5*_gpio_partX structures and also exynos5*_gpio_get() macros can be removed now. > struct exynos5420_gpio_part2 { > struct s5p_gpio_bank y7; /* 0x1340_0000 */ > - struct s5p_gpio_bank res[0x5f]; /* */ > +}; > + > +struct exynos5420_gpio_part3 { > struct s5p_gpio_bank x0; /* 0x1340_0C00 */ > struct s5p_gpio_bank x1; /* 0x1340_0C20 */ > struct s5p_gpio_bank x2; /* 0x1340_0C40 */ > struct s5p_gpio_bank x3; /* 0x1340_0C60 */ > }; > > -struct exynos5420_gpio_part3 { > +struct exynos5420_gpio_part4 { > struct s5p_gpio_bank c0; > struct s5p_gpio_bank c1; > struct s5p_gpio_bank c2; > @@ -164,7 +166,7 @@ struct exynos5420_gpio_part3 { > struct s5p_gpio_bank y6; > }; > > -struct exynos5420_gpio_part4 { > +struct exynos5420_gpio_part5 { > struct s5p_gpio_bank e0; /* 0x1400_0000 */ > struct s5p_gpio_bank e1; /* 0x1400_0020 */ > struct s5p_gpio_bank f0; /* 0x1400_0040 */ > @@ -175,7 +177,7 @@ struct exynos5420_gpio_part4 { > struct s5p_gpio_bank j4; /* 0x1400_00E0 */ > }; > > -struct exynos5420_gpio_part5 { > +struct exynos5420_gpio_part6 { > struct s5p_gpio_bank z0; /* 0x0386_0000 */ > }; > > @@ -200,16 +202,20 @@ struct exynos5_gpio_part1 { > struct s5p_gpio_bank y4; > struct s5p_gpio_bank y5; > struct s5p_gpio_bank y6; > - struct s5p_gpio_bank res1[0x3]; > +}; > + > +struct exynos5_gpio_part2 { > struct s5p_gpio_bank c4; > - struct s5p_gpio_bank res2[0x48]; > +}; > + > +struct exynos5_gpio_part3 { > struct s5p_gpio_bank x0; > struct s5p_gpio_bank x1; > struct s5p_gpio_bank x2; > struct s5p_gpio_bank x3; > }; > > -struct exynos5_gpio_part2 { > +struct exynos5_gpio_part4 { > struct s5p_gpio_bank e0; > struct s5p_gpio_bank e1; > struct s5p_gpio_bank f0; > @@ -221,20 +227,25 @@ struct exynos5_gpio_part2 { > struct s5p_gpio_bank h1; > }; > > -struct exynos5_gpio_part3 { > +struct exynos5_gpio_part5 { > struct s5p_gpio_bank v0; > struct s5p_gpio_bank v1; > - struct s5p_gpio_bank res1[0x1]; > +}; > + > +struct exynos5_gpio_part6 { > struct s5p_gpio_bank v2; > struct s5p_gpio_bank v3; > - struct s5p_gpio_bank res2[0x1]; > +}; > + > +struct exynos5_gpio_part7 { > struct s5p_gpio_bank v4; > }; > > -struct exynos5_gpio_part4 { > +struct exynos5_gpio_part8 { > struct s5p_gpio_bank z; > }; > There are also unchanged gpios initializations in files: - arndale/arndale.c line 19 - smdk5420/smdk5420.c line 45 Thanks -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com