[U-Boot] [PATCH v6 1/4] EXYNOS5: Add gpio pin numbering feature

[Przemyslaw Marczak]

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 <rajeshwari.s at samsung.com>
>
> 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 <l.krishna at samsung.com>
> Signed-off-by: Rajeshwari Shinde <rajeshwari.s at samsung.com>
> Signed-off-by: Akshay Saraswat <akshay.s at samsung.com>
> ---

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