[U-Boot] [PATCH 1/3 V4] EXYNOS5: Add gpio pin numbering feature

[Minkyu Kang]

Dear Rajeshwari Birje,

On 02/04/13 14:08, Rajeshwari Birje wrote:
> Hi Minkyu Kang,
> 
> Thank you for comments.
> 
> On Mon, Apr 1, 2013 at 5:27 PM, Minkyu Kang <mk7.kang at samsung.com> wrote:
>> On 21/03/13 20:33, Rajeshwari Shinde wrote:
>>> This patch adds support for gpio pin numbering support on
>>> EXYNOS5250
>>> 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>
>>> ---
>>> Changes in V2:
>>>         - none.
>>> Changes in V3:
>>>         - none.
>>> Changes in V4:
>>>       - To have consistent 0..n-1 GPIO numbering the banks are divided
>>>       into different parts where ever they have holes in them.
>>>       - Combined previous patch 1 and 2 into single patch.
>>>  arch/arm/cpu/armv7/exynos/pinmux.c      |  148 +++++-------
>>>  arch/arm/include/asm/arch-exynos/cpu.h  |   10 +-
>>>  arch/arm/include/asm/arch-exynos/gpio.h |  376 ++++++++++++++++++++++++++++++-
>>>  board/samsung/smdk5250/smdk5250.c       |   24 +--
>>>  drivers/gpio/s5p_gpio.c                 |   65 ++++++-
>>>  5 files changed, 508 insertions(+), 115 deletions(-)
>>>
>>> diff --git a/drivers/gpio/s5p_gpio.c b/drivers/gpio/s5p_gpio.c
>>> index 656bf4a..1b882cb 100644
>>> --- a/drivers/gpio/s5p_gpio.c
>>> +++ b/drivers/gpio/s5p_gpio.c
>>> @@ -36,6 +36,27 @@
>>>  #define RATE_MASK(x)         (0x1 << (x + 16))
>>>  #define RATE_SET(x)          (0x1 << (x + 16))
>>>
>>> +
>>> +struct gpio_info {
>>> +     unsigned int reg_addr;  /* Address of register for this part */
>>> +     unsigned int max_gpio;  /* Maximum GPIO in this part */
>>> +};
>>> +
>>> +#ifdef CONFIG_EXYNOS5
>>
>> I think, this ifdef is unnecessary.
> I had put this #ifdef as EXYNOS4 will also have a gpio_data structure.

Exynos doesn't allow ifdefs on own drivers.
Please don't use it.

>>
>>> +static const struct gpio_info gpio_data[EXYNOS5_GPIO_NUM_PARTS] = {
>>
>> maybe it should be exynos5_gpio_data?
> No kept it as gpio_data as it would b easy to call the same from
> s5p_gpio_get_bank function and exynos4 will also have a separate
> structure.

I suggest that make two structure exynos5_gpio_data and exynos4_gpio_data,
and please remove ifdef.

>>
>>> +     { EXYNOS5_GPIO_PART1_BASE, EXYNOS5_GPIO_MAX_PORT_PART_1 },
>>> +     { EXYNOS5_GPIO_PART2_BASE, EXYNOS5_GPIO_MAX_PORT_PART_2 },
>>> +     { EXYNOS5_GPIO_PART3_BASE, EXYNOS5_GPIO_MAX_PORT_PART_3 },
>>> +     { EXYNOS5_GPIO_PART4_BASE, EXYNOS5_GPIO_MAX_PORT_PART_4 },
>>> +     { EXYNOS5_GPIO_PART5_BASE, EXYNOS5_GPIO_MAX_PORT_PART_5 },
>>> +     { EXYNOS5_GPIO_PART6_BASE, EXYNOS5_GPIO_MAX_PORT_PART_6 },
>>> +     { EXYNOS5_GPIO_PART7_BASE, EXYNOS5_GPIO_MAX_PORT_PART_7 },
>>> +     { EXYNOS5_GPIO_PART8_BASE, EXYNOS5_GPIO_MAX_PORT },
>>> +};
>>> +
>>> +#define HAVE_GENERIC_GPIO
>>> +#endif
>>> +
>>>  void s5p_gpio_cfg_pin(struct s5p_gpio_bank *bank, int gpio, int cfg)
>>>  {
>>>       unsigned int value;
>>> @@ -141,7 +162,30 @@ void s5p_gpio_set_rate(struct s5p_gpio_bank *bank, int gpio, int mode)
>>>
>>>       writel(value, &bank->drv);
>>>  }
>>> -
>>> +#ifdef HAVE_GENERIC_GPIO
>>> +static struct s5p_gpio_bank *s5p_gpio_get_bank(unsigned int gpio)
>>
>> please consider exynos4 also.
> Yes will update the same for exynos4 and send as soon as possible.

So, please modify this function and resend.

>>
>>> +{
>>> +     const struct gpio_info *data;
>>> +     unsigned int upto;
>>> +     int i;
>>> +
>>> +     for (i = upto = 0, data = gpio_data; i < EXYNOS5_GPIO_NUM_PARTS;

here. EXYNOS5 stuff.

>>> +                     i++, upto = data->max_gpio, data++) {
>>> +             debug("i=%d, upto=%d\n", i, upto);
>>> +             if (gpio < data->max_gpio) {
>>> +                     struct s5p_gpio_bank *bank;
>>> +                     bank = (struct s5p_gpio_bank *)data->reg_addr;
>>> +                     bank += (gpio - upto) / GPIO_PER_BANK;
>>> +                     debug("   gpio=%d, bank=%p\n", gpio, bank);
>>> +                     return bank;
>>> +             }
>>> +     }
>>> +#ifndef CONFIG_SPL_BUILD
>>> +     assert(gpio < EXYNOS5_GPIO_MAX_PORT);   /* ...which it will not be */
>>> +#endif
>>> +     return NULL;
>>> +}
>>> +#else
>>>  struct s5p_gpio_bank *s5p_gpio_get_bank(unsigned gpio)
>>>  {
>>>       int bank;
>>> @@ -151,6 +195,7 @@ struct s5p_gpio_bank *s5p_gpio_get_bank(unsigned gpio)
>>>       bank *= sizeof(struct s5p_gpio_bank);
>>>       return (struct s5p_gpio_bank *) (s5p_gpio_base(gpio) + bank);
>>>  }
>>> +#endif
>>>
>>>  int s5p_gpio_get_pin(unsigned gpio)
>>>  {
>>> @@ -196,3 +241,21 @@ int gpio_set_value(unsigned gpio, int value)
>>>
>>>       return 0;
>>>  }
>>> +
>>> +void gpio_set_pull(int gpio, int mode)
>>> +{
>>> +     s5p_gpio_set_pull(s5p_gpio_get_bank(gpio),
>>> +                     s5p_gpio_get_pin(gpio), mode);
>>> +}
>>> +
>>> +void gpio_set_drv(int gpio, int mode)
>>> +{
>>> +     s5p_gpio_set_drv(s5p_gpio_get_bank(gpio),
>>> +                     s5p_gpio_get_pin(gpio), mode);
>>> +}
>>> +
>>> +void gpio_cfg_pin(int gpio, int cfg)
>>> +{
>>> +     s5p_gpio_cfg_pin(s5p_gpio_get_bank(gpio),
>>> +                     s5p_gpio_get_pin(gpio), cfg);
>>> +}
>>>
>>
>> Thanks,
>> Minkyu Kang.
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> 

Thanks,
Minkyu Kang.