[U-Boot] [PATCH 3/3 V2] EXYNOS5: GPIO: Enable GPIO Command for EXYNOS5
Simon Glass
sjg at chromium.org
Wed Feb 6 05:00:40 CET 2013
Hi Rajeshwari,
On Sun, Feb 3, 2013 at 10:35 PM, Rajeshwari Birje
<rajeshwari.birje at gmail.com> wrote:
> Hi Simon,
>
> Thank you for comments.
>
> On Sun, Jan 27, 2013 at 1:29 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Rajeshwari,
>>
>> On Wed, Jan 23, 2013 at 2:48 AM, Rajeshwari Shinde
>> <rajeshwari.s at samsung.com> wrote:
>>> This patch enables GPIO Command for EXYNOS5.
>>> Function has been added to asm/gpio.h to decode the
>>> input gpio name to gpio number.
>>> example: gpio set gpa00
>>> GPIO_INPUT in cmd_gpio.c has been modified to
>>> GPIO_DIRECTION_INPUT as GPIO_INPUT is alraedy defined in
>>> exynos5 and leading to a error.
>>>
>>> Signed-off-by: Rajeshwari Shinde <rajeshwari.s at samsung.com>
>>
>> Great to see this - some comments below.
>>
>>> ---
>>> Chnages in V2:
>>> - New patch
>>> arch/arm/include/asm/arch-exynos/gpio.h | 88 +++++++++++++++++++++++++++++++
>>> common/cmd_gpio.c | 6 +-
>>> include/configs/exynos5250-dt.h | 1 +
>>> 3 files changed, 92 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/arch-exynos/gpio.h b/arch/arm/include/asm/arch-exynos/gpio.h
>>> index af882dd..66e4a8e 100644
>>> --- a/arch/arm/include/asm/arch-exynos/gpio.h
>>> +++ b/arch/arm/include/asm/arch-exynos/gpio.h
>>> @@ -657,6 +657,94 @@ static inline unsigned int s5p_gpio_part_max(int nr)
>>> void gpio_cfg_pin(int gpio, int cfg);
>>> void gpio_set_pull(int gpio, int mode);
>>> void gpio_set_drv(int gpio, int mode);
>>> +
>>> +#include <common.h>
>>> +
>>> +static inline int name_to_gpio(const char *name)
>>> +{
>>> + int num;
>>> +
>>> + name++;
>>> +
>>> + if (*name == 'p') {
>>> + ++name;
>>> +
>>> + switch (*name) {
>>> + case 'a':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_A00 + num;
>>> + break;
>>> + case 'b':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_B00 + num;
>>> + break;
>>> + case 'c':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 10);
>>> + if (num >= 40)
>>> + num = GPIO_C40 + (num - 40);
>>> + else {
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_C00 + num;
>>> + }
>>> + break;
>>> + case 'd':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_D00 + num;
>>> + break;
>>> + case 'y':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_Y00 + num;
>>> + break;
>>> + case 'x':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_X00 + num;
>>> + break;
>>> + case 'e':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_E00 + num;
>>> + break;
>>> + case 'f':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_F00 + num;
>>> + break;
>>> + case 'g':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_G00 + num;
>>> + break;
>>> + case 'h':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_H00 + num;
>>> + break;
>>> + case 'v':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_V00 + num;
>>> + break;
>>> + case 'z':
>>> + name++;
>>> + num = simple_strtoul(name, NULL, 8);
>>> + num = GPIO_Z0 + num;
>>> + break;
>>> + default:
>>
>> It seems like you need a table (C struct) containing the GPIO letter
>> and the associated GPIO_... value. Then this code could become a for()
>> loop to search for the match. This would reduce code duplication. 'c'
>> would presumable still be a special case.
> -Okay
>>
>>> + return -1;
>>> + }
>>> + } else
>>> + return -1;
>>> +
>>> + return num;
>>> +}
>>> +
>>> +#define name_to_gpio(n) name_to_gpio(n)
>>> #endif
>>>
>>> /* Pin configurations */
>>> diff --git a/common/cmd_gpio.c b/common/cmd_gpio.c
>>> index 47eee89..450e6d1 100644
>>> --- a/common/cmd_gpio.c
>>> +++ b/common/cmd_gpio.c
>>> @@ -16,7 +16,7 @@
>>> #endif
>>>
>>> enum gpio_cmd {
>>> - GPIO_INPUT,
>>> + GPIO_DIRECTION_INPUT,
>>
>> Actually I think it is exynos that should change - perhaps put an
>> EXYNOS prefix on that one.
> Ya but since we use a generic driver for S5P only renaming with EXYNOS
> prefix is not correct.
> Hence I had renamed in cmd_gpio.c file as it would not effect anything else.
Sure, but that is a generic file - do you should not rename it to fit
with exynos! Maybe use a different prefix, but I really think it
should be your code that changes!
>>
>>> GPIO_SET,
>>> GPIO_CLEAR,
>>> GPIO_TOGGLE,
>>> @@ -44,7 +44,7 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>
>>> /* parse the behavior */
>>> switch (*str_cmd) {
>>> - case 'i': sub_cmd = GPIO_INPUT; break;
>>> + case 'i': sub_cmd = GPIO_DIRECTION_INPUT; break;
>>> case 's': sub_cmd = GPIO_SET; break;
>>> case 'c': sub_cmd = GPIO_CLEAR; break;
>>> case 't': sub_cmd = GPIO_TOGGLE; break;
>>> @@ -63,7 +63,7 @@ static int do_gpio(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>> }
>>>
>>> /* finally, let's do it: set direction and exec command */
>>> - if (sub_cmd == GPIO_INPUT) {
>>> + if (sub_cmd == GPIO_DIRECTION_INPUT) {
>>> gpio_direction_input(gpio);
>>> value = gpio_get_value(gpio);
>>> } else {
>>> diff --git a/include/configs/exynos5250-dt.h b/include/configs/exynos5250-dt.h
>>> index 7b9c393..b20b8f2 100644
>>> --- a/include/configs/exynos5250-dt.h
>>> +++ b/include/configs/exynos5250-dt.h
>>> @@ -113,6 +113,7 @@
>>> #define CONFIG_CMD_EXT2
>>> #define CONFIG_CMD_FAT
>>> #define CONFIG_CMD_NET
>>> +#define CONFIG_CMD_GPIO
>>>
>>> #define CONFIG_BOOTDELAY 3
>>> #define CONFIG_ZERO_BOOTDELAY_CHECK
>>> --
>>> 1.7.4.4
>>>
>>
>> Can you also please implement gpio_info(), so it will list out the
>> GPIOs by name, and their state?
> Okay would take it up as separate patch.
> What details would we exactly print in state info?
GPA00 input 0
GPA01 input 1
GPA02 output 1
etc...
Regards,
Simon
More information about the U-Boot
mailing list