Hi Simon,
Thank you for comments.
On Sun, Jan 27, 2013 at 1:29 AM, Simon Glass <s...@chromium.org> wrote:
> Hi Rajeshwari,
>
> On Wed, Jan 23, 2013 at 2:48 AM, Rajeshwari Shinde
> <rajeshwar...@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 <rajeshwar...@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.
>
>> 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?
>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
--
Regards,
Rajeshwari Shinde
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
